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

  • +1 for a great, comprehensive answer. I guess I should stop using my phone to post answers! Lesson learned! – Max Vernon Jul 16 '14 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. – Max Vernon Jul 16 '14 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
  • You have two set @stmt = ... in the 2nd code sample. I guess one of them should be removed. – ypercubeᵀᴹ Jul 16 '14 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? – Aaron Bertrand Jul 16 '14 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 '14 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... – Aaron Bertrand Jul 22 '14 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.

  • I'd provide an example, but I'm writing this on my phone. – Max Vernon Jul 16 '14 at 4:54
  • 1
    Haha, thanks. That was my next question. :) – Jargs Jul 16 '14 at 5:03
  • 1
    See the first example here: msdn.microsoft.com/en-us/library/ms188001.aspx – Max Vernon Jul 16 '14 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 '14 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. – Aaron Bertrand Jul 16 '14 at 13:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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