3

I know that this is very dangerous:

EXEC sp_executesql 'SELECT * FROM ' + @Table

But, is this safe from SQL injection?

IF EXISTS(SELECT * FROM information_schema.tables WHERE TABLE_NAME = @Table) BEGIN
    EXEC sp_executesql 'SELECT * FROM ' + @Table
END

3 Answers 3

5

There is still a potential for injection here. Let's assume that:

  1. someone has the ability to create tables in this database, but not drop them
  2. that someone has the ability to call this procedure
  3. the procedure runs as an elevated user

That person could call this procedure to get results from foo, but in the meantime create a table called [foo;drop table bar], then pass foo;drop table bar in as the parameter value. It would pass your check, then you would blindly execute it. They get the results from foo but they also drop table bar.

Now, that is a pretty contrived scenario, and you probably aren't worried about internal people dropping tables, but they could also use this to exploit data they don't have access to, for example they could create a table called [foo;SELECT name,salary FROM dbo.Employees;] and end up with two result sets, one with data you probably shouldn't be letting them see.

There is also a potential for getting data from the wrong table, in the event that there are tables with the same names in different schemas. Always, always, always use the schema prefix when creating or referencing objects:

Finally, don't use INFORMATION_SCHEMA. The catalog views and metadata functions are much more complete, current and reliable:

I would probably do it this way (and I'm going to assume for simplicity that all of your tables are in dbo):

DECLARE @table SYSNAME; -- procedure parameter, right?

DECLARE @sql NVARCHAR(MAX);
IF OBJECT_ID(N'dbo.' + QUOTENAME(@table), N'U') IS NOT NULL
BEGIN
  SET @sql = N'SELECT * FROM dbo.' + QUOTENAME(@table) + ';';
  EXEC sp_executesql @sql;
END

This way, even if they pass foo;drop table bar or foo;select name,salary from dbo.employees as the table name, that whole string gets surrounded in square brackets, thus the worst thing that can happen is they'll get the results from the table they created, rather than any other table or worse.

And I would ensure that the procedure runs as that user, rather than escalate using EXECUTE AS (a common bypass for proper permissions configuration). Also see:

2
  • +1 for a great, comprehensive answer. I guess I should stop using my phone to post answers! Lesson learned!
    – Hannah Vernon
    Jul 16, 2014 at 14:25
  • 1
    If I ever see a table name with a semi-colon embedded, I'm going to kick the dba who let that happen square in the ahem.
    – Hannah Vernon
    Jul 16, 2014 at 14:37
2

Have you tried executing this, both of the scripts? Because this is not a correct syntax.I will assume that the first script intended to be something like:

declare @tVariable nvarchar(max)
declare @stmt nvarchar(max) 
declare @par1 nvarchar(500); 

set @tVariable= 'sysusers'    
set @stmt = 'select * from ' +@tVariable
set @par1 = null
EXEC sp_executesql @stmt, @par1

But it can be easily hacked if you change the set @tVariable= 'sysusers' with [set @tVariable= 'sysobjects; drop table SOMETABLE'][1]

So instead you should really use the parameters as the sp_executesql is supposed to be used.

declare @tVariable nvarchar(50)
declare @stmt nvarchar(max) 
declare @par1 nvarchar(500);    

set @tVariable= 'sysusers'    
set @stmt = 'select * from ' +@tVariable +' order by 1'
set @par1 = N'@tVariable nvarchar(50)'
EXEC sp_executesql @stmt, @par1, @tVariable
4
  • You have two set @stmt = ... in the 2nd code sample. I guess one of them should be removed. Jul 16, 2014 at 10:26
  • You know that you're passing in the parameter @tVariable but it is never used because you've already concatenated it into @stmt, right? Jul 16, 2014 at 13:31
  • of course it will be used, the idea was to prevent the sql injection of malicious queries inputed for @tVariable, but I got confused, because the second query as I wrote it can still be missused. In order to not be able to execute scripts like select * from sysusers; drop table SOMETABLE AN ORDER BY or GROUP BY or WHERE clauses should be added.
    – yrushka
    Jul 18, 2014 at 9:32
  • The parameter will absolutely not be used - it will be passed in but nothing will happen to it. Look closer, you've already concatenation it to the string and it's no longer a parameter. You can't parameter use a table or column name anyway... Jul 22, 2014 at 14:24
1

Yes. By checking the parameter first, you are removing the injection vulnerability.

NO - As Aaron pointed out in his answer, you could still create SQL Injection using this construct.

Just to be clear, you should never concatenate SQL. Use sp_executesql's built-in parameter handling.

7
  • I'd provide an example, but I'm writing this on my phone.
    – Hannah Vernon
    Jul 16, 2014 at 4:54
  • 1
    Haha, thanks. That was my next question. :)
    – Jargs
    Jul 16, 2014 at 5:03
  • 1
    See the first example here: msdn.microsoft.com/en-us/library/ms188001.aspx
    – Hannah Vernon
    Jul 16, 2014 at 5:11
  • I'm going to mess with it some more tomorrow, but it doesn't look like I can set the table name up as a sp_executesql parameter. It keeps telling me "Must declare the table variable @Table"
    – Jargs
    Jul 16, 2014 at 5:40
  • 3
    @Max I agree with using parameters, but if you're passing in a table name, at some point you have to just concatenate or replace tokens, because the parameter cannot be a table / column etc. Jul 16, 2014 at 13:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.