'I bet Russian hackers weren't expecting their target to suck so epically hard as this'

It's all fun and games until someone gets hurt


Line Break Welcome back to Line Break, our weekly column of terrible code our readers have encountered in the wild.

So far we've featured astonishingly brain-dead designs in production and amusing code from yesteryear machines. Our emphasis has been on learning through others' mistakes while also brightening drab Wednesday mornings with a chuckle and sage shake of the head.

It's all good fun until someone gets hacked.

SQL injections are still real in 2016

Reg reader Daniel is seething: the buggy code below was almost exploited by Russian hackers, we're told.

"This morning I encountered a classic SQL injection vulnerability which I recount here to demonstrate that really, really bad programmers still abound and are not restricted to ancient FORTRAN or COBOL," said Daniel, who described himself as livid.

The vulnerable code was imported from an external team, we're told: "The villain in this piece is a professional company, or I could say cowboy posse, that should know better and is big enough to afford better. If PHP has a bad reputation, it is because of imbeciles like these.

"Our site was attacked as a result of the following..."

$path = (string) getURLParam('path');
if($path) {
  $path[0] == "\/" ? $path = substr($path, 1, strlen($path)) : $path;

  $tableName = getTableName();
  $write = getDBConnection();

  $query = "select MAIN_TABLE.`product_id` from `{$tableName}` as MAIN_TABLE where MAIN_TABLE.`request_path` in('{$path}')";
  $readresult=$write->query($query);
  if ($row = $readresult->fetch() ) {
    $productId=$row['product_id'];
  }
}

This source really sucks. It lets an attacker inject arbitrary SQL commands into the code. We've changed some of the function names so no one innocent gets in trouble. All you need to know is that $path is set directly from the HTTP request sent to the web app, and thus under attacker control, ultimately potentially leading to SQL injection. The weird use of in(), though, thwarted initial exploitation of the SQL hole because it was unexpected by the hackers. It wouldn't have taken long for the miscreants to have worked around it.

"The main problem is $path goes unquoted and so any page request with a single quote may execute any SQL statement," explained Daniel.

"The incompetence doesn't stop there, I also see: the pointless use of a ternary operator when there is no 'else' action; raw SQL in a controller; the direct use of a database resource; not checking if product_id is valid – this could easily be added to the query's where clause if they feel PHP is too difficult; and the bizarre use of in() instead of =, although this strange practice ultimately saved our site because out of the hundreds of attacks we received, none had the crucial "');" needed to end the first statement.

"I bet the Russian hackers weren't expecting their target to suck so epically hard as this!"

Lesson: Prepared SQL statements – use them. Never pass raw user input direct to sensitive code without sanitizing it. Just don't write PHP like the above. It's 2016. Daniel, you have our sympathies.

Whisky Tango Foxtrot

The above code is dangerous to system security. The following is dangerous to mental security. Rafa writes in with some more programming misery.

"I used to work with someone who wrote this," we're told.

    /**
     *
     * @param alpha first item
     * @param omega last item
     * @return true if the last item is null
     */
    public static boolean omegaIsNull(Object alpha, Object omega) {
        boolean eval = false;
        if (alpha == null) {

            eval = false;
        }

        if (alpha != null && omega == null) {

            eval = true;
        }

        if ((alpha != null & !alpha.equals("") & !alpha.equals(" ")) && (omega == null || omega.equals("") || omega.equals(" "))) {

            eval = true;
        }

        return eval;
    }

Argh. ARRGHHH! Why not just simply check for omega == null?

"Now," Rafa warns us, "take a look at the sloppy use of operators: logical (&, |) and conditional (&&, ||) are used apparently at random with disastrous results – NullPointerException for example.

"I simply cannot explain my astonishment. Why all this 27 lines, when all you need is 11 characters? Needless to say, this method does not work at all.

"Lessons learned? KISS. Reduce the code to a bare minimum. Select big blocks and press the delete key with joy. It can only work better."

Lesson: It would easy to say something like "fire them". But that doesn't help at all. Learn that more code does not equal better code. The best code gets the job done and only the job required done, no ifs and no buts. Well, maybe some if()s.

Unsign me up

Let's finish on a happy note rather than an angry one. Paul, a software engineer for 20 years, has sent us in a hat trick of cockups:

"I once worked with an engineer who had been asked to investigate why serial communications with an embedded device no longer worked in a program someone wrote years before. On investigation, it turned out that an essential delay was being done by means of a for loop (eg: for(int i=1; i <10000; i++) {}).

"As soon as computers got a little bit quicker, the delay became shorter and communications was no longer possible. You can probably guess what he did to fix it."

Lesson: We've all made a bodged-in timing loop to get characters out to a serial port or similar in our time. It worked, we saw some text, we knew the board was working. Crucially, you then make the timing code a little more sophisticated. It only takes a few extra minutes. Skip the second pint at lunch and implement it, ta.

Next.

"About 15 years ago, one of our engineers was asked to rewrite an old Delphi application in C++ as the company had decided to standardized on C based languages," Paul continued.

"At the time, this application used a proprietary binary file format, with many settings being bit encoded. To encode these bits, the engineer decided to bitwise OR the destination byte with 2 raised to a particular power representing the bit position.

"Bad approach, I know, but it was made worse by the fact that the engineer had a Visual Basic background. In Visual Basic, raising something to power of another value can be expressed as ^, eg: x = x || (2^3).

"In C based languages, ^ means bitwise XOR. This resulted in numerous customer files being saved incorrectly and requiring significant manual effort on our part to restore them. Not a good time."

Lesson: Different languages do bitwise operations differently. Also, there's got to be a better way to store configuration data than using raw binary. JSON, YAML, or (dare I say it) XML are pretty cool instead.

Next.

"Many engineers I know, me included, have fallen foul of counting backwards down to zero using an unsigned variable instead of a signed one," says Paul. "This results in a loop from which there is no escape, for example..."

for(uint i = 100; i >= 0; i--)
{
  //in here forever
}

Lesson: Unsigned integers do not cross zero to -1, they'll typically roll around to their maximum value – a very positive number. Either stop at zero or use a signed integer instead.

Phew, OK. That's enough pain. We'll see you next week for some more therapy. Send in your code submissions, please, we love to hear from you. ®

Click here to see all Line Break columns


Other stories you might like

Biting the hand that feeds IT © 1998–2021