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

Question

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?

SQL Server will not allow a paramerized table name - you have to do it about how you show it. At my last gig we came up with a number possible solutions (because the first query we did was "what database and what table contains the master data for this customer"). One solution we came up with was to have a specific table replacement call, with the table checked to see that it was in a list of tables in the database (from sys.tables, cached). In the end, we decided to accept the risk and keep it simple — Flydog57
– Flydog57, Commented Nov 18, 2021 at 1:26
@Flydog57 That's where I'm leaning right now as well. Maybe some sanitization attempts before it gets concatenated into the query — Nigel
– Nigel, Commented Nov 18, 2021 at 1:28
If you are like us, tableName is a string that we obtain from a table where we store customer metadata. Because it was completely under our control and was untouched by user input or any other tainted data, the feeling what that there was no vulnerability and it wasn't worth adding complication. I still wanted a to use a separate token format and a separate method in our data layer to do the table name replacement (it just bothered me the way it was). I lost — Flydog57
– Flydog57, Commented Nov 18, 2021 at 1:32

Aaron Bertrand · Accepted Answer · 2021-12-28 02:42:23Z

6

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.

edited Dec 28, 2021 at 2:42

answered Nov 18, 2021 at 2:03

Aaron Bertrand

282k37 gold badges469 silver badges468 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

T.S. Over a year ago

I would also add to strip ; -- / * characters. Or kick out when these found. If they have SELECT TOP 0 * FROM {tableName}, it can be now SELECT TOP 0 * FROM sys.objects where 1=2; drop table Users

Aaron Bertrand Over a year ago

@T.S. QUOTENAME() takes care of that, even in the case where someone managed to first create a table called [sys.objects where 1=2; drop table Users], as it would first have to pass the check against sys.objects. You can strip characters if you like, but it's just extra work that doesn't offer any additional protection over (a) validating against the metadata and (b) using QUOTENAME().

Charlieface Over a year ago

@T.S. As you can see, it works fine in fiddle dbfiddle.uk/…

sticky bit · Accepted Answer · 2021-11-18 02:29:31Z

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

Nigel · Accepted Answer · 2021-11-18 18:34:43Z

0

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;
}

answered Nov 18, 2021 at 18:34

Nigel

3,3612 gold badges18 silver badges33 bronze badges

3 Comments

Aaron Bertrand Over a year ago

It's with several tables, not with "any database" - the issue I have with having that logic in the application code is that if I as a DBA want to make my checks more stringent I shouldn't have to modify code in a language I don't understand.

Nigel Over a year ago

The purpose of my class library is to work for an arbitrary database. It's a stand-alone library. Not only would a DBA have no business messing with it, but if the library were closed source they wouldn't even be able to. I didn't state that in the question because it isn't relevant to the problem

Aaron Bertrand Over a year ago

You can of course solve the problem the same way without a stored procedure. My point is that the people responsible for the database might have different goals (and different ideas about protection) than the author of the class library that accesses the database. :-)

Collectives™ on Stack Overflow

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

3 Answers 3

3 Comments

Comments

3 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related