Secure all post and get data in framework php [duplicate]

Question

This question already has answers here:

How can I prevent SQL injection in PHP? (27 answers)

Closed 9 years ago.

I just had this idea of "preSecure" all userinput data from $_post and $_get. But I was wondering if this is good practice and would like some input on this. Here is what I came up with:

function clean_str($str){
    return preg_replace('#[^a-z_ 0-9@.-]#i', '', $str);
}

if ($_POST){
    foreach ($_POST AS $key => $val){
        $_POST[$key] = clean_str($val);
    }
}

if ($_GET){
    foreach ($_GET AS $key => $val){
        $_GET[$key] = clean_str($val);
    }
}

This snippet would simply be run at the beginning of each http request. The clean_str function can be developed to allow other chars and replace characters etc (this is just an example). But I think the first goal are to simply prevent sql injection. The only bad thing I can see with this approach right now is if your "plugin" need to send sql commands from user input. The appraoch above could we wrapped in a function of course and be called if needed. Post and Get are global vars so that would not be a problem.

I'm actually writing my own framework (still a lot of work) that I will release if I ever be able to finish it. The thing is that I often see novice developers add $_POST['userinput'] inside database queries. The code above should make even that okay. Well that was some background of what I'm up to and why I bring this up.

I would be very happy to hear your thoughts. This might not be the best question for Stack Overflow, I suppose I want to open more like a discussion to this approach to share thoughts and inputs. But to formulate my question(s) to this, it would be something in line with: Are there any other approaches that would be faster or more scalable than this, or is equal to this, or can another function complement this approach? Is this approach good practice? Is it okay to overwrite the global vars of post and get data like above?

I know the code above is not object oriented, but it's about the approach of cleaning the users' input datas automatically before running checks on them. I think this will save a lot of code and headaches.

Please share your thoughts with me. As comments are limited here on Stack Overflow, I would appreciate if you reply as answers if you bring new thoughts to this table. Comments are to comment on specific thoughts/answers in this case.

Answer 1

First off if the user tries to inject an array then it will generate a notice, you should check for a string before calling it, else:

Notice: Array to string conversion 

But to be honest, you are just creating your own version of magic quotes, which isn't a good idea.

The simple and better solution would be to use prepared statements, with PDO or MySQLi.

A post on the PHP Website for magic quote sums it up nicely:

The very reason magic quotes are deprecated is that a one-size-fits-all approach to escaping/quoting is wrongheaded and downright dangerous. Different types of content have different special chars and different ways of escaping them, and what works in one tends to have side effects elsewhere. Any sample code, here or anywhere else, that pretends to work like magic quotes --or does a similar conversion for HTML, SQL, or anything else for that matter -- is similarly wrongheaded and similarly dangerous.

Magic quotes are not for security. They never have been. It's a convenience thing -- they exist so a PHP noob can fumble along and eventually write some mysql queries that kinda work, without having to learn about escaping/quoting data properly. They prevent a few accidental syntax errors, as is their job. But they won't stop a malicious and semi-knowledgeable attacker from trashing the PHP noob's database. And that poor noob may never even know how or why his database is now gone, because magic quotes (or his spiffy "i'm gonna escape everything" function) gave him a false sense of security. He never had to learn how to really handle untrusted input.

Data should be escaped where you need it escaped, and for the domain in which it will be used. (mysql_real_escape_string -- NOT addslashes! -- for MySQL (and that's only unless you have a clue and use prepared statements), htmlentities or htmlspecialchars for HTML, etc.) Anything else is doomed to failure.

Answer 2

This is not a good approach. Preventing MySQL injection isn't just a matter of making sure certain characters are escaped -- there are still plenty of attacks you can do even if you try to sanitize this way. It seems like you're overcomplicating things, when you can use prepared statements and no longer worry about all the things you need to check for. http://www.php.net/manual/en/mysqli.prepare.php