How safe is T-SQL after you replace the ' escape character?
(Insert remarks about dangers of broken sanitization and escaping here. See comment of marc_s.)
What you propose here is the same method that the Microsoft SQL Server Managed Objects (SMO) use. Those are .NET DLLs that one can inspect using a decompiler. For example:
internal static string MakeSqlString(string value)
{
StringBuilder builder = new StringBuilder();
builder.Append("N'");
builder.Append(EscapeString(value, '\''));
builder.Append("'");
return builder.ToString();
}
public static string EscapeString(string value, char escapeCharacter)
{
StringBuilder builder = new StringBuilder();
foreach (char ch in value)
{
builder.Append(ch);
if (escapeCharacter == ch)
{
builder.Append(ch);
}
}
return builder.ToString();
}
So yes, simply doing a Replace("'", "''")
is enough according to Microsoft. I'm sure this is not just intern code but has been audited for security. They always do this due to the SDL.
Note also that this code seems to be made to work with Unicode (see the N
prefix). Apparently, this is Unicode-safe as well.
On a more subjective note: I do escape T-SQL string literals just like this if I have to. I trust this method.
Yes, a single-quote is the only escape character so you are mostly, but not entirely ok.
Using parameters, while best, is mostly just doing the '
to ''
replacement that you are doing manually. BUT, they also enforce a maximum string length. Of course, if we were talking about non-string parameters, they would have the benefit of enforcing the type of the data (i.e. a '
does not need to be escaped for numeric, date/time, etc types as it is not valid for them to begin with).
The issue you might still be left with is a subset of SQL Injection called SQL Truncation. The idea is to force some part of the dynamic sql off the end of the string. I am not sure how likely this is to happen in practice, but, depending on how and where you are constructing the dynamic sql, you need to make sure that the variable holding the dynamic SQL to execute is large enough to hold the static pieces in your code plus all of the variables assuming they are submitted at their maximum lengths.
Here is an article from MSDN Magazine, New SQL Truncation Attacks And How To Avoid Them, that shows both regular SQL Injection as well as SQL Truncation. You will see in the article that to avoid SQL Injection they mostly just do the REPLACE(@variable, '''', '''''')
method, but also show using QUOTENAME(@variable, '[')
for some situations.
EDIT (2015-01-20): Here is a good resource, though not specific to SQL Server, that details various types of SQL Injection: https://www.owasp.org/index.php/Testing_for_SQL_Injection_(OTG-INPVAL-005)
The following article is related to the one above. This one is specific to SQL Server, but more general in terms of overall security. There are sections related to SQL Injection:
https://www.owasp.org/index.php/Testing_for_SQL_Server
This is in .NET
I now notice the question is tagged TSQL but not .NET
Marc implied you could fool it with the unicode or hex representation. I tested and neither unicode nor hex fooled it in .NET.
As for fooling it with a proper number of apostrophe. If you are always replacing one with two I don't see how a proper number could fool it.
u0027 and x0027 is apostrophe and it was replaced with two apostrophe
2018 and 2019 are left and right quotes and SQL just treated them as literals
string badString = "sql inject \' bad stuff here hex \x0027 other bad stuff unicode \u0027 other bad stuff left \u2018 other bad stuff right \u2019 other bad stuff";
System.Diagnostics.Debug.WriteLine(badString);
System.Diagnostics.Debug.WriteLine(SQLclean(badString));
}
public string SQLclean(string s)
{
string cleanString = s.Replace("\'", "\'\'");
return "N\'" + cleanString + "\'";
}
I think parameters is a better practice