DevDisasters

Bad Code in Any Language

Code audits seemed unnecessary to everyone except the auditor. Good thing the auditor finally had one sympathetic ear -- "Andrew" -- to hear him out.

• By Mark Bowytz
• 05/05/2015

Andrew flipped open and laid out the three-ring binder on his desk. Looking like an impossibly thick butterfly with vibrantly colored manila dividers, its contents flopped open with a dull thud.

In the meeting where the binders were passed out, everybody's reaction ranged from somewhere between moral outrage and disgust -- for years, upper IT management was presented with auditors' findings, with warnings that upper IT management summarily ignored.

See, historically when the auditors came around, rumor was that you could get into some serious trouble if you neglected to include the appropriate sign-offs and process controls for each and every change. But the reality was, the worst that happened was a "talking to" for not being as diligent in document retention (even though everybody followed the process, of course) and an organizational "goal" would be added for the year-end performance review.

As a result, much like those in management, everybody in the development group treated the whole auditing process as, well, kind of a joke. However, the things that didn't reach the developers' plates were certainly not a joke. The auditing group ran a series of tools, occasionally employing outside specialists to provide additional analysis, all looking for code issues or application issues that needed to be addressed.

When presented with the findings, the uppers would evaluate the findings, promise to fix them in the coming months, and every year, they signed a waiver acknowledgment stating that they accepted the risk. They'd insist, "You can't just demand a bunch of changes be made ahead of real work, like bug fixes and system enhancements," and, "Most of these systems aren't even fully live yet -- we're actively working on the system."

To them, these findings weren't just a low priority, they were a non-priority. Well, everything changed after "The Crash."

Uh Oh!
It all started with the breach of one system that led to multiple breaches and, finally, everything grinding to a halt as the person or group responsible went about cleaning up after themselves by wiping out everything.

Andrew's group felt that they were always very good about staying on top of things and no way could their code provide such an easy means of attack. Yet here he was facing a two-inch-thick binder containing his part of the toxic spill cleanup and an understanding that if anybody had aspirations for upper management, there were a few new openings.

Pinned into a Corner
The tricky part was that he only had two weeks to turn around the changes before the next auditing inquisition ... err ... auditor quality meeting.

Of course, this meant that after two weeks, he'd have to pull double-duty to keep the rest of the projects he was on the line for afloat, along with keeping up with any of the auditing findings that were in-progress.

Joy.

Figuring that one issue was the same as any other, he picked one of the mid-section tabs and flopped the page over to that section: "User Login PIN Stored as Plain Text in Database."

"You've got to be kidding me." he remarked to nobody in particular as he rubbed at his temples. Here it was, the 21st century -- at a multi-billion dollar company, mind you -- and some developer wasn't thinking things through when he came up with this thought: Passwords are just too complicated; bank cards are secure -- let's try PIN numbers!

Hoping that somehow, corrective measures had already been made or that maybe this was all some elaborate joke, he navigated over to the test portal and created a new account for "Mr. Nosmo King" at "123 Main Street, Anytown, XX 12345." When prompted to enter a "secure PIN," he plugged in 9999 and checked the back-end via a quick SQL query:

SELECT pin_no FROM dbo.tblWebUsers WHERE user_name = 'nking'

Unfortunately, the fix didn't make it into the code, but much to his surprise, Andrew found that the value stored was ... 228?!

Andrew bounced back to the app and tried a few other PINs and even strings. Heck, it asked for a number but if you wanted to use the "number" A the Web form validation wouldn't stop you --and the results were pretty close to each other. The result of every entry he tried was a three- or four-digit number on the back-end.

There was no way that app stored an unprotected PIN. It looked like it was some kind of simple hash. Curious, Andrew dug in and found something. But it was written in German:

Private Function zCodeName(ByVal strName As String) As Double
  Try
    zCodeName = 0.0

    Dim strAktBuchstabe As String
    Dim strAktBuchstabeAscii As String
    Dim dblAktZahlAscii As Double

    For i As Integer = 0 To strName.Length - 1

      strAktBuchstabe = strName.Substring(i, 1)

      For j As Integer = 0 To 255

        strAktBuchstabeAscii = Chr(j)

        If strAktBuchstabe = strAktBuchstabeAscii Then
          dblAktZahlAscii = j
          Exit For
        End If

      Next

      zCodeName = zCodeName + dblAktZahlAscii

    Next
  Catch ex As Exception

  End Try

End Function

Why would the developer introduce a function written in German when the company, users and system were all located in the United States? Was the developer hoping to obfuscate the purpose of the code? And what if the developer was from Germany?

Andrew couldn't say, nor did it matter to him for that matter. What mattered right now was that Andrew hoped the author of the code was consuming some bad seafood and duplicating the feeling that he was currently experiencing.

Hoping to get a handle on what the code hoped to accomplish, Andrew learned that "AktBuchstabe" translated into "Raw Characters" and "AktZahl" meant "Reference number," which made reading though the code easier to read, but during his testing, Andrew found the code's true strange nature to be revealed.

Unarguably, the code takes the long way to get the Integer-value of a character: using a Loop over all Integers (0 - 255) and comparing Chr(j) to the current character. But that's only half of the story.

You see, this particular method of generating a hash value has another curious side effect. As long as the value provided has the same digits (or letters) in any order, it would validate against the stored value means that the PIN "1234" has the same return value as "4321," "2143," "3412" and so on. And thanks to how ASCII code works, the value "24d" validates, as well.

After seeking the appropriate approvals, and working with a customer communications coordinator, Andrew updated the users' records to mass-expire at a certain date, forcing them to enter new credentials. The difference was that the PIN_NO field was updated from an integer to a varchar(32) field to accommodate an MD5 hash.

It was just one issue down with many more to go.