CodeSOD: A Few Updates

Brian was working on landing a contract with a European news agency. Said agency had a large number of intranet applications of varying complexity, all built to support the news business.

Now, they understood 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 built the system.

Said code guru was notoriously explosive. When users came to him with complaints like "your system lost all the research I've been gathering for the past three months!" the guru would shout about how users were doing it wrong, couldn't be trusted to handle the most basic tasks, and "wiping your ass isn't part of my job description."

With a stellar personality like that, what was his PHP code like?

$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's common that the first line of a submission is bad. It's rare that the first 7 characters fill me with a sense of dread. $req000. Oh no. Oh noooo. We're talking about those kinds of variables.

We query using $req000 and store the result in $rep000, using $db4 to run the query. My skin is crawling so much from that that I feel like the obvious SQL injection vulnerability using $_GET to write the query is probably not getting enough of my attention. I really hate these variable names though.

We execute our gaping security vulnerability, and check how many rows we got (using $nb000 to store the result). While we have rows, we store each row in $row000, to populate $req001- an update query. We execute this query once for each row, storing the result in $rep001.

Now, the initial SELECT could return up to 4,000 rows. That's not a massive dataset, but as you can imagine, this whole application was running on a potato-powered server stuffed in the network closet. It was slow.

The fix was obvious- you could replace both the SELECT and the UPDATEs with a single query: UPDATE fiche SET idevent=NULL WHERE idevent=?- that's all this code actually does.

Fixing performance wasn't how Brian proved he was the right person for more contract work, though. Once Brian saw the SQL injection, he demonstrated to the boss how a malicious user could easily delete the entire database from the URL bar in their browser. The boss was sufficiently terrified by the prospect- the code guru was politely asked to leave, and Brian was told to please fix this quickly.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!