How safe is T-SQL after you replace the ' escape character?

Question

I am fully aware that the correct practice for sanitising SQL queries is to parameterise them.

I work on a lot of pre-existing code where the sanitisation measure was to replace all instaces of ' with '' in dynamic strings. I am trying to figure out how concerned I should be.

Here's the thing: this code runs exclusively on T-SQL (SQL Server 2008R2 and higher) and, as far as I can tell, ' is the only escape character for T-SQL.

So, how would you execute an injection attack to get past the above measure? I.e. is that very "naïve" sanitisation actually pretty solid on T-SQL, as it looks like?

Kronos

One can replace the single quote character with its hexadecimal representation, or its Unicode representation, or one can actually trick your "sanitation" by expecting that behavior and using the proper number of single quotes so that your "sanitation" totally fails.... seriously: don't do this. NEVER EVER - use parmaeters - ALWAYS – Lm

@Lm Please write an insert statement that inserts 1000 lines with 50 fields. If you can do that (hint: you can not) then I call that a good comment. Otherwise: ignorant. The number of parameters is limited and there ARE good cases for using another approach, at times. – Osmium

If you're doing that TomTom use SQLBulkCopy - marc_s is right, at least regarding using parameters – Handley

@TomTom: no sweat: INSERT INTO dbo.YourTable(Col1, Col2, .., Col50) VALUES(@Val1, @Val2, ...., @ValN); GO 1000 - done. – Lm

@Lm "or one can actually trick your "sanitation" by expecting that behavior using the proper number of single quotes so that your "sanitation" totally fails" Can you type half a quote? Because otherwise I don't see how you can pick a "proper number" N such that 2*N is odd. – Kronos

Answer 1

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

Feola

Thank you! Exactly the type of answer I was looking for. – Kronos

Answer 2

(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.

Contretemps

Answer 3

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

Petitioner