CodeSOD: A few updates

Brian he worked on concluding a contract with a European news agency. The aforementioned agency had a large number of intranet applications of varying complexity, all of which were built to support the news business.

Now they realized that, as a news agency, they had no real internal corporate knowledge of good software development practices, so they did what came naturally: They hired a self-proclaimed “code guru” to build the system.

The aforementioned code guru was notoriously explosive. When users approached him with complaints like “your system lost all the research I’ve been collecting for the past three months!” the guru would yell about how users were doing it wrong, how he couldn’t be trusted to handle the most basic tasks, and “wiping your ass is not part of my job description.”

With such a stellar personality, how was his PHP code?

$req000="SELECT idfiche FROM fiche WHERE idevent=".$_GET['id_evt'];
$rep000=$db4->query($req000);
$nb000=$rep000->numRows();
if ($nb000>0) 
	while ($row000=$rep000->fetchRow(DB_FETCHMODE_ASSOC)) 
		$req001="UPDATE fiche SET idevent=NULL WHERE idfiche=".$row000['idfiche'];
		$rep001=$db4->query($req001);

It is common for the first line of the submission to be bad. Rarely do the first 7 characters fill me with a sense of dread. $req000. Oh no. Oh nooo. We are talking about such variables.

We set the query using $req000 and store the result in $rep000using $db4 to run the query. It makes my skin crawl so much it feels like an obvious SQL injection vulnerability $_GET to write a query probably doesn’t get enough of my attention. I really hate these variable names although.

We run our gaping security vulnerability and check how many rows we have (using $nb000 to store results). While we have rows, we store each row $row000populate $req001– update request. We execute this query once for each row, storing the result in $rep001.

Now, the starter SELECT can return up to 4000 rows. It’s not a huge data set, but as you can imagine, this entire application was running on a server running potato that was in a network closet. It was slow.

The solution was obvious – you could replace both SELECT and UPDATEwith one query: UPDATE fiche SET idevent=NULL WHERE idevent=?– that’s all this code actually works.

Still, fixing performance wasn’t how Brian proved he was the right person for more contracts. After Brian saw the SQL injection, he showed his boss how a malicious user could easily delete an entire database from the URL bar of their browser. The boss was sufficiently terrified by the prospect – the code guru was politely asked to leave and Brian was told to fix it quickly.