How can I parameterize an SQL table without vulnerability to SQL injection-CodePudding

I'm writing a C# class library in which one of the features is the ability to create an empty data table that matches the schema of any existing table.

For example, this:

private DataTable RetrieveEmptyDataTable(string tableName)
{
    var table = new DataTable() { TableName = tableName };

    using var command = new SqlCommand($"SELECT TOP 0 * FROM {tableName}", _connection);
    using SqlDataAdapter dataAdapter = new SqlDataAdapter(command);
    dataAdapter.Fill(table);

    return table;
}

The above code works, but it has a glaring security vulnerability: SQL injection.

My first instinct is to parameterize the query like so:

    using var command = new SqlCommand("SELECT TOP 0 * FROM @tableName", _connection);
    command.Parameters.AddWithValue("@tableName", tableName);

But this leads to the following exception:

Must declare the table variable "@tableName"

After a quick search on Stack Overflow I found this question, which recommends using my first approach (the one with sqli vulnerability). That doesn't help at all, so I kept searching and found this question, which says that the only secure solution would be to hard-code the possible tables. Again, this doesn't work for my class library which needs to work for arbitrary table names.

My question is this: how can I parameterize the table name without vulnerability to SQL injection?

CodePudding user response：

An arbitrary table name still has to exist, so you can check first that it does:

IF EXISTS (SELECT 1 FROM sys.objects WHERE name = @TableName)
BEGIN
  ... do your thing ...
END

And further, if the list of tables you want to allow the user to select from is known and finite, or matches a specific naming convention (like dbo.Sales%), or belongs to a specific schema (like Reporting), you could add additional predicates to check for those.

This requires you to pass the table name in as a proper parameter, not concatenate or token-replace. (And please don't use AddWithValue() for anything, ever.)

Once your check that the object is real and valid has passed, then you will still have to build your SQL query dynamically, because you still won't be able to parameterize the table name. You still should apply QUOTENAME(), though, as I explain in these posts:

So the final code would be something like:

CREATE PROCEDURE dbo.SelectFromAnywhere
  @TableName sysname 
AS
BEGIN
  IF EXISTS (SELECT 1 FROM sys.objects
      WHERE name = @TableName)
  BEGIN
    DECLARE @sql nvarchar(max) = N'SELECT * 
      FROM '   QUOTENAME(@TableName)   N';';
    EXEC sys.sp_executesql @sql;
  END
  ELSE
  BEGIN
    PRINT 'Nice try, robot.';
  END
END
GO

If you also want it to be in some defined list you can add

AND @TableName IN (N't1', N't2', …)

Or LIKE <some pattern> or join to sys.schemas or what have you.

Provided nobody has the rights to then modify the procedure to change the checks, there is no value you can pass to @TableName that will allow you to do anything malicious, other than maybe selecting from another table you didn’t expect because someone with too much access was able to create before calling the code. Replacing characters like -- or ; does not make this any safer.

CodePudding user response：

You could pass the table name to the SQL Server to apply quotename() on it to properly quote it and subsequently only use the quoted name.

Something along the lines of:

...

string quotedTableName = null;

using (SqlCommand command = new SqlCommand("SELECT quotename(@tablename);", connection))
{
    SqlParameter parameter = command.Parameters.Add("@tablename", System.Data.SqlDbType.NVarChar, 128 /* nvarchar(128) is (currently) equivalent to sysname which doesn't seem to exist in SqlDbType */);
    parameter.Value = tableName;
    object buff = command.ExecuteScalar();
    if (buff != DBNull.Value
        && buff != null /* theoretically not possible since a FROM-less SELECT always returns a row */)
    {
        quotedTableName = buff.ToString();
    }
}

if (quotedTableName != null)
{
    using (SqlCommand command = new SqlCommand($"SELECT TOP 0 FROM { quotedTableName };", connection))
    {
        ...
    }
}
...

(Or do the dynamic part on SQL Server directly, also using quotename(). But that seems overly and unnecessary tedious, especially if you will do more than one operation on the table in different places.)

CodePudding user response：

Aaron Bertrand's answer solved the problem, but a stored procedure is not useful for a class library that might interact with any database. Here is the way to write RetrieveEmptyDataTable (the method from my question) using his answer:

private DataTable RetrieveEmptyDataTable(string tableName)
{
    const string tableNameParameter = "@TableName";
    var query =
        "  IF EXISTS (SELECT 1 FROM sys.objects\n"  
        $"      WHERE name = {tableNameParameter})\n"  
        "  BEGIN\n"  
        "    DECLARE @sql nvarchar(max) = N'SELECT TOP 0 * \n"  
        $"      FROM '   QUOTENAME({tableNameParameter})   N';';\n"  
        "    EXEC sys.sp_executesql @sql;\n"  
        "END";


    using var command = new SqlCommand(query, _connection);
    command.Parameters.Add(tableNameParameter, SqlDbType.NVarChar).Value = tableName;
    using SqlDataAdapter dataAdapter = new SqlDataAdapter(command);
    var table = new DataTable() { TableName = tableName };
    Connect();
    dataAdapter.Fill(table);
    Disconnect();
    return table;
}