How should I pass a table name into a stored proc?

Question

I just ran into a strange thing...there is some code on our site that is taking a giant SQL statement, modifying it in code by doing some search and replace based on some user values, and then passing it on to SQL Server as a query.

I was thinking that this would be cleaner as a parameterized query to a stored proc, with the user values as the parameters, but when I looked more closely I see why they might be doing it...the table that they are selecting from is variably dependant on those user values.

For instance, in one case if the values were ("FOO", "BAR") the query would end up being something like "SELECT * FROM FOO_BAR"

Is there an easy and clear way to do this? Everything I'm trying seems inelegant.

EDIT: I could, of course, dynamically generate the sql in the stored proc, and exec that (bleh), but at that point I'm wondering if I've gained anything.

EDIT2: Refactoring the table names in some intelligent way, say having them all in one table with the different names as a new column would be a nice way to solve all of this, which several people have pointed out directly, or alluded to. Sadly, it is not an option in this case.

Solution 1

First of all, you should NEVER do SQL command compositions on a client app like this, that's what SQL Injection is. (Its OK for an admin tool that has no privs of its own, but not for a shared use application).

Secondly, yes, a parametrized call to a Stored procedure is both cleaner and safer.

However, as you will need to use Dynamic SQL to do this, you still do not want to include the passed string in the text of the executed query. Instead, you want to used the passed string to look up the names of the actual tables that the user should be allowed to query in the way.

Here's a simple naive example:

CREATE PROC spCountAnyTableRows( @PassedTableName as NVarchar(255) ) AS
-- Counts the number of rows from any non-system Table, *SAFELY*
BEGIN
    DECLARE @ActualTableName AS NVarchar(255)

    SELECT @ActualTableName = QUOTENAME( TABLE_NAME )
    FROM INFORMATION_SCHEMA.TABLES
    WHERE TABLE_NAME = @PassedTableName

    DECLARE @sql AS NVARCHAR(MAX)
    SELECT @sql = 'SELECT COUNT(*) FROM ' + @ActualTableName + ';'

    EXEC(@SQL)
END

---

Some have fairly asked why this is safer. Hopefully, little Bobby Tables can make this clearer: 0

---

Answers to more questions:

1. QUOTENAME alone is not guaranteed to be safe. MS encourages us to use it, but they have not given a guarantee that it cannot be out-foxed by hackers. FYI, real Security is all about the guarantees. The table lookup with QUOTENAME, is another story, it's unbreakable.

2. QUOTENAME is not strictly necessary for this example, the Lookup translation on INFORMATION_SCHEMA alone is normally sufficient. QUOTENAME is in here because it is good form in security to include a complete and correct solution. QUOTENAME in here is actually protecting against a distinct, but similar potential problem know as latent injection.

---

I should note that you can do the same thing with dynamic Column Names and the INFORMATION_SCHEMA.COLUMNS table.

You can also bypass the need for stored procedures by using a parameterized SQL query instead (see here: https://docs.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlcommand.parameters?view=netframework-4.8). But I think that stored procedures provide a more manageable and less error-prone security facility for cases like this.

Solution 2

(Un)fortunately there's no way of doing this - you can't use table name passed as a parameter to stored code other than for dynamic sql generation. When it comes to deciding where to generate sql code, I prefer application code rather that stored code. Application code is usually faster and easier to maintain.

In case you don't like the solution you're working with, I'd suggest a deeper redesign (i.e. change the schema/application logic so you no longer have to pass table name as a parameter anywhere).

Solution 3

I would argue against dynamically generating the SQL in the stored proc; that'll get you into trouble and could cause injection vulnerability.

Instead, I would analyze all of the tables that could be affected by the query and create some sort of enumeration that would determine which table to use for the query.

Solution 4

Sounds like you'd be better off with an ORM solution.

I cringe when I see dynamic sql in a stored procedure.

Solution 5

One thing you can consider is to make a case statement that contains the same SQL command you want, once for each valid table, then pass as a string the table name into this procedure and have the case choose which command to run.

By the way as a security person the suggestion above telling you to select from the system tables in order to make sure you have a valid table seems like a wasted operation to me. If someone can inject passed the QUOTENAME() then then injection would work on the system table just as well as on the underlying table. The only thing this helps with it to ensure it is a valid table name, and I think the suggestion above is a better approach to that since you are not using QUOTENAME() at all.

View more solutions