Avoiding SQL injection without parameters
We are having another discussion here at work about using parametrized sql queries in our code. We have two sides in the discussion: Me and some others that say we should always use parameters to safeguard against sql injections and the other guys that don't think it is necessary. Instead they want to replace single apostrophes with two apostrophes in all strings to avoid sql injections. Our databases are all running Sql Server 2005 or 2008 and our code base is running on .NET framework 2.0.
Let me give you a simple example in C#:
I want us to use this:
string sql = "SELECT * FROM Users WHERE Name=@name";
SqlCommand getUser = new SqlCommand(sql, connection);
getUser.Parameters.AddWithValue("@name", userName);
//... blabla - do something here, this is safe
While the other guys want to do this:
string sql = "SELECT * FROM Users WHERE Name=" + SafeDBString(name);
SqlCommand getUser = new SqlCommand(sql, connection);
//... blabla - are we safe now?
Where the SafeDBString function is defined as follows:
string SafeDBString(string inputValue)
{
return "'" + inputValue.Replace("'", "''") + "'";
}
Now, as long as we use SafeDBString on all string values in our queries we should be safe. Right?
There are two reasons to use the SafeDBString function. First, it is the way it has been done since the stone ages, and second, it is easier to debug the sql statements since you see the excact query that is run on the database.
So then. My question is whether it really is enough to use the SafeDBString function to avoid sql injection attacks. I have been trying to find examples of code that breaks this safety measure, but I can't find any examples of it.
Is there anybody out there that can break this? How would you do it?
EDIT: To summarize the replies so far:
- Nobody has found a way to get around the SafeDBString on Sql Server 2005 or 2008 yet. That is good, I think?
- Several replies pointed out that you get a performance gain when using parametrized queries. The reason is that the query plans can be reused.
- We also agree that using parametrized queries give more readable code that is easier to maintain
- Further it is easier to always use parameters than to use various versions of SafeDBString, string to number conversions and string to date conversions.
- Using parameters you get automatic type conversion, something that is especially useful when we are working with dates or decimal numbers.
- And finally: Don't try to do security yourself as JulianR wrote. The database vendors spend lots of time and money on security. There is no way we can do better and no reason we should try to do their job.
So while nobody was able to break the simple security of the SafeDBString function I got lots of other good arguments. Thanks!
I think the correct answer is:
Don't try to do security yourself. Use whatever trusted, industry standard library there is available for what you're trying to do, rather than trying to do it yourself. Whatever assumptions you make about security, might be incorrect. As secure as your own approach may look (and it looks shaky at best), there's a risk you're overlooking something and do you really want to take that chance when it comes to security?
Use parameters.
And then somebody goes and uses " instead of '. Parameters are, IMO, the only safe way to go.
It also avoids a lot of i18n issues with dates/numbers; what date is 01/02/03? How much is 123,456? Do your servers (app-server and db-server) agree with each-other?
If the risk factor isn't convincing to them, how about performance? The RDBMS can re-use the query plan if you use parameters, helping performance. It can't do this with just the string.
The argument is a no-win. If you do manage to find a vulnerability, your co-workers will just change the SafeDBString function to account for it and then ask you to prove that it's unsafe all over again.
Given that parametrized queries are an undisputed programming best practice, the burden of proof should be on them to state why they aren't using a method that is both safer and better performing.
If the issue is rewriting all the legacy code, the easy compromise would be to use parametrized queries in all new code, and refactor old code to use them when working on that code.
My guess is the actual issue is pride and stubbornness, and there's not much more you can do about that.
First of all, your sample for the "Replace" version is wrong. You need to put apostrophes around the text:
string sql = "SELECT * FROM Users WHERE Name='" + SafeDBString(name) & "'";
SqlCommand getUser = new SqlCommand(sql, connection);
So that's one other thing parameters do for you: you don't need to worry about whether or not a value needs to be enclosed in quotes. Of course, you could build that into the function, but then you need to add a lot of complexity to the function: how to know the difference between 'NULL' as null and 'NULL' as just a string, or between a number and a string that just happens to contain a lot of digits. It's just another source for bugs.
Another thing is performance: parameterized query plans are often cached better than concatenated plans, thus perhaps saving the server a step when running the query.
Additionally, escaping single quotes isn't good enough. Many DB products allow alternate methods for escaping characters that an attacker could take advantage of. In MySQL, for example, you can also escape a single quote with a backslash. And so the following "name" value would blow up MySQL with just the SafeDBString()
function, because when you double the single quote the first one is still escaped by the backslash, leaving the 2nd one "active":
x\' OR 1=1;--
Also, JulianR brings up a good point below: NEVER try to do security work yourself. It's so easy to get security programming wrong in subtle ways that appear to work, even with thorough testing. Then time passes and a year later your find out your system was cracked six months ago and you never even knew it until just then.
Always rely as much as possible on the security libraries provided for your platform. They will be written by people who do security code for a living, much better tested than what you can manage, and serviced by the vendor if a vulnerability is found.