Injections and reflections and web apps, oh my!
I love application security. It has always been a part of my work, whether as an explicit part of my job or as something my curious nature just couldn’t resist spilling over into.
I hate tickingclock.gif. A long time ago, there was a horrible animated GIF image of a ticking clock in the web interface of my employer’s main product. When you ran a report, you saw the ticking clock. There was no progress indicator, no cancel button, just the ticking clock.
In the 5.0 release, engineering added a Cancel button, and there was much rejoicing. The cancel button was just the submit button of an HTML form that held in a hidden field the PID of the running report process, which would be sent a kill signal on the server. Of course, since the PID was unchecked on the server side, anybody who had a little cunning and permission to run reports could kill any process running as NH_USER, including the server processes themselves. Oops. They fixed that issue a year or so later.
Fast-forward to the present. A good samaritan notified my new employer of an SQL injection vulnerability in the asset information part of the OpenNMS web app. We were building SQL queries directly out of data received from the web UI, which is high on the list of ways to get your application cracked. Remember how I said I can’t resist app security? I spent a few hours changing all the code in the web app AssetModel classes to use prepared statements and parameter binding, which gives you escaping of SQL special characters for free. It also gives a performance boost in some situations.
I knew at the time I was doing these fixes that it would be impossible to plug every hole. The web app is something we plan to redo from whole cloth in the next year or two, so this is work that will be thrown away. As if to remind me of these twin facts, I got a follow-up e-mail from our good samaritan just moments after checking in the SQL injection fixes. He had now found a reflected cross-site scripting (XSS) vulnerability. I was fried and my wife had come home, so I shelved the XSS problem until this morning.
Reflected XSS vulnerabilities are the less nasty kind (the nastier being the stored variety), and their potential for wreaking havoc is sometimes difficult to see at first. An attacker has to be somewhat crafty in order to get a victim to bite, but complacency and e-mails composed in 24-point purple MS Comic Sans font (with importance set to high, please) make the attacker’s job easier. Once the victim clicks on the attacker’s link (and logs in if not already authenticated) the fun can begin. It’s almost no work at all to craft a URL that sends the user’s session ID cookie to a drop-box where the attacker can retrieve it, allowing him to hijack the victim’s web app session. If the victim is assigned administrative privileges, it’s game over.
Fixing a reflected XSS vulnerability is harder than the technical part of exploiting one. When I started banging on this bug this morning, I did a proof-of-concept fix that I felt confident would work: I escaped the offending parameter string before putting it into the exception that our code throws when it’s unable to parse the string as an integer. Still, the alert pop-up that I had crafted into my test URL came right back. Why didn’t that work? I did a clean build and a fresh new install and tried again. Still no change. Another cup of coffee helped. The problem was that the Javascript encoded into one of the URL parameters was being reflected not once but twice. Even though I had escaped the dirty data before putting it into my exception’s message, the next exception up the stack contained the dirty data verbatim. Building a nice tailored exception class and a corresponding error-page fixed this particular XSS issue, and had the added benefit of making it look much nicer when a typo or an old link triggered the same exception-handling code that the XSS attack targeted.
Almost every web app vulnerability exploits the fact that some code uses data received from the browser without validating the data first. I went through the remainder of the web app code and replaced about a million Integer.parseInt method calls with an equivalent method that sanitizes the data first, removing anything that’s not a decimal digit. There were also a dozen or so Long.parseLong calls and a handful of Double.parseDouble calls, all of them now using safe equivalent methods. I also ran across one more SQL injection vulnerability that I had previously missed because the offending code was hidden in inner classes instead of being in plain sight in its own package.
There are really two lessons from all of this, I think. First, never trust data received from the user. That’s elementary, but too often overlooked by even good programmers who just want to finish a project. Second, never fall into the trap of thinking you’ve plugged all the holes. There are always more out there.
