CodeSOD: Anti-Injection

SQL injection attacks are, in most environments, easy to avoid. Pass user input through parameterized commands and never do any string munging to build your SQL queries. And yet, we constantly see places where people fail to do this correctly.

Eric's co-worker is one of "those" people. They were aware of what SQL injection was, and why it was a risk, but instead of using PHP's built-in functionality for avoiding it, they reinvented the wheel- now in a triangular shape!

// Anti-SQL Injection function check_inject() { $badchars = array(";","'","*","/"," \ ","DROP", "SELECT", "UPDATE", "DELETE", "drop", "select", "update", "delete", "WHERE", "where", "-1", "-2", "-3","-4", "-5", "-6", "-7", "-8", "-9",); foreach($_POST as $value) { $value = clean_variable($value); if(in_array($value, $badchars)) { //detect($value); die("SQL Injection Detected - Make sure only to use letters and numbers!\n<br />\nIP: ".getIpAddr()); } else { $check = preg_split("//", $value, -1, PREG_SPLIT_OFFSET_CAPTURE); foreach($check as $char) { if(in_array($char, $badchars)) { //detect($value); die("SQL Injection Detected - Make sure only to use letters and numbers!\n<br />\nIP: ".getIpAddr()); } } } } } function clean_variable($var){ $newvar = preg_replace('/[^a-zA-Z0-9\_\-]/', '', $var); return $newvar; }

There are so many layers of WTFery here. First, we stuff a $badchars array with some characters and more substrings, including " \ ", and "-9", which I'm not aware of any SQL injection attacks built out of negative numbers, yet here we are.

Then we inspect every $_POST variable, even the ones that aren't going anywhere near the database. We clean_variable, which strips most of the characters in $badchars in the first place, but then we check the variable to see if it's in the $badchars array. I want to stress, this is checking to see if the entire variable matches any string in the $badchars array. So it protects against a DROP but not a DROP TABLE users.

But that's okay, because they then split it into characters using an empty regex, and check each character against the array which isn't made up of single character strings.

So while clean_variable will do a decent enough job protecting against SQL injection attacks (by destroying your input data and preventing loads of realistic strings), the actual check_inject function is otherwise useless.

Forget injection, this code needs to be ejected into the cold hard vacuum of space, never to be heard from again.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!