Avoiding SQL injection without parameters

I think the correct answer is:

<b>Don't try to do security yourself</b>. 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.

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.

Here are a few reasons to use parameterized queries:

1. Security - The database access layer knows how to remove or escape items that are not allowed in data.
2. Separation of concerns - My code is not responsible for transforming the data into a format that the database likes.
3. No redundancy - I don't need to include an assembly or class in every project that does this database formatting/escaping; it's built in to the class library.

There were few vulnerability(I can't remember which database it was) that is related to buffer overflow of the SQL statement.

What I want to say is, SQL-Injection is more then just "escape the quote", and you have no idea what will come next.

It can be broken, however the means depends on exact versions/patches etc.

One that has already been brought up is the overflow/truncation bug that can be exploited.

Another future means would be finding bugs similar to other databases - for example the MySQL/PHP stack suffered an escaping problem because certain UTF8 sequences could be used to manipulate the replace function - the replace function would be tricked into *introducing* the injection characters.

At the end of the day, the replacement security mechanism relies on *expected* but not *intended* functionality. Since the functionality was not the intended purpose of the code, there is a high probablity that some discovered quirk will break your expected functionality.

If you have a lot of legacy code, the replace method could be used as a stopgap to avoid lengthy rewriting and testing. If you are writing new code, there is no excuse.

So I'd say:

1) Why are you trying to re-implement something that's built in? it's there, readily available, easy to use and already debugged on a global scale. If future bugs are found in it, they'll be fixed and available to everyone very quickly without you having to do anything.

2) What processes are in place to *guarantee* that you *never* miss a call to SafeDBString? Missing it in just 1 place could open up a whole host of issues. How much are you going to eyeball these things, and consider how much wasted that effort is when the accepted correct answer is so easy to reach.

3) How certain are you that you've covered off every attack vector that Microsoft(the author of the DB and the access library) knows about in your SafeDBString implementation ...

4) How easy is it to read the structure of the sql? The example uses + concatenation, parameters are very like string.Format, which is more readable.

Also, there are 2 ways of working out what was actually run - roll your own LogCommand function, a simple function with *no security concerns*, or even look at an sql trace to work out what the database thinks is really going on.

Our LogCommand function is simply:

string LogCommand(SqlCommand cmd)
{
StringBuilder sb = new StringBuilder();
sb.AppendLine(cmd.CommandText);
foreach (SqlParameter param in cmd.Parameters)
{
sb.Append(param.ToString());
sb.Append(" = \"");
sb.Append(param.Value.ToString());
sb.AppendLine("\"");
}
return sb.ToString();
}

Right or wrong, it gives us the information we need without security issues.

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.

Another important consideration is keeping track of escaped and unescaped data. There are tons and tons of applications, Web and otherwise, that don't seem to properly keep track of when data is raw-Unicode, &-encoded, formatted HTML, et cetera. It's obvious that it will become difficult to keep track of which strings are `''`–encoded and which aren't.

It's also a problem when you end up changing the type of some variable — perhaps it used to be an integer, but now it's a string. Now you have a problem.

For the reasons already given, parameters are a very good idea. But we hate using them because creating the param and assigning its name to a variable for later use in a query is a triple indirection head wreck.

The following class wraps the stringbuilder that you will commonly use for building SQL requests. It lets you **write paramaterized queries without ever having to create a parameter**, so you can concentrate on the SQL. Your code will look like this...

var bldr = new SqlBuilder( myCommand );
bldr.Append("SELECT * FROM CUSTOMERS WHERE ID = ").Value(myId, SqlDbType.Int);
//or
bldr.Append("SELECT * FROM CUSTOMERS WHERE NAME LIKE ").FuzzyValue(myName, SqlDbType.NVarChar);
myCommand.CommandText = bldr.ToString();
Code readability, I hope you agree, is greatly improved, and the output is a proper parameterized query.

The class looks like this...

using System;
using System.Collections.Generic;
using System.Text;
using System.Data;
using System.Data.SqlClient;

namespace myNamespace
{
/// <summary>
/// Pour le confort et le bonheur, cette classe remplace StringBuilder pour la construction
/// des requêtes SQL, avec l'avantage qu'elle gère la création des paramètres via la méthode
/// Value().
/// </summary>
public class SqlBuilder
{
private StringBuilder _rq;
private SqlCommand _cmd;
private int _seq;
public SqlBuilder(SqlCommand cmd)
{
_rq = new StringBuilder();
_cmd = cmd;
_seq = 0;
}
//Les autres surcharges de StringBuilder peuvent être implémenté ici de la même façon, au besoin.
public SqlBuilder Append(String str)
{
_rq.Append(str);
return this;
}
/// <summary>
/// Ajoute une valeur runtime à la requête, via un paramètre.
/// </summary>
/// <param name="value">La valeur à renseigner dans la requête</param>
/// <param name="type">Le DBType à utiliser pour la création du paramètre. Se référer au type de la colonne cible.</param>
public SqlBuilder Value(Object value, SqlDbType type)
{
//get param name
string paramName = "@SqlBuilderParam" + _seq++;
//append condition to query
_rq.Append(paramName);
_cmd.Parameters.Add(paramName, type).Value = value;
return this;
}
public SqlBuilder FuzzyValue(Object value, SqlDbType type)
{
//get param name
string paramName = "@SqlBuilderParam" + _seq++;
//append condition to query
_rq.Append("'%' + " + paramName + " + '%'");
_cmd.Parameters.Add(paramName, type).Value = value;
return this;
}

public override string ToString()
{
return _rq.ToString();
}
}
}

Always use parameterized queries where possible. Sometimes even a simple input without the use of any weird characters can already create an SQL-injection if its not identified as a input for a field in the database.

So just let the database do its work of identifying the input itself, not to mention it also saves allot of trouble when you need to actually insert weird characters that otherwise would be escaped or changed. It can even save some valuable runtime in the end for not having to calculate the input.