CodeSOD: $art000

Many years ago, Brian was on the lookout for a new contract. While hunting, he found an opportunity to maintain some Intranet software for a large European news agency. He contacted them, and had a series of conversations with one of the managers.

"You see," the manager explained, "Sven was the code guru behind this entire system. He was... a colorful personality, but extremely smart. We don't have a lot of confidence that we could just slot a contractor into his role and expect them to be able to deliver what Sven could deliver for us."

To give them that confidence, Brian agreed to do some very small contract work before they moved on to a larger, longer-term contract. This let him see exactly what Sven could deliver.

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

The first thing Brian pointed out to his prospective employer that, as written, this was vulnerable to SQL injection attacks right from the address bar. That alone gave the manager a fright and may have been what clinched a large contract for Brian to continue cleaning up this code.

But a SQL injection vulnerability isn't a WTF. So it's helpful that everything else about this is awful.

The goal of this block of code is to find all of the forms where idevent has a certain value and null that field out. To do this, we do a select from form to find all the idforms where that's the case, then we can iterate across that list to UPDATE the form table.

Which, of course, could all be done in a single update: UPDATE form SET idevent NULL WHERE idevent=".$_GET["id_evt"]. I've left the SQL injection vulnerability in there, just to keep at least some of the WTF in place.

But it's the variable names that are the fascinating part. Sort of a monstrous version of hungarian notation where the prefix gives you a vague sense of the role of the variable: nb for "number", req for a database "request", rep for a... reply? And then the unique name of the variable is... its numeric component. That we see $db4 in here implies some other horrors lurking just outside of this block.

Brian writes:

"Code gurus" like this keep me employed so I should be thankful. It made me laugh, then cry, and then laugh again.

[Advertisement] ProGet's got you covered with security and access controls on your NuGet feeds. Learn more.