is_array, mysql_real_escape_string

[Q695]

What is wrong with the injection preventer (array function) seen below:

//injection prevention
if (isset($_GET)){
    if (!is_array($_POST)){
        foreach( $_POST as $key => $value){
        $_POST["$key"]=mysql_real_escape_string($value) ;
        }



    } else {//here
        while (is_array($key)){
            foreach( $_POST as $key => $value){
            $_POST["$key"]=mysql_real_escape_string($value) ;
                echo $key;
            }
    }
}

[mac_gyver]

1) all of the super-global variables are set on every page request. they may be empty, but they are always set, so, isset($_GET) will always be true.

 

2) by definition all the super-global variables are arrays, so, is_array($_POST) will always be true.

 

3) you shouldn't escape the data and leave it in the $_POST array itself as that will alter it, so if you are echoing it back in the form fields it won't necessarily be the same as what was submitted. if you are going to escape all the $_POST data, you should escape a copy of the data and only use that escaped copy in sql query statements. i also hope you are properly validating numerical data since using a string escape function on them won't protect against sql injection.

 

4) you shouldn't be using the mysql_ functions any more since they are depreciated and will be removed in an upcoming php version. you should be using mysqli or PDO functions, where you can use prepared queries and avoid the need to do any escaping of data.

 

5) to do what you want, would require either using a function like array_walk_recursive() and let php do the work for you or you would loop over the $_POST array and detect if each element (the $value variable in your) is an array or not.

[Jacques1]

This is wrong on several levels.

 

First of all, it's conceptually wrong to apply one particular escaping function to all input. The PHP core developers tried a similar concept in the early days of the language (see Magic Quotes), and it ended in a disaster. As long as you indeed use the values in a query, it may seem convenient that they're already “pre-escaped”. But what if you want to use them in a different context like your HTML document? Then you need to revert the previous escaping, retrieve the original value and finally apply the correct escaping technique to this value. That's twice as much work. And should you ever forget to remove the SQL-escaping, you'll end up with the infamous backslash bug: All quotes and blackslashes will be preceded by an actual backslash character. This really doesn't look good on a website.

 

Another problem is that your pre-escaping will never cover all input. OK, so you've escaped the entries of $_POST. But what about $_GET? And $_COOKIE? And $_REQUEST? And $_FILES? And $_SERVER? And other values you read from external sources like your database or a file or another website? Those are all just as dangerous as POST parameters, but you keep them as they are. This is the perfect setting for a security vulnerability: If you become used to just dropping variables into query strings and then encounter a value which is not escaped yet, you're likely to forget the escaping. Now you have a full-blown SQL injection vulnerability.

 

Apart from the conceptual aspect, the code simply doesn't make sense. I can vaguely recognize the idea behind it, but almost every line is wrong in some sense. Why do you check if $_GET is set? This superglobal is always set. And what do the URL parameters have to do with POST parameters, anyway? What's the matter with the loops? When you've just found out that $_POST is not an array, why do you then try to iterate over its elements as if was an array? And so on.

 

Last but not least, the mysql_* functions are hopelessly outdated and will be removed in one of the next PHP releases. We have much better alternatives (PDO and MySQLi) since more than 10 years, and we've given up manual SQL-escaping in favor of prepared statements a long time ago. You're riding a dead horse.

 

Long story short: This is not a good idea. Use PDO instead of the old MySQL extension. Use prepared statements instead of manual SQL-escaping. And always escape data for its specific context. Blanket escaping doesn't work.

[Q695]

Working from this tutorial with PDO: http://www.phpeveryday.com/articles/PDO-Insert-and-Update-Statement-Use-Prepared-Statement-P552.html

 

Since you said to PDO it, what am I doing wrong:

// configuration
$dbtype     = "MySQL";
$dbhost     = "localhost";
$dbname     = "eagle";
$dbuser     = "root";
$dbpass     = "";


// database connection
$conn = new PDO("mysql:host=$dbhost;dbname=$dbname",$dbuser,$dbpass);


$title = 'PHP AJAX';

// query $sql = "INSERT INTO member_data WHERE fname = ?, mname = ?, lname = ?, street = ?, apt = ?, city = ?, state = ?, zip = ?, email = ?, number = ?, job = ?, awarded = ?"; $q = $conn->prepare($sql); $q->execute(array($title));

I created the database from the variable names.

Edited June 22, 2014 by Q695

[Q695]

lol, running a while on an insert