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.

Quite

SQL-Server 2008 allows TABLE variables. – Simmer

@Lance: True, it does, but I think (I could be wrong) that they work differently than that. A table variable stores table data, not table names. It's a similar concept to a temp table. – Quite

You do not have to "refactor the table names", they are already in the system supplied view: INFORMATION_SCHEMA.TABLES. – Elma

Answer 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://learn.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.

Elma

Although I don't like using dynamic SQL, this seems like the best option to solve the issue at hand. +1 – Acquah

Sorry, I can't get why is it SAFER? Safer in what regard? There's no guarantee that that table exists, i.e. we still can get a syntax error when doing EXEC(@SQL). – Cantatrice

Hmm...I don't know about your first statement...maybe you're using a different definition for SQL Injection than I am. Certainly, if I took that option, I would be "injecting" text into a SQL Server query, but I'm using it in the pejoritive sense where a user modifies something on purpose to send something to SQL that shouldn't be sent. I don't have to worry about that, since the user never sees or has access to any of the parameters that would be being sent. – Quite

See my comment on AlexS's answer. – Simmer

Oh, yes...I love Little Bobby Tables. Had that one posted in my cube for quite some time. – Quite

Beska: I think that you are confusing the exploit with the attack. Injection is when when unvalidated text is promoted to the command stream, it's an exploit that developers (that's us) inadvertently create. The attack is when someone intentionally tries to exploit it. – Elma

Cute (I mean 'Little Bobby Tables'). My point here is that querying INFORMATION_SCHEMA is redundant - QUOTENAME will do protection job alone. Anyway it seems to me that we're already well beyond the scope of original question. – Cantatrice

To some extent, I think we're arguing semantics. Some people are using SQL injection to mean an attack, which is the way I'm familiar with. Semantics aside, I'm not super worried about sending the text over because it wouldn't be unvalidated...the only things that I would be sending would be tables from a set list, essentially hardcoded into the system. (That's a bit oversimplified, but it's the basic idea.) Though I do understand the concern...if the user can modify anything that is being sent over, it's a potential avenue for exploitation. – Quite

I think what Beska is saying is that rather than allow the user to write "tablename" in an input field and using that directly with the SQL, they (and I'm guessing here) allow the user to select from a pulldown list that has, say, numbers as the values for the pulldown list. Those numbers are mapped to table names using internal code. If true, and again, I'm just guessing, that is not SQL injection, even if the table name is dynamically inserted into the SQL via application code. – Acquah

@AlexS: but querying Information_schema will determine that the table exists. You can then check for @ActualTableName being null and raise an error before execing. – Acquah

@Randolpho: pretty much, yep. (It's actually even a bit more obscure than that...the user never even gets to see it...it's attached to their profile, on the server side, and there's no way to edit it.) – Quite

I think this point ("being able to query for table existence") may be the idea that pushes me towards thinking this is the better option, since there's not a trivial way to do that code-side. – Quite

@AlexS: Also, I'd just like to point out that QUOTENAME isn't even necessary for SQL Injection protection at all, since that's actually handled by @PassedTableName being a parameterized value. All in all, this seems like the best choice to solve the issue Beska is having. Although I still don't like dynamic SQL. :) – Acquah

Beska: Yes I am familiar with the widespread tendency to refer to the attack as injection. But if you think about it, the attacker isn't actually injecting anything. It's the code that's actually doing the injecting, the attacker is just piggybacking on (exploiting) that. – Elma

Randophlo: Nope, that's a common misunderstanding of what parameterization does for us. It won't stop little Bobby Tables if we inject it into the command stream like we want to. – Elma

What parameterization does for use it to turn our text into an variable that can be used as "un-promoted" command arguments. That is like variables. But if you bypass that by injecting it into the command stream anyway, you forfeit that protection also. Since you have to do that for table names, column names, etc, you then also have to add in your own protection. – Elma

@RBarryYoung: I think you and I are arguing cross purposes here. In your example, you did not inject @PassedTableName into the command stream, you used it as a variable. Your query for getting @ActualTableName was therefore safe from SQL injection. You got @ActualTableName from the system itself rather than from the user; it was therefore safe from SQL injection. QUOTENAME was not necessary as a protection against SQL injection, since it was called against a system value, not against a user. Now, granted, this totally ignores permissions; do we really want the user to do this? – Acquah

There are many applications that allow the users to create their own custom tables or whatever name they want. ... Now, what if our current user had just made a table named after little Bobby Tables? Nasty, eh? – Elma

@Martin: yes, I probably should have used the SYSNAME datatype. I don't think that that would change anything else though. – Elma

@Martin: there's no need that I can see to use it (QUOTENAME) twice, the first time should have taken care of that. – Elma

@RBarry. I deleted the previous comment as I realised the second bit was wrong but in that case you don't need to add the square brackets manually which is what confused me. QUOTENAME('foo') = [foo] – Calvados

@Martin: Yeah, I just noticed that too, ... Hmm, I'll have to check to be sure... – Elma

Can sp_executesql be used instead of exec #548590 ? – Doucet

@MichaelFreidgeim yes. – Elma

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

Cantatrice

Well, it's not that I don't like it so much as I was hoping there was a better way to do it. If the current solution (modifying a sql string in code) is the best way, then if I want to be a good programmer, I had better like it, right? – Quite

AlexS: What your are recommending is SQL Injection. – Elma

@RBarryYoung: I recommend a 'deeper redesign ... so you no longer have to pass a table name as a parameter'. This is not 'SQL Injection' I guess ;-) – Cantatrice

@RBarryYoung: Using dynamic SQL is not 'SQL injection', it is just a method prone to 'SQL injection' that has to be done carefully. – Simmer

AlexS: "When it comes to deciding where to generate sql code, I prefer application code rather that stored code" was my concern. "Deeper redesign", is of course an excellent suggestion. – Elma

Lance: I never said that dynamic SQL was SQL Injection, and my Answer clearly argues the opposite. However, when we, as authoritative sources suggest to someone that they dynamically composite SQL code, without warning them about injection, then yes, we are in fact (unintentionally perhaps) suggesting SQL Injection, because that's the path of least resistance for any reader who doesn't know any better and 9 times out of 10, that's what the code will end up being. – Elma

@Cantatrice and @RBarryYoung...Deeper redesign is a good idea, conceptually, but infeasible here, sadly. – Quite

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

Acquah

I thought about this, but the problem is that there are many (about 50) tables that this could happen against, but perhaps more critically, more are being added periodically. – Quite

I'm not overly worried about SQL injection, since we are never querying the user for anything that would be going into the parameter...they're all internal values that a user wouldn't have access to. Still, better people than I have probably thought they were safe when they weren't, so the warning is well taken. – Quite

He's already got SQL Injection in the client code. It's no safer there than in the SQL Server. And it's perfectly possible to use dynamic SQL, including for this problem, without any injection. – Elma

It looks like @RBarry Young's answer may be the best bet. It's dynamic SQL, but it's safe enough to let you determine the table name. – Acquah

Answer 4

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

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

Ferdinandferdinanda

Some times it is unavoidable, like when using the PIVOT command. – Vizzone

Right, but generally the way you find out that you didn't do it correctly is if you get hacked. – Ferdinandferdinanda

@RBarryYoung: Yep, it's perfectly fine, if done safely. You should still cringe when you see it. If you're doing it, odds are you could have architected the system better. – Acquah

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

Mccullough

This last observation is wrong. The query that RBarryYoung wrote is NOT dynamic and does not do any updates and does not populate any variables with untrusted data. Therefore there is ZERO risk of either direct, latent or persisted SQL injection. Very different than doing EXEC('UPDATE ' + @tablename + ' SET...'). Not only is there no risk, but it is the key item that makes the full thing safe. – Plaintive

Answer 6

Depending on whether the set of columns in those tables is the same or different, I'd approach it in two ways in the longer term:

1) if they the same, why not create a new column that would be used as a selector, whose value is derived from the user-supplied parameters ? (is it a performance optimization?)

2) if they are different, chances are that handling of them is also different. As such, it seems like splitting the select/handle code into separate blocks and then calling them separately would be a most modular approach to me. You will repeat the "select * from" part, but in this scenario the set of tables is hopefully finite.

Allowing the calling code to supply two arbitrary parts of the table name to do a select from feels very dangerous.

Merca

Sadly, while I could have an enumeration to decide which table to select from, it would be very large (there are about 50 tables now), but worse, more are being added periodically, which would lead to a maintenance issue. – Quite

so you have more a case of (1) above rather than (2) ? e.g. something like a list of tables containing "Name, Surname", tables being named "Programmers", "Managers", "Directors", "Users", etc. ? – Merca

Answer 7

I don't know the reason why you have the data spread over several tables, but it sounds like you are breaking one of the fundamentals. The data should be in the tables, not as table names.

If the tables have more or less the same layout, consider if it would be best to put the data in a single table instead. That would solve your problem with the dynamic query, and it would make the database layout more flexible.

Jamey

I agree in theory, but no can do. It's a Commerce Server issue. I don't understand all the details of it, but it's a huge site, and the various catalog tables part of it is at the core of the site. There's no way we can change it in the short term. – Quite

Answer 8

Instead of Querying the tables based on user input values, you can pick the procedure instead. that is to say
1. Create a procedure FOO_BAR_prc and inside that you put the query 'select * from foo_bar' , that way the query will be precompiled by the database.
2. Then based on the user input now execute the correct procedure from your application code.

Since you have around 50 tables, this might not be a feasible solution though as it would require lot of work on your part.

Rijeka

Well, I'm not afraid of a lot of work if it's one time work, but maintaining 50 stored procs if we needed to, say, add a column to the query, would be nightmarish. – Quite

Answer 9

In fact, I wanted to know how to pass table name to create a table in stored procedure. By reading some of the answers and attempting some modification at my end, I finally able to create a table with name passed as parameter. Here is the stored procedure for others to check any error in it.

USE [Database Name] GO /****** Object: StoredProcedure [dbo].[sp_CreateDynamicTable] Script Date: 06/20/2015 16:56:25 ******/ SET ANSI_NULLS ON GO SET QUOTED_IDENTIFIER ON GO CREATE PROCEDURE [dbo].[sp_CreateDynamicTable] @tName varchar(255) AS BEGIN SET NOCOUNT ON; DECLARE @SQL nvarchar(max)

SET @SQL = N'CREATE TABLE [DBO].['+ @tName + '] (DocID nvarchar(10) null);'

    EXECUTE sp_executesql @SQL

END

Adder

Answer 10

@RBarry Young You don't need to add the brackets to @ActualTableName in the query string because it is already included in the result from the query in the INFORMATION_SCHEMA.TABLES. Otherwise, there will be error(s) when executed.

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 + '];'

-- changed to this
SELECT @sql = 'SELECT COUNT(*) FROM ' + @ActualTableName + ';'

EXEC(@SQL)

END

Kafiristan

Answer 11

I would avoid dynamic SQL at all costs.

Isn't the most elegant solution but does the job perfectly.

PROCEDURE TABLE_AS_PARAMTER (
        p_table_name IN VARCHAR2
    ) AS
    BEGIN
        CASE p_table_name
            WHEN 'TABLE1' THEN
                UPDATE TABLE1
                SET
                    COLUMN1 =1
                WHERE
                    ID =1;
            WHEN 'TABLE2' THEN
                UPDATE TABLE1
                SET
                    COLUMN1 =1
                WHERE
                    ID =2;
        END CASE;

        COMMIT;
    EXCEPTION
        WHEN OTHERS THEN
            ROLLBACK
END TABLE_AS_PARAMTER;

Hazy

Answer 12

You can use the prepared statement within your stored procedure for executing query.

Simple example demo:

DELIMITER //
CREATE PROCEDURE example (IN p_table_name VARCHAR(20), IN p_col VARCHAR(20))
BEGIN
  SET @sql = CONCAT('SELECT ', p_col, '_input, ', p_col, '_output', ' FROM ', p_table_name);
  PREPARE stmt FROM @sql;
  EXECUTE stmt;
  DEALLOCATE PREPARE stmt;
END//
DELIMITER ;

Here, if you have columns like "first_input", "first_output", "second_input", and "second_output" from table input_output.

If you want to retrieve data of "first_input", "first_output" from table input_output:

call example('input_output', 'first');

Westbound