views:

267

answers:

7

I know that mysql_real_escape_string()
prepends backslashes to the following characters: \x00, \n, \r, \, ', " and \x1a

I know how this protects a query from injection into something like a variable in a where clause. But here's a scenario I am unsure of:

$query = "SELECT * FROM $db WHERE 1";

If $db is taken from a user input, then the user could insert something like:
$db = 'RealDatabase WHERE 1; DELETE FROM RealDatabase WHERE 1; SELECT FROM RealDatabase';

From my understanding, mysql_real_escape_string() would not affect this string, making the final query: $query = "SELECT * FROM RealDatabase WHERE 1; DELETE FROM RealDatabase WHERE 1; SELECT FROM RealDatabase WHERE 1";

which would delete the database. Is there another level of protection I am unaware of?

A: 

Since table names do not accept whitespace characters, just strip them out. That would make the above $DB RealDatabaseWHERE1;DELETEFROMRealDatabase..... Such would invalidate the query, but prevent the flaw.

If you want to prevent this kind of 'hackish' things, just do explode(' ', $db) then get the result array's [0]. That would get the first part (RealDatabase) and nothing else.

Jimmie Lin
Are you sure MySQL does not accept spaces in table names? I have used them successfully in the past.
Brian
+1  A: 

You should really look into binding your SQL queries.

This will protect you from basically all SQL injection. It boils down to this:

(taken from PHP.net)

$stmt = mssql_init('NewUserRecord');

// Bind the field names
mssql_bind($stmt, '@username',  'Kalle',  SQLVARCHAR,  false,  false,  60);

// Execute
mssql_execute($stmt);

And PHP has support for binded queries on basically all databases. Oh and of course you should still sanitize all input & output(display).

More info: - http://php.net/manual/en/function.mssql-bind.php

Mr-sk
You can't use query parameters for table names or column names or SQL keywords.
Bill Karwin
this does not solve the OP's problem
DeveloperChris
A: 

Its just best to use it any time that you have questionable data being used. If you are specifying the table yourself and there's no room for tampering, there's no need to escape it. If your users are deciding anything that could potentially get run as a query, escape it.

Citizen
I know it's good practice to escape all queries, but would escaping be enough to prevent an attack in the scenario I drew up?
Brian
+4  A: 

The level of protection you are looking for is supplied by backticks:

"SELECT * FROM `$db` WHERE 1";

Backticks are used to qualify identifiers that could otherwise be ambiguous (ie. MySQL reserved words), and if you are accepting user input or have variably-named columns or databases, you absolutely should use backticks, or I can promise that you will run into trouble in the future. For example, what if you had a system where a temporary field name was created with some user input, only it turned out the field ended up being named update?

"SELECT field1,field2,update FROM table;"

It fails miserably. However:

"SELECT `field`,`field2`,`update` FROM table"

works just fine. (This is actually a real example from a system I worked on a few years ago that had this problem).

This solves your problem in terms of putting in bad SQL. For instance, the following query will simply return an "unknown column" error, where test; DROP TABLE test is the injected attack code:

"SELECT * FROM `test; DROP TABLE test`;"

Be careful though: SQL Injection is still possible with backticks!

For instance, if your $db variable contained data that had a backtick in it, you could still inject some SQL in the normal way. If you're using variable data for database and field names, you should strip it of all backticks before putting it into your statement, and then qualifying it with backticks once inside.

$db = str_replace('`','',$db);
$sql = "SELECT * FROM `$db` WHERE 1";

I utilize a database wrapper which has separate functions for sanitizing data and sanitizing database identifiers, and this is what the latter does :)

zombat
adding backticks makes little difference. you state it yourself the table name still needs to be escaped in some way, so the backticks while a good coding practice actually adds nothing in this instance. and worse than that the name of the table is leaked to the user. who knows what they could infer from that. the trouble with escaping is you have no way of knowing what future exploits may be discovered.
DeveloperChris
A: 

If you really really must use a get from the user (bad bad bad) for your database then use the following style of coding...

$realname = '';
switch ($_get['dbname']){
    case 'sometoken' : $realname = 'real_name'; break;
    case 'sometoken1' : $realname = 'real_name1'; break;
    case 'sometoken2' : $realname = 'real_name2'; break;
    case 'sometoken3' : $realname = 'real_name3'; break;
    case 'sometoken4' : $realname = 'real_name4'; break;
    case default : die ('Cheeky!!!');
}

$query = "SELECT * FROM `{$realname}` WHERE 1";

or alternativally ...

$realname = $tablenames[$_get['dbname']];

if (!$realname)
    die ('Cheeky!!!');

Using these 2 ways or some similar coding will protect your input from unexpected values.

it also means the user never gets to see the real table or database names which they may be able to infer information from.

Make sure you check the content of $_get['dbname'] to make sure its valid first otherwise warnings will be issued.

I still say this is a very bad design, it is reminiscent of allowing users to provide a filename and passing that through to io functions without a check. it simply too unsafe to consider.

Security is too important to let laziness rule.

DC

DeveloperChris
Same comment here as I made to Micheal Madsen's answer. You're forcing an unnecessary link between your code and your database, and with this method you couldn't update one without the other. As long as you do proper escaping, there's no reason you can't try a query with dynamic table/field names, and if the query fails, you simply catch the error and return the message. Obviously prepared queries are better suited for this, but there's no reason you can't do it with the regular MySQL driver as well.
zombat
unnecessary link between code and database? Except with stored procedures (not applicable in this case) everytime you alter your db you must alter your code. You do realise the OP wanted to get his table names from the user. At best a very risky move. Prepared queries don't work with dynamic table names nor do stored procedures. My answer was to satisfy the OP's requirements AND provide a modicum of safety. Your comment is as irrelevant to my answer as it is to Micheal's
DeveloperChris
Just a correction to my comment above, You can do this in a stored procedure by building the query string within the stored procedure, but you are back to square one, you still need to escape the user input otherwise the sql injections will still be possible.
DeveloperChris
+1  A: 

Instead of inserting the database name in the get query you can make a separate table of database names and ids. Then append only the id to the query. Then you can look up the corresponding database name for that id and use that. You can then make sure that the id received is numeric (is_numeric) and you can also be certain that the user can only choose from the databases that are in your list.

(Additionally this will prevent users from finding out names of databases and possibly use them elsewhere in an SQL injection on your site.)

Using the first method you parse the database name before using it in your query and make sure it contains no spaces.

Chaim Chaikin
Originally I said you should make sure that all database names are purely text and do not contain the ; character but it turns out that database and table names can have the ; character in their name.
Chaim Chaikin
+1  A: 

No, mysql_real_escape_string isn't going to help you here. The function is not context-sensitive (it can't be, because it doesn't HAVE any context), and this is a completely different threat model.

You need to go and validate that the table exists, without sending the user-inputted table name directly to the server. The best solution is to use a server-side array/look-up table containing the table names they are allowed to use. If they try to use something that's not in there, then don't let them.

If you really need ALL of the tables, then you can just ask the server "what tables do you have?" and run through it's output (optionally caching it for some period of time to prevent asking the server every time) - but chances are, eventually you'll have a table that you don't want then to poke around in, and then you need to use the array thing anyway, so just go ahead and do that.

Michael Madsen
I'd disagree with this solution. You're just adding overhead, and forcing an unnecessary correlation between your code and your database. Every time you added a table, you'd have to update your code array. There's no reason you can't try a query with dynamic table/field names, and if the query fails, you simply catch the error and return the message.
zombat