views:

291

answers:

2

Is there anything else that the code must do to sanitize identifiers (table, view, column) other than to wrap them in double quotation marks and "double up" double quotation marks present in the identifier name? References would be appreciated.

I have inherited a code base that has a custom object-relational mapping (ORM) system. SQL cannot be written in the application but the ORM must still eventually generate the SQL to send to the SQL Server. All identifiers are quoted with double quotation marks.

string QuoteName(string identifier) 
{ 
    return "\" + identifier.Replace("\"", "\"\"") + "\"";
}

If I were building this dynamic SQL in SQL, I would use the built-in SQL Server QUOTENAME function:

declare @identifier nvarchar(128);
set @identifier = N'Client"; DROP TABLE [dbo].Client; --';

declare @delimitedIdentifier nvarchar(258);
set @delimitedIdentifier = QUOTENAME(@identifier, '"');

print @delimitedIdentifier;
-- "Client""; DROP TABLE [dbo].Client; --"

I have not found any definitive documentation about how to escape quoted identifiers in SQL Server. I have found Delimited Identifiers (Database Engine) and I also saw this stackoverflow question about sanitizing.

If it were to have to call the QUOTENAME function just to quote the identifiers that is a lot of traffic to SQL Server that should not be needed.

The ORM seems to be pretty well thought out with regards to SQL Injection. It is in C# and predates the nHibernate port and Entity Framework etc. All user input is sent using ADO.NET SqlParameter objects, it is just the identifier names that I am concerned about in this question. This needs to work on SQL Server 2005 and 2008.


Update on 2010-03-31

While the application is not supposed to allow user-input for identifier names in queries, the ORM does via the query syntax that it has for both ORM-style reads and custom queries. It is the ORM that I am trying to ultimately prevent all possible SQL Injection attacks as that is very small and easy to verify as opposed to all the application code.

A simple example of the query interface:

session.Query(new TableReference("Client")
    .Restrict(new FieldReference("city") == "Springfield")
    .DropAllBut(new FieldReference("first_name"));

ADO.NET sends over this query:

exec sp_executesql N'SELECT "T1"."first_name" 
FROM "dbo"."Client" AS "T1" 
WHERE "T1"."city" = @p1;', 
N'@p1 nvarchar(30)', 
N'Springfield';

Perhaps it would help to think about how something similar this might look in nHibernate Query Language (HQL):

using (ISession session = NHibernateHelper.OpenSession())
{
    Client client = session
        .CreateCriteria(typeof(Client))  \\ <-- TableReference in example above
        .Add(Restrictions.Eq("city", "Springfield"))  \\ <-- FieldReference above
        .UniqueResult<Client>();
    return client;
}

Maybe I should look and see how nHibernate protects the input.

A: 

Can you not just use [ and ] delimiters instead of quotes (single or double)?

Identifiers should never really contain any quotes (unless you're more unlucky than now) so you remove the normal use factor of quotes in names etc

Edit:

But if the calls to the ORM are already parameterised, you don't need to worry about it, no? Using [ and ] removes the need for complex escaping in c# strings

gbn
Those have to be escaped as well."Client]; DROP TABLE dbo.Client; --" would have to be escaped as [Client]]; DROP TABLE dbo.Client; --]The actual object names in the database don't ever contain anything strange, its just making the ORM protected against sending malicious queries.
Ross Bradbury
If all they can pass is an identifier name, then before you execute the string, can't you check if the identifier is, in fact, an object? Your injection string obviously won't pass that sniff test.
Aaron Bertrand
Thanks Aaron, that is not a bad idea (and in fact is done for some cases already). However, I don't want to have to query the database just to see if the object exists all the time. I can keep a cache of the object names in memory for most of them, but thats kind of large (about 12,000 columns, the names take up 500KB of RAM - not a huge deal but still more than I'd like). I do appreciate the suggestion and I am open to alternatives, but realize I have an existing code base to work with and I am looking for an answer to the question I asked.
Ross Bradbury
+1  A: 

Your QuoteName function needs to check the length, because the T-SQL QUOTENAME function specifies the maximum length it returns. Using your example:

String.Format(@"declare @delimitedIdentifier nvarchar(258);
set @delimitedIdentifier = {0};", QuoteName(identifier));

If QuoteName(identifier) is longer than 258 characters, it will be silently truncated when assigned to @delimitedIdentifier. When that happens, you open up the possibility for @delimitedIdentifier to be escaped improperly.

There is an MSDN article by Bala Neerumalla, a "security software developer at Microsoft", that explains the topic in more depth. The article also contains the closest thing I have found to "definitive documentation about how to escape quoted identifiers in SQL Server":

The escaping mechanism is simply doubling up the occurrences of right square brackets. You don't need to do anything with other characters, including left square brackets.

This is the C# code I am currently using:

/// <summary>
/// Returns a string with the delimiters added to make the input string
/// a valid SQL Server delimited identifier. Brackets are used as the
/// delimiter. Unlike the T-SQL version, an ArgumentException is thrown
/// instead of returning a null for invalid arguments.
/// </summary>
/// <param name="name">sysname, limited to 128 characters.</param>
/// <returns>An escaped identifier, no longer than 258 characters.</returns>
public static string QuoteName(string name) { return QuoteName(name, '['); }

/// <summary>
/// Returns a string with the delimiters added to make the input string
/// a valid SQL Server delimited identifier. Unlike the T-SQL version,
/// an ArgumentException is thrown instead of returning a null for
/// invalid arguments.
/// </summary>
/// <param name="name">sysname, limited to 128 characters.</param>
/// <param name="quoteCharacter">Can be a single quotation mark ( ' ), a
/// left or right bracket ( [] ), or a double quotation mark ( " ).</param>
/// <returns>An escaped identifier, no longer than 258 characters.</returns>
public static string QuoteName(string name, char quoteCharacter) {
    name = name ?? String.Empty;
    const int sysnameLength = 128;
    if (name.Length > sysnameLength) {
        throw new ArgumentException(String.Format(
            "name is longer than {0} characters", sysnameLength));
    }
    switch (quoteCharacter) {
        case '\'':
            return String.Format("'{0}'", name.Replace("'", "''"));
        case '"':
            return String.Format("\"{0}\"", name.Replace("\"", "\"\""));
        case '[':
        case ']':
            return String.Format("[{0}]", name.Replace("]", "]]"));
        default:
            throw new ArgumentException(
                "quoteCharacter must be one of: ', \", [, or ]");
    }
}
Michael Kropat
I think you mean I should require the length of the input to my function is <= 128 characters. That makes sense.The T-SQL QUOTENAME function's parameter is nvarchar(128) and returns nvarchar(258). I can't see how it could ever return more than 258 characters from a 128 character input, because it seems that at most each character would be doubled (256), plus a beginning and ending delimiter. Could you give an example of when QUOTENAME would truncate?select len(quotename(replicate(']', 128))), len(quotename(replicate(']', 129)))returns: 258, null
Ross Bradbury
Exactly, your function must validate that the input is <= 128 characters. The T-SQL QUOTENAME function itself never truncates--the truncation you have to watch out for is assigning the return value of your C# QuoteName function to an nvarchar(258) variable in (dynamic) SQL. As long as your C# QuoteName function requires that the input is <= 128 characters, i.e. it behaves likes the T-SQL QUOTENAME function, you have nothing to worry about.
Michael Kropat