-3

I know its probably unconventional but I want to know if the below code is secure or not.

First piece of code is htee jquery object creation plus the call to the retrieve_data function:

var dataset = [
        {
            query_column: "articles.id,articles.category_id,articles.text,articles.slug article_slug,site_categories.title,site_categories.slug site_categories_slug",
            table_name: 'articles',
            query_join: 'LEFT JOIN site_categories ON site_categories.Id = ' + category,
            query_filter: ['articles.category_id LIKE ', '%' + category + '%'],
            query_limit: 'LIMIT ' + limit,
            unique_column_switch: '1'
        }
    ];
    retrieve_data(dataset, function (data) {

Next is the retrieve_data function itself:

function retrieve_data(dataset, callback) {
    $.ajax(
        {
            type: "POST",
            url: "<?php echo ROOT_URL; ?>php/content/retrieve_data.php",
            data: {json: JSON.stringify(dataset)},
            success: function (data) {
                var data = $.parseJSON(data);
                callback(data);
            }
        });
}

Finally the php that retrieves the data and prints it out for the return to jquery:

mb_internal_encoding("UTF-8");
session_start();
include '../../config.php';
include ROOT_DIR . "php/dbconnection/dbconnection.php";
include ROOT_DIR . 'php/authentication/encryption.php';
$encrypt_decrypt = new encryption();
$json = json_decode($_POST['json']);
$array = array();

/*
 * THIS IS TO BUILD A STRING OF DIFFERENT QUERIES TO BE PERFORMED UPON UPDATE BEING PRESSED
 * PARAMS:
 * data_value:::::::::::: THE VALUE USED TO FIND THE ROW
 * table_name:::::::::::: TABLE NAME
 * unique_column::::::::: UNIQUE DATA ELEMENT THAT LINKS ALL THE TABLES TOGETHER
 * query_end::::::::::::: END OF QUERY (EXTRA WHERE CLAUSES, ORDER BY, LIMIT, ETC)
 * query_column:::::::::: COLUMNS THAT ARE GOING TO BE CALLED, DEFAULTS TO * IF USING JOINS THEN      THIS MUST BE SPECIFIED I.E. TABLE1.*, TABLE2.*, ETC
 * query_join:::::::::::: SET ANY JOINS HERE
 * unique_column_switch:: IF SET TO 1 DISABLES USE OF A UNIQUE COLUMN AND USES QUERY END EXCLUSIVELY
 */
foreach($json as $item){
    $table_name = $mysqli->real_escape_string($item->table_name);
    $unique_column = $mysqli->real_escape_string($item->unique_column);
$data_value = $mysqli->real_escape_string($item->data_value);
$query_column = $mysqli->real_escape_string($item->query_column);
$query_join = $mysqli->real_escape_string($item->query_join);
$query_filter = $item->query_filter;
$query_order = $mysqli->real_escape_string($item->query_order);
$query_limit = $mysqli->real_escape_string($item->query_limit);
$unique_column_switch = $mysqli->real_escape_string($item->unique_column_switch);
$query_filter_safe = array();
foreach($query_filter as $key1 => $val1){
    array_push($query_filter_safe, ($key1 % 2) ? "'" . $mysqli->real_escape_string($val1) . "'" : $mysqli->real_escape_string($val1));
}
if(empty($unique_column) && $unique_column_switch != '1'){
    $query1 = $mysqli->query("SHOW KEYS FROM `$table_name` WHERE Key_name = 'PRIMARY'");
    $fetch1 = $query1->fetch_array(MYSQLI_ASSOC);
    $unique_set = $fetch1['Column_name'] . " = '" . $data_value . "'";
    $unique_column = $fetch1['Column_name'];
} else{
    $unique_set = ($unique_column_switch != '1') ? "`" . $table_name . "`.`" . $unique_column . "` = '" . $data_value . "'" : '';
}
$unique_column = (empty($unique_column)) ? '' : $unique_column;
$where = (empty($unique_set) && empty($query_filter)) ? '' : 'WHERE';
$select_items = (empty($query_column)) ? '*' : $query_column;
$query2 = "SELECT " . $select_items . " FROM " . $table_name . " " . $query_join . " " . $where . " " . $unique_set . " " . join(' ', $query_filter_safe) . " " . $query_order . " " . $query_limit;
//echo $query2;
$query2 = $mysqli->query($query2);
for($x = 0; $fetch2 = $query2->fetch_array(MYSQLI_ASSOC); $x++){
    $fetch2 = $encrypt_decrypt->decrypt_val($fetch2, $table_name, $mysqli);
    foreach($fetch2 as $column => $value){
        ($unique_column == $column) ? $array[$table_name][$x]['INDEX_VALUE'] = $value : $array[$table_name][$x][$column] = $value;
    }
}
}
echo json_encode($array);

EDIT 12/5 12:00PM EST

I have rewritten what I was trying to do. Thanks again for pointers everyone! @MonkeyZeus and @Carth were extremely useful.

include '../../config.php';
include ROOT_DIR . "php/dbconnection/dbconnection_pdo.php";
$query = "SELECT * FROM site_users WHERE username = :username";
$query = $pdo->prepare($query);
$query->execute(array('username' => $_POST['username']));
$result = $query->fetchAll(PDO::FETCH_ASSOC);
echo json_encode($result);

Going back to jquery:

function article_box_basic(category, limit, max_char_count, location) {
    $.ajax(
        {
            type: "POST",
            url: "<?php echo ROOT_URL; ?>php/content/article_box_basic.php",
            data: {username: 'moltmans'},
            success: function (data) {
                var data = $.parseJSON(data);

Do something here with data

Brad Larson
  • 170,088
  • 45
  • 397
  • 571
  • why would you build queries on client side? Cant you just pass parameters and build query on server side? – GGio Dec 05 '13 at 14:14
  • 6
    Are you trying to win an award for `least secure website`? – MonkeyZeus Dec 05 '13 at 14:15
  • The escaping mechanism is designed to escape a single token (think a string literal), *not* fragments of a query spanning multiple tokens. Also, nothing in your code stops a user from passing a JOIN you don't expect (e.g. to the users table). – DCoder Dec 05 '13 at 14:16
  • MonkeyZeus Im asking a question your the reason people hate using this site. Secondly DCoder thanks for the repsonse. – Mike Oltmans Dec 05 '13 at 14:36
  • 1
    I am not sorry and kudos to DCoder for being able to phrase my thoughts in a nicer tone. There are simply too many basic pre-requisites which were either completely missed or blatantly ignored. Please stop what you're doing and don't touch another piece of code until you have gone through http://stackoverflow.com/questions/14880531/passing-mysql-query-via-javascript and then http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php – MonkeyZeus Dec 05 '13 at 14:49
  • Thanks for that @MonkeyZeus again posting snarkey remarks doesn't help anyone. Your above response is extremely useful. Just trying to use the best practice for this. Also the code I wrote still works with just PHP side practices so its not exactly going to waste. – Mike Oltmans Dec 05 '13 at 15:09
  • 1
    You're not the first and I'm sure you won't be the last to try this. I'm just here to put a simple big red `Caution: HOT` sign on an open flame rather than stating `Caution: the difference between the body's upper temperature tolerance level is far exceeded by the temperature provided by this flame` – MonkeyZeus Dec 05 '13 at 15:24
  • Well stick around I'm rewriting let me know if this doesn't get a red hot from you :) – Mike Oltmans Dec 05 '13 at 15:43
  • I hope you know that I am always willing to help. And even if you personally vow to never make a comment like the one I did, one day you will see something that will make you at least think it. I would have been doing you a disservice if I used a less blunt comment because I knew that there would be some fine and knowledgeable people willing to take the long route with this question – MonkeyZeus Dec 05 '13 at 16:10
  • Yes I suppose but please keep in mind not everyone is as hardy as me and a person that has been cyberbullied before would take that completely to left field (reason why it gets me annoyed). Outside of that @MonkeyZeus please look at my edit to original with some new code. – Mike Oltmans Dec 05 '13 at 16:59
  • Question to OP and @Carth - Why can't you use :xxx in two places in pdo? – Mike Oltmans Dec 05 '13 at 17:12
  • Taking a quote from Thor `This code, I like it!`. Please feel free to start a new question on SO with the new code and I will be willing to help, it will also simply gain attention from other users because it will be at the top of the list for PHP, MySQL, PDO, AJAX, jQuery tags, FYI people love to see proper use of PHP PDO =) – MonkeyZeus Dec 05 '13 at 17:18

1 Answers1

5

This is emphatically not a "secure" approach. Validation and control on the client side should be treated as an inherently insecure convenience for generating a request not a means of enforcing true security. Your server side code should be validating the request parameters within the context of who your user is and what they're doing. Since a user can set "dataset" to whatever they want it doesn't matter if the category variable is itself prone to injection based on its usage in the rest of the statement.

By exposing your schema on the client side like this you're revealing valuable information that there's no need to expose.

Carth
  • 2,303
  • 1
  • 17
  • 26
  • I think your answer is correct. The problem is that the solution can be rather difficult and costly. I was wondering if OP can force `$mysql->query` to execute only one query, thus avoiding the injection of other malicious executions inside the same query. – Tomás Dec 05 '13 at 14:26
  • So I am just playing with this on my site I can easily take what I built and utilize it purely php side. I was just wondering if there was a way to make this secure or if its completely impossible? – Mike Oltmans Dec 05 '13 at 14:35
  • The implementation here could certainly attempt to ensure that only a single statement is executed but encapsulating additional statements to be run via injection is only one type of injection attack. Since the current approach allows an attacker to set the statement to issue an query they desire the system is already compromised. GGio's comment about only allowing users to provide parameters is the typical approach for limiting the exposure of your underlying system to the client-side. Check this out- http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php – Carth Dec 05 '13 at 14:35
  • @MikeOltmans - Totally understand, Mike. Most reactions, myself included are based on the idea of "if this were in production". The link in my previous comment to the SO thread in regard to locking down MYSQL/PHP statements is a good source for what you could do here to help improve the site security. – Carth Dec 05 '13 at 14:38
  • But in reality using this is simply not secure regardless of what I do. This approach just makes it simpler to handle quick queries needed during a site page being live on someones browser. – Mike Oltmans Dec 05 '13 at 14:43
  • I see clear now. As stated in this answer the implementation should be changed, because I can easily `JOIN` with other tables and retrieve compromised data if I want. – Tomás Dec 05 '13 at 15:41
  • @Tomás `mysqli::query` does already perform only one single statement. For multiple statements you would need `mysqli::multi_query` instead. – Gumbo Dec 05 '13 at 19:03