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."