SQL Injection

drewbee · 06-25-2008, 02:01 PM

Hey everyone, I am looking for some ideas or opinions on how well my secure my scripting is.

Should a query come from GET, POST, or any other means...

I always do this in the queries.

PHP Code:


			
$_GET['group_id'] = isset($_GET['group_id']) ? (int)$_GET['group_id'] : 0;
$_POST['username'] = isset($_POST['username']) ? $_POST['username'] : '';
 
SELECT        account_id, username, account_group
FROM          users
WHERE        belongs_to_group = '" . $_GET['group_id'] . "' AND
                  username = '" . mysql_real_escape_string($_POST['username']) . "'

I know the query doesn't really make sense... it doesn't need too. Display purposes only.

As well, the username value would have validation from the form (overflow, and proper regex validation against the username).

I let the query fail should a user enter a string through proper handling though as it (atleast to my understanding) will be looking for '0' if anything but an integer is entered.

so, since no rows were found...

PHP Code:


			
if (mysql_num_rows($query) > 0)
{
    // bla hbal hbalh do stuff
}
else
{
     $this->throwError('Invalid account information supplied');
}

Also, my form class (everytime form input is sent in, all values are automatically trim'ed and strip_slashed for redisplay (if form error) / processing (if passed form validation).

Thoughts? Ideas? Suggestions to better protect myself? I am just getting ready to start another project and want to get even tighter security implemented if possible.

drewbee · 06-25-2008, 05:54 PM

To clean things up even a bit more, I am now going to use sprintf to write queries. Is this really necessary? I see them doing this in the manual, and have never done it myself.

PHP Code:


			
// No need for casting as %d in sprintf will take care ofit.
$_GET['group_id'] = isset($_GET['group_id']) ? $_GET['group_id'] : 0;
$_POST['username'] = isset($_POST['username']) ? $_POST['username'] : '';
 
$query = sprintf("SELECT   *  
                  FROM     users 
                  WHERE    user_name='%s' AND 
                           user_group='%d'",
         mysql_real_escape_string($_POST['user_name']),
         $_GET['user_group']
                 );

ryanmr · 06-25-2008, 06:17 PM

I'm not sure if sprintf covers this but I normally use is_numeric for ids and some regex for usernames.
In anycase, you can look at this cheat sheet: 0x000000 # The Hacker Webzine - SQL Injection Cheat Sheet, it shows you a lot of different ideas.

Wildhoney · 06-25-2008, 07:44 PM

In the past, before I switched over to Zend Framework, I ran all my PHP variables, to be used in a MySQL statement, through the following function. I'm not sure if it's actually 100% secure, but it seemed to be. I think Salathe, a while ago, pointed out some problems regarding different file-types, such as boolean values, but I don't believe anybody actually discovered any such major flaws.

It'll serve as a good base nonetheless, for if you wish to add onto it, or switch some things around, or merely use it as a reference.

php Code:

function mysql_parse_value($value, $strip_tags=true, $allowable_tags=null)
{
    if (is_array($value))
    {
        return; 
    }
    
    if (get_magic_quotes_gpc())
    {
        $value = stripslashes($value);
    }
         
    if ($strip_tags)
    {
        $value = strip_tags($value, $allowable_tags);
    }
        
    if (!is_numeric($value))
    {
     $value = "'" . mysql_real_escape_string($value) . "'";
    }

    return $value;
}

I did use it in conjunction with sprintf, as you mentioned. This function places in any quotes for you, and so you shouldn't actually put in any quotes into sprintf's first argument.

To give an example:

php Code:

$szMyVar1 = 'TalkPHP.com';
$szMyVar2 = 'TalkRSI.com';

$szSQL = sprintf
(
    "SELECT * FROM myTable WHERE myColumn = %s OR myColumn = %s",
    mysql_parse_value($szMyVar1), mysql_parse_value($szMyVar2)
);

maZtah · 06-25-2008, 07:46 PM

Just a little note: with mysql you don't have to put quotes around integers. So user_group = %d will do the job, no need to put quotes around it!

Futhermore, it looks quite safe to me.

Edit:
Wildhoney won the race to mention about the quotes. ;)

drewbee · 06-25-2008, 08:20 PM

Yeah, I know you don't have to put quotes around integer values. For some reason, I always have. Not sure why. Just one of those things I started doing and formed a habit around it. Is there any actual benefit to not using quotes around numeric values?

maZtah · 06-26-2008, 08:59 AM

Quote:

Originally Posted by drewbee

Yeah, I know you don't have to put quotes around integer values. For some reason, I always have. Not sure why. Just one of those things I started doing and formed a habit around it. Is there any actual benefit to not using quotes around numeric values?

For me it's just about "correct" coding, a string is a string, and a integer is an integer. I have no idea if there's any benefit if you're putting quotes around it or not. Though, logically it's more correct (to me).

Wildhoney · 06-25-2008, 10:06 PM

None as far as I can see. I thought maybe MySQL wouldn't be able to perform mathematical equations on numbers which guise themselves as strings, but it appears I am wrong, as both of these work correctly:

sql Code:

UPDATE myTable SET myCount = myCount + "2"
SELECT "1" + "10"

Kalle · 06-26-2008, 03:33 AM

I don't even bother to make all the *_real_escape_string() functions, I've designed an input filtering class that does it all for me and I just call it like:

PHP Code:


			
$tuxxedo->input->doSqlSafe('p', Array('input1', 'input2', '...'));

The 'p' tells my class it has to clean post, it can also combine with more like 'pg' (POST + GET or simply r for REQUEST (POST / GET / COOKIE)) and then it modifies the global scope variables so I don't need to care about them when calling the query after.

Using something like this made my coding much easier ;)

drewbee · 06-26-2008, 01:32 PM

Yeah, I have a database abstraction class too. I just happened to use mysql_real_escape_string in the example for better understanding;

usually, my queries look like:

$this->db->query(sprintf("SELECT blah blah blah x = '%s'", $this->db->safe($_POST['var'])));

I love it because if all is well with the query, it just returns and is ready for processing but if an error happens (crucial) if I have debug turned on it will output what i passed in as the query, error message etc. In production, it sends an email to me so I can catch SQL Injection attempts and the like.

Plus should I ever change databases i simply need to change the code in the query()method. Gotta love it:)

blakew · 07-03-2008, 01:18 PM

I have been using my own escape functions on $_GET and $_POST before running them in queries but I am migrating over to the Zend Framework. Is this still necessary or does the Zend Framework take care of that for me?

Jim · 07-03-2008, 02:20 PM

@ Blakew: How about running a test-query via the Zend Framework? That way you'll have your awnser in a few minutes :)

(I don't know)