CodeSOD: Now I Need an Injection

Frankie was handed a pile of PHP and told, "Move this to a new host." The process didn't go well- simply copying the code to the server chucked out a 500 error. So Frankie started digging into the code.

Like a lot of PHP code, this code wasn't written. It happened. A long chain of revisions, emergency fixes, quick and dirty hacks, and "I dunno what I did, but that fixes it," meant that it was a twisty pile of spaghetti that wasn't drained properly and now is all sort of sticking together into a starch blob that only vaguely resembles the pasta it once was.

While trying to unpick the mess, Frankie spotted this:

public function executeQuery ($query) {$this->DbConnect();$this->preventSQLInjection($query);$result=mysql_query($query);$this->DbClose();return $result; }

The "important" line is preventSQLInjection($query). If you see that statement, you know something is horribly wrong. You're in that case of someone who knows enough to know that SQL injections are a threat, but only understands regexes as the solution.

private function preventSQLInjection($query){ $querySTR = strtoupper($query); $querySTR = str_replace("/**/", "", $querySTR); $querySTR = preg_replace("/\/\*(.+)\*\\//iSU", "", $querySTR); $querySTR = str_replace("/*", "", $querySTR); $querySTR = str_replace("*/", "", $querySTR); if (preg_match("/UNION(.*)SELECT/iSU", rawurldecode($querySTR))) die("erro"); return true; }

There isn't anything correct here. First, we make the string upper case. Of course, regexes can be case insensitive, so we don't need to do that. It's unnecessary, but I suppose not wrong.

Then, we use str_replace- a non-regex replace, to remove any empty comment blocks from the query. Then, we use a case insensitive regex to any "" followed by one or more of any character, followed by another "". Which" again, sounds like they're worried about comments, but this is a greedy operation, so SELECT /* some fields */ x, y, z FROM /* some table */ my_table would become SELECT / / my_table.

Then we strip any comment block indicators.

Finally, we check: if this query UNIONs another SELECT we just die, because we're good capitalists and we hate unions, apparently.

Now, yes, using comment blocks is a common way to break parsing so you can do your SQL injection. So this isn't completely misguided, but it's clearly wrong.

As Frankie sums up: "Just disregard all there is about the subject and implement your own solution. Beware of UNIONS and SELECTS, there may be demons. DROP it all for all I care."

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!