sanitize json input

[ilbiffi]

Hi,
I want to sanitize json strings encoded by my application, my concern is that an attacker could forge a fake json string to launch malicious code.
Can you please validate my method?
This is what I do (I attach only the relevant portions of the script):

$json = trim($_POST['json']);
$data = json_decode($json, true);
$st = mysqli_prepare($link, 'INSERT INTO `a`.`b` (x,y,z) VALUES (?, ?, ?)');
mysqli_stmt_bind_param($st, 'sss', $x, $y, $z);
$data = json_decode($json, true);
    foreach ($data as $row) {
        $x1 = $row['x'];
        $x2 = trim($x1);
        $x = mysqli_real_escape_string($link, $x2);
        $y1 = $row['y'];
        $y2 = trim($y1);
        $y = mysqli_real_escape_string($link, $y2);
        $z1 = $row['z'];
        $z2 = trim($z1);
       $z = mysqli_real_escape_string($link, $z2);
  mysqli_stmt_execute($st);
}

 

So basically I trim the raw json string, then I extract all the data and just before executing the insert I sanitize the data.
It works because my db gets updated, I was wondering if it works also from a security perspective. My understanding is that "mysqli_real_escape_string" is a sort of general purpose sanitize function right?
Thanks

[Jacques1]

mysqli_real_escape_string() is a mostly obsolete function for manual SQL-escaping (hence the name). It was needed in the early days of PHP when prepared statements weren't supported yet. Since we do have prepared statements now, it's pretty much useless.

 

So, no, this function does not magically “sanitize” data (whatever that means). In fact, you cannot use it in conjuction with a prepared statement, because it will prepend literal backslashes to all quotes and other special characters, damaging your data.

 

Simply use the prepared statement you already have and get rid of SQL escaping. Also get rid of the duplicate json_decode() call (unless that's just a copy-and-paste error).

Edited March 7, 2016 by Jacques1

[ilbiffi]

mysqli_real_escape_string() is a mostly obsolete function for manual SQL-escaping (hence the name). It was needed in the early days of PHP when prepared statements weren't supported yet. Since we do have prepared statements now, it's pretty much useless.

 

So, no, this function does not magically “sanitize” data (whatever that means). In fact, you cannot use it in conjuction with a prepared statement, because it will prepend literal backslashes to all quotes and other special characters, damaging your data.

 

Simply use the prepared statement you already have and get rid of SQL escaping. Also get rid of the duplicate json_decode() call (unless that's just a copy-and-paste error).

 

I know that my data must have a certain format, so any kind of special character to me is not valid; I know my windows client application is well-behaved but if someone tries to execute my public page with my post variables and inserts malicious characters shouldn't I protect my web app?

What do you mean by duplicate json_decode, my understanding was I needed it to "explode" a json string that contains several rows of data. My usage here is that once all the rows are extracted, every field must be cleaned before insertion; why a prepared statement alone should be considered enough to protect my db?

Thanks

[Barand]

 

$json = trim($_POST['json']);

$data = json_decode($json, true);

$st = mysqli_prepare($link, 'INSERT INTO `a`.`b` (x,y,z) VALUES (?, ?, ?)');

mysqli_stmt_bind_param($st, 'sss', $x, $y, $z);

$data = json_decode($json, true);

    foreach ($data as $row) {

 

Does that make it clearer what he means by duplicate json_decode() ?

[ilbiffi]

Does that make it clearer what he means by duplicate json_decode() ?

 

Yep, that's a copy and paste error.

[Jacques1]

@ilbiffi:

 

You should forget this notion of “malicious characters” which somehow need to be “cleaned”. This is naive and misleading.

 

Input data is just that: data. It may contain all kinds of characters, and that's perfectly fine. In fact, we would be in trouble if we weren't allowed to use quotes and other “special characters” in this forum.

 

Security issues occur when data is misinterpreted as code. If, for example, you were to insert the decoded data straight into your query, you would indeed create an SQL injection vulnerability, because now the data is misinterpreted as part of the query. But you don't do that. By passing all dynamic input to the parameters of the prepared statement, you ensure that the data will always be treated as data and not executed in any way. There's no injection risk.

 

You should, however, check if the JSON-decoding actually succeeded. Otherwise you'll get weird errors if the input is invalid.

Edited March 7, 2016 by Jacques1

[ilbiffi]

@ilbiffi:

 

You should forget this notion of “malicious characters” which somehow need to be “cleaned”. This is naive and misleading.

 

Input data is just that: data. It may contain all kinds of characters, and that's perfectly fine. In fact, we would be in trouble if we weren't allowed to use quotes and other “special characters” in this forum.

 

Security issues occur when data is misinterpreted as code. If, for example, you were to insert the decoded data straight into your query, you would indeed create an SQL injection vulnerability, because now the data is misinterpreted as part of the query. But you don't do that. By passing all dynamic input to the parameters of the prepared statement, you ensure that the data will always be treated as data and not executed in any way. There's no injection risk.

 

You should, however, check if the JSON-decoding actually succeeded. Otherwise you'll get weird errors if the input is invalid.

 

Super-clear, thanks.

So the correct way is simply this prepared statement

 

$st = mysqli_prepare($link, 'INSERT INTO `a`.`b` (x,y,z) VALUES (?, ?, ?)');

mysqli_stmt_bind_param($st, 'sss', $x, $y, $z);

$data = json_decode($json, true);

    foreach ($data as $row) {

        $x = $row['x'];

        $y = $row['y'];

        $z = $row['z'];

  mysqli_stmt_execute($st);

}

 

and nothing else, right?

Edited March 7, 2016 by ilbiffi

[Jacques1]

Yes. A few things:

• Do the JSON-decoding on top of the script, check the return value to detect errors and log the exact error so that you have a chance to actually fix the problem. Without explicit error handling, PHP will crash somewhere and give you nothing but a cryptic error message.
• Avoid the use of backticks in queries, because this is unnecessary (as long as you use standard identifiers, which you should) and tends to mask errors.

Edited March 7, 2016 by Jacques1