Wednesday, 3 July 2013

SQL Injection with Entity Framework 5 and Static Code Analysis

It's interesting times here at the moment having started a new, big, project which is using MVC 4 Web API and Entity Framework 5 and some other cool stuff.  There is also a big push for secure coding at the moment so I'm evaluating various tools to help out with code and solution analysis for this.

Thinking about exploits


So the project is using Entity Framework 5, Code First.  It's a really nice technology even if it does have some limitations and quirks, but then that's the trade-off you make when you choose not to do everything manually.

Whilst using it, and looking at static code analysis tools I got to wondering how easy it would be to create a SQL injection exploit and, more importantly, how well do the code analysis tools pick it up.  The reason this kind of attack came to mind was because I was at the time reading the updated 2013 OWASP Top 10 and this is pretty close to the top (actually, I think it might be number 1).

Typically when using Entity Framework you would use LINQ to query your data source, this helps in that the framework will create parametrized queries, a great line of defence (maybe, more on this later).  But, what if you want to do something funky, well EF5 has you covered and allows you to run any SQL you want.

So I created a basic Web API project to retrieve data from a books database (imaginative!).  It included a couple of entity POCOs for the books and authors with some referential integrity, a DbContext class, and of course the API controller.  The method I implemented simply takes a string value and searches for all books with that text in the title.  What could possibly go wrong?  On to the code.


Exploiting the code


So if I run the code from Visual Studio 2012 and point my web browser to the resource method at "/api/books/?title=shadow" I get the single book in my limited database back.

Great, working so far.  So what else can we provide as an API request?  Well how about the following

/api/books/?title=' drop database test --'

Well, what does the response say?


That looks fine doesn't it?  Well yes, until I look in my database server and realise that my "test" database has suddenly vanished!

Looking at the exploit


As you can see, it's a fairly obvious exploit and one that should be picked up quite easily during a code review, although it frankly should never get to that point anyway and I would hope that most developers wouldn't think that this is acceptable.  But, with deadlines looming and managers breathing down your neck, even the best of us can make silly mistakes, so what can we do?

Well, one option is static code analysis.  This is where a tool looks at your code and determines if you are breaking any rules.  The most obvious choice here is the tool which comes with Visual Studio (in 2012 this is now available in the professional edition), also known as FxCop.  It comes with a built-in rule-set called "Microsoft Security Rules" and running it is as simple as going to the Code Analysis window, clicking the settings icon, setting the rule set and then running it.

Unfortunately it doesn't detect this exploit, so having code analysis running on your build environment and reporting issues isn't going to save you here.  So I tried the version of Code Analysis which comes with Visual Studio 2013 Preview Edition, still no luck.

Next up was CAT.NET, this is a tool which has a very limited set of rules and hasn't seen much attention from Microsoft since 2010, but it's still available so I tried it.  As expected it didn't find anything!

Last up was a commercial product which I'm now going to name here, but is in the NIST list.  It touts a large list of security rules and in demonstrations is very impressive, but here it failed to detect the issue.  It did detect that I should be using column names instead of "SELECT *", but that's not going to save the day really.

So lets make it parametrized


So will making it a parametrized query help?  Well lets see what it looks like in code (thanks to Troy Hunt for highlighting this in the first place).


This uses a stored procedure (even safer right?), the code for which looks like this.


So is this better?  Well, no.

The problem here is that I've shifted the injection vulnerability to the database.  But in a large development team where there are people looking after the database and others writing the C# code, in this kind of environment, without proper oversight of the full product you might find that you run into this problem.


So what do we do


Well, the only way to prevent this kind of simple mistake is education.  Make sure that developers don't think that using ORMs like Entity Framework protect them completely from exploits, and make sure that they're aware of coding defensively.

For now we're going to have to keep a closer eye on the code, but keep on pushing the vendors to support the latest technologies so that some of these checks can be automated, then we can turn our attention to less obvious problems.

Update

Thanks to Rowan Miller in the comments below for suggesting that I update the post to show how to use the above pattern safely with Entity Framework. When I wrote this post I wanted to highlight how easily you could misuse good framework, and how tools out of the box failed to detect it. The trouble is that it's easy to forget to show how to do something positively when you're looking at a negative.

So, with that in mind, here is an example using the above scenario, but this time using Database.SqlQuery<T> with a parametrized query and using a SqlParameter (although you can pass the value in directly in an object array if you don't want to create a parameter).


It's perhaps still not the best way of doing it, but if you have to write code this way then it's better that you do it right and reduce the risk of attack rather than bypassing all the secure features in place for you and opening up your application for exploits.