views:

474

answers:

7

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.

+2  A: 

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.

Randolpho
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.
Beska
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.
Beska
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.
RBarryYoung
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.
Randolpho
+5  A: 

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

AlexS
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?
Beska
AlexS: What your are recommending is SQL Injection.
RBarryYoung
@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 ;-)
AlexS
@RBarryYoung: Using dynamic SQL is not 'SQL injection', it is just a method prone to 'SQL injection' that has to be done carefully.
Lance Roberts
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.
RBarryYoung
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.
RBarryYoung
@AlexS and @RBarryYoung...Deeper redesign is a good idea, conceptually, but infeasible here, sadly.
Beska
+2  A: 

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

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

ScottE
As do we all...
Beska
It's perfectly fine, *IF* you do it correctly.
RBarryYoung
Some times it is unavoidable, like when using the PIVOT command.
BoltBait
Right, but generally the way you find out that you didn't do it correctly is if you get hacked.
ScottE
@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.
Randolpho
+11  A: 

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 shaerd 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:

alt text


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.

RBarryYoung
Although I don't like using dynamic SQL, this seems like the best option to solve the issue at hand. +1
Randolpho
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).
AlexS
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.
Beska
See my comment on AlexS's answer.
Lance Roberts
Oh, yes...I love Little Bobby Tables. Had that one posted in my cube for quite some time.
Beska
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.
RBarryYoung
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.
AlexS
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.
Beska
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.
Randolpho
@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.
Randolpho
@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.)
Beska
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.
Beska
@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. :)
Randolpho
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.
RBarryYoung
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.
RBarryYoung
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.
RBarryYoung
@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?
Randolpho
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?
RBarryYoung
A: 

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.

Andrew Y
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.
Beska
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. ?
Andrew Y
A: 

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.

Guffa
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.
Beska
A: 

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.

Rajat
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.
Beska