PHP - SQL Injection?Is this code safe?

godolfin

Hi All,

I have some code that someone told me was susceptible to a SQL injection attack. I am basically using a variable passed to the PHP file in a querystring as part of a SELECT statement. I have made some changes which I think will help and it now looks like this:

<?php
$value = $_GET['id'];
//quote_smart($value);
function quote_smart($value)
{
// Stripslashes
if (get_magic_quotes_gpc()) {
$value = stripslashes($value);
}
// Quote if not a number or a numeric string
if (!is_numeric($value)) {
$value = "'" . mysql_real_escape_string($value) . "'";
}
return $value;
}

// Connect
$link = mysql_connect("localhost","xxxxxxxxxxxxx","xxxxxxxxxx") OR die(mysql_error());

mysql_select_db("xxxxxxxxxxxx", $link);

// Make a safe query
$query = sprintf("SELECT * FROM rss WHERE ID=".$value."");

$result = mysql_query($query);

Can anyone tell me if the above code is susceptible to a SQL injection attack?

Thanks a lot in advance,

Ray

NogDog

If you use the quote_smart() function you defined, then it should be OK:

$query = sprintf("SELECT * FROM rss WHERE ID=%s", quote_smart($value));

blackhorse

His function doesn't make sure the value is numeric, and he calls the value in the sql as a numeric value, will that be the security problem?

NogDog

blackhorse wrote:
His function doesn't make sure the value is numeric, and he calls the value in the sql as a numeric value, will that be the security problem?

The way the function works, SQL injection will not be an issue. However, some validation of the input data would be in order to ensure it is of the correct type and range of values, e.g.:

$value = trim($_GET['id']);
if(!ctype_digit($value) or $value < 0)
{
   // output error here for invalid ID
}
else
{
   // do the DB query, etc.
}

blackhorse

If he doesn't use the ctype_digit check on the numeric value for the query like his codes, what if input the ID value $var="1, OR 1=1"

Will the query become

SELECT * FROM rss WHERE ID=1 OR 1=1

To prevent the sql injection attacks, should we both need use mysql_real_escape_string on the string values in the sql. and is_numeric or ctype_digit checkinng on the numeric values in the sql?

MarkR

Firstly, I implore you, don't make your application compatible with magic_quotes_gpc. If magic_quotes_gpc is on, just quit with an error before doing anything. It's evil. Most schemes to "fix" magic quotes have bugs in themselves.

Secondly, using such a "universal" escaping function is naive in the extreme. You must escape things before they go into the database, NOT after they come out of the request.

Things need to be escaped differently depending on where they're going - so you can't have such a "Universal" function.

Third, I really, really suggest you just use parameterised queries every, it's much easier and more likely to work.

Mark

godolfin

Hi again,

Thanks again for your help with this, your advice is much appreciated. I'm afraid that my PHP knowlegde is not great and to be honest it is going over my head. I was hoping that Mark you could maybe demonstrate the 3 points you were talking about in your reply by changing my code which is as follows:

// Connect
$link = mysql_connect("localhost","xxxxxx","xxxxxxxx") OR die(mysql_error());

mysql_select_db("xxxxxxxxx", $link);

// Make a safe query
$query = sprintf("SELECT * FROM rss WHERE ID=%s", quote_smart($value)); 

$result = mysql_query($query);

Again many thanks for your responses and help.

Ray

NogDog

MarkR wrote:
Firstly, I implore you, don't make your application compatible with magic_quotes_gpc. If magic_quotes_gpc is on, just quit with an error before doing anything. It's evil. Most schemes to "fix" magic quotes have bugs in themselves.
...

Please explain with concrete examples.

Not everyone has the luxury of controlling the environment under which their applications will be executed, or perhaps they are writing scripts they hope to distribute to any who want to use it (either open-source or licensed) that they'd like to work in as wide a range of environments as possible. Why not make such an application robust enough to deal with the situation if it occurs? It's not rocket science: you check to see if magic_quotes_gpc is turned on; and if it is, negate its effect, then proceed as though it was not turned on. What's the big deal? Am I missing something? Should I stop trying to make my scripts as robust as possible and instead make them only work if the user's configuration is exactly the way I prefer it to be?

MarkR

NogDog wrote:
Please explain with concrete examples.

Basically what I was getting at is, magic_quotes_gpc fixup routines typically don't traverse the $GET $POST, $REQUEST $COOKIES etc arrays correctly, thus missing some elements, particularly if there are nested elements. Recall that you can do:

<input type="text" name="something[1][qwer][]">

And it will populate (e.g.) the next element of the array $_POST['something'][1]['qwer'].

The problem I have is testing: It is not feasible to rigorously test an application with every possible combination of ini settings that exists - even with a small subset, a test setup would be a maintenance nightmare, EVEN if the testing was 100% automated.

Testing typically takes far longer than coding on a reasonable sized project anyway, and introducing new bugs into a complex system is extremely easy.

I always try to minimise the amount of testing required, above most other requirements.

Not everyone has the luxury of controlling the environment under which their applications will be executed, or perhaps they are writing scripts they hope to distribute to any who want to use it (either open-source or licensed) that they'd like to work in as wide a range of environments as possible.

Because "as wide a range of environments as possible" is precisely one. Testing will kill your developers' time if you try to support even a very small number of different environments.

I consider this environmental control a necessity rather than a luxury. My development time is far more valuable than any expense notionally saved by deploying on to a substandard platform.

Why not make such an application robust enough to deal with the situation if it occurs?

Because that is additional code which needs to be maintained in a working state.

I do try to make things as robust as possible generally, but not at the expense of additional maintenance work.

It's not rocket science: you check to see if magic_quotes_gpc is turned on; and if it is, negate its effect, then proceed as though it was not turned on.

The algorithm is surprisingly nontrivial. Some implementations I've seen are wrong. Yours may be correct.

Should I stop trying to make my scripts as robust as possible and instead make them only work if the user's configuration is exactly the way I prefer it to be?

Obviously your requirements WILL be different from mine, you should use your judgement.

If it's a setting you can easily workaround, you may prefer to work-around it. Some settings (e.g. allow_url_fopen) can't be worked around by design. I consider, for instance, writing my own HTTP client is an unnecessary piece of code when I can simply use fopen (and of course, have my app "fail fast" if it's disabled).

Settings that can be changed at runtime can just be changed at runtime- which is nice and safe (provided you check that ini_set succeeded).

Mark

Sxooter

MarkR wrote:
Firstly, I implore you, don't make your application compatible with magic_quotes_gpc. If magic_quotes_gpc is on, just quit with an error before doing anything. It's evil. Most schemes to "fix" magic quotes have bugs in themselves.

Mark

MarkR is absolutely right on this. If magic quotes are on, all bets for security are off.

MarkR

Having magic quotes on doesn't cause a security problem (at least, it shouldn't). What it does do is cause a major data integrity problem adding backslashes all over the place where they shouldn't be - if these are saved into a database, all sorts of trouble can ensue trying to repair the damage.

I'm very keen on the principle of "fail fast" - that's to say, if something is broken it should fail very quickly and with a very big, loud error. This prompts a) the users to notice it and complain about it and b) the developers to be able to fix it more easily.

The nastiest errors are ones which generate no noise whatsoever, just stealthily wreck your data. Therefore we should strive towards the loud, immediate and safe type.

I've seen countless sites which suffer from "backslashitis" (Facebook being a notable popular example) - where data are contaminated by progressively more and more backslashes.

Mark

laserlight

What it does do is cause a major data integrity problem adding backslashes all over the place where they shouldn't be - if these are saved into a database, all sorts of trouble can ensue trying to repair the damage.

I agree, but this is still a problem in PHP5: magic quotes may be deprecated and set to off by default, but since it is not yet removed, a user may be using a host that has turned it back on. It is back to the chicken and egg problem with PHP -> PHP5. If you support magic quotes, then users will be happy, but they will not pressure their hosts to change to better settings (or upgrade their PHP installations, as the case may be). If you do not support magic quotes, then users will be unhappy, but they may complain to their hosts as well and get some momentum for change that benefits everybody.