Is it safe to not parameterize an SQL query when the parameter is not a string?

Solution 1:

I think it's safe... technically, but it's a terrible habit to get into. Do you really want to be writing queries like this?

var sqlCommand = new SqlCommand("SELECT * FROM People WHERE IsAlive = " + isAlive + 
" AND FirstName = @firstName");

sqlCommand.Parameters.AddWithValue("firstName", "Rob");

It also leaves you vulnerable in the situation where a type changes from an integer to a string (Think employee number which, despite its name - may contain letters).

So, we've changed the type of EmployeeNumber from int to string, but forgot to update our sql queries. Oops.

Solution 2:

When using a strongly-typed platform on a computer you control (like a web server), you can prevent code injection for queries with only bool, DateTime, or int (and other numeric) values. What is a concern are performance issues caused by forcing sql server to re-compile every query, and by preventing it from getting good statistics on what queries are run with what frequency (which hurts cache management).

But that "on a computer you control" part is important, because otherwise a user can change the behavior used by the system for generating strings from those values to include arbitrary text.

I also like to think long-term. What happens when today's old-and-busted strongly-typed code base gets ported via automatic translation to the new-hotness dynamic language, and you suddenly lose the type checking, but don't have all the right unit tests yet for the dynamic code?

Really, there's no good reason not to use query parameters for these values. It's the right way to go about this. Go ahead and hard-code values into the sql string when they really are constants, but otherwise, why not just use a parameter? It's not like it's hard.

Ultimately, I wouldn't call this a bug, per se, but I would call it a smell: something that falls just short of a bug by itself, but is a strong indication that bugs are nearby, or will be eventually. Good code avoids leaving smells, and any good static analysis tool will flag this.

I'll add that this is not, unfortunately, the kind of argument you can win straight up. It sounds like a situation where being "right" is no longer enough, and stepping on your co-workers toes to fix this issue on your own isn't likely to promote good team dynamics; it could ultimately hurt more than it helps. A better approach in this case may be to promote the use of a static analysis tool. That would give legitimacy and credibility to efforts aimed and going back and fixing existing code.

Solution 3:

In some cases, it IS possible to perform SQL injection attack with non-parametrized (concatenated) variables other than string values - see this article by Jon: http://codeblog.jonskeet.uk/2014/08/08/the-bobbytables-culture/ .

Thing is that when ToString is called, some custom culture provider can transform a non-string parameter into its string representation which injects some SQL into the query.

Solution 4:

This is not safe even for non-string types. Always use parameters. Period.

Consider following code example:

var utcNow = DateTime.UtcNow;
var sqlCommand = new SqlCommand("SELECT * FROM People WHERE created_on <= '" + utcNow + "'");

At the first glance code looks safe, but everything changes if you make some changes in Windows Regional Settings and add injection in short date format:

Now resulting command text looks like this:

SELECT * FROM People WHERE created_on <= '26.09.2015' OR '1'<>' 21:21:43'

The same can be done for int type as user can define custom negative sign which can be easily changed into SQL injection.

One could argue that invariant culture should be used instead of current culture, but I have seen string concatenations like this so many times and it is quite easy to miss when concatenating strings with objects using +.