-2

need a bit of help with this sql injection issue:

The following is a version of a parameterised stored procedure. Excluding how it is called from an application, is there anyway to prevent @v_string from being treated as dynamic SQL?

I think this is fairly water tight - there's no execute or concatenated sql, but still inserting a semicolon allows additional data to be returned.

I know there are multiple levels to consider this question on, but I want to know if there is some simple solution I am missing here as the majority of injection fixes involve dynamic queries.

create table dbo.Employee (EmpID int,EmpName varchar(60))


declare
    @v_id int,
    @v_string varchar(60)
begin

    set @v_string='test'''; waitfor delay '0:0:5' -- 

    if  @v_id is null

    begin
        set @v_id =            (select   EmpID 
                                from     Abc.Employee 
                                where    EmpName=@v_string);
    end


    print @v_id
end 
oooo ooo
  • 314
  • 1
  • 2
  • 11
  • The only time that you would get into trouble is if the SP uses parameters in the query that are directly provided by the user, and they are directly concatenated into the select statement (as opposed to being parameters). – Lynn Crumbling Jan 09 '17 at 13:17
  • "inserting a semicolon allows additional data to be returned." I can't see how that can happen. Can you give us details about where you're inserting this semicolon and how you know extra data is being returned because of it? – Matt Gibson Jan 09 '17 at 13:28
  • replace the waitfor delay with select * from sys.databases – oooo ooo Jan 09 '17 at 13:32
  • Please show the **full** stored procedure and how you are **calling** it (either through an EXEC call in SQL, or through e.g. .NET code). You seem to be mixing things up by manually putting "dangerous text" into a random piece of SQL and yes of course that text will then be executed as SQL. But that does not mean that the same text when put in `@v_string` (from the calling side) will lead to injection, on the contrary! – Peter B Jan 09 '17 at 13:36

2 Answers2

2

is there anyway to prevent @v_string from being treated as dynamic SQL?

I would not expect @v_string to be treated as dynamic SQL here since the T-SQL code has no EXECUTE or EXECUTE sp_executeSQL. The value will not be executed, but treated as a WHERE clause value not vulnerable to SQL injection.

If this doesn't answer your question, post a full example that demonstrates the value being treated as dynamic SQL.

Dan Guzman
  • 43,250
  • 3
  • 46
  • 71
  • for brevity its as above: the set `@v_string='test'''; waitfor delay '0:0:5' -- is the value that is passed in. As far as I can see when, the second set takes place the injected SQL is executed – oooo ooo Jan 09 '17 at 13:30
  • The @v_string value is passed to the query but the value within is not executed. The code in your updated question does not show the issue you are concerned about. – Dan Guzman Jan 09 '17 at 13:39
2

You're being confused by your own testing. The line:

 set @v_string='test'''; waitfor delay '0:0:5' -- 

Is creating a string @v_string with the value test', and then executing waitfor delay '0:0:5'. Then your actual Employee query is being run.

So if you run your query as is, with your additional example:

 set @v_string='test'''; select * from sys.databases

...what will happen is that line of code will set @v_string to be test', then immediately execute select * from sys.databases. Then the rest of your code will run, executing your actual select. So you'll see the result of select * from sys.databases, followed by the result of your Employee query, but only because you actually hard-coded the statement select * from sys.databases into your procedure without realising it :)

If you want the string @v_string to be set to test'; waitfor delay '0:0:5' then you've got the string quoting wrong. It should be:

set @v_string='test''; waitfor delay ''0:0:5'''
Community
  • 1
  • 1
Matt Gibson
  • 37,886
  • 9
  • 99
  • 128