views:

60

answers:

2

Is it a good, or stupid idea to sanitize all the data that could be sqlinjected? I wrote a function that should do it, but I've never seen it done and was wondering if it was a poor idea. The function I wrote:

function sanitizeData()
{
    $_SERVER['HTTP_USER_AGENT'] = mysql_real_escape_string($_SERVER['HTTP_USER_AGENT']);
    foreach(array_keys($_COOKIE) as $key)
    {
          $_COOKIE[$key] = mysql_real_escape_string($_COOKIE[$key]);
    }
    foreach(array_keys($_POST) as $key)
    {
          $_POST[$key] = mysql_real_escape_string($_POST[$key]);
    }    
    foreach(array_keys($_GET) as $key)
    {
          $_GET[$key] = mysql_real_escape_string($_GET[$key]);
    }
}
+3  A: 

A bad idea; this is basically another version of the deprecated magic_quotes. Most of that data probably won't end up going into the database, so you'll end up escaping unnecessarily, and potentially double-escaping.

Instead, use prepared statements as needed. Look at mysqli_stmt (part of mysqli) and PDOStatement (part of PDO).

Matthew Flaschen
+1 -- I don't understand why so many find SQL injection complicated -- use of prepared statements stops this entire class of attacks cold (not that your app is secure just because it cannot be SQL injected).
Billy ONeal
alright thanks.
Lienau
@Billy not entirely. identifiers cannot be parametrized. It's worth to keep it in mind.
Col. Shrapnel
@Col. Shrapnel: If you're putting identifiers from POST or GET data, then there are bigger problems with your system design than SQL injection.
Billy ONeal
@Billy To clarify, I am not putting identifiers from POST or GET data or even from cookie. But what's wrong with design that uses dynamical sorting for example?
Col. Shrapnel
@Col.: Well then you're not getting the identifier name from user input, and therefore it cannot be SQL injected (Unless you do so on purpose). But really, if you have to dynamically generate identifier names, unless you're working on MDB2/PDO, that's signs of serious problems with your schema.
Billy ONeal
@Billy I am not dynamically generating identifier names. I merely generate a query, where order by field may vary based on user input. And you cannot use parametrizing to protect it. That's was my point.
Col. Shrapnel
A: 

It is also very important to understand that mysql_real_escape_string do not sanitize anything.
By applying this function you do not make any data safe. That's very widespread misunderstanding.
This function merely escaping string delimiters. So, it works only for strings, quote delimited ones. Thus, real sanitization could be only like this:

$_GET[$key] = "'".mysql_real_escape_string($_GET[$key])."'";

And even this one isn't suffice.

But as already Matt mentioned it would be very bad practice. And even more: as a matter of fact, not only input data should be properly formatted/paramertized. It's database function, not user input one! It has nothing to do with user input. Some data may come not from user input but from a file or other query or some service - it all should be properly formatted as well. That's very important to understand.

Also you are using an odd way to iterate arrays.
this one is more common:

foreach($_GET as $key => $value)
{
      $_GET[$key] = mysql_real_escape_string($value);
}
Col. Shrapnel
Well, it does a little more there -- it *does* do things like change NULL into \0, newlines into \n, etc. If it was just escaping quotes then there'd be no need for it because you could just use a string replace or addslashes. (But you should never use string replace or addslashes in real code)
Billy ONeal
@Billy actually to replace only quotes and a backslash itself is enough for most cases :) There was a guy who asked to break such escaping for utf8-encoded data, any many tried, but no luck. If you know a way it would be great. But it doesn't matter anyway. My point is different from that: escaping alone does not help anything
Col. Shrapnel