6

Let's imagine a stored procedure that retreives data and do some kind of pagination. This procedure has some inputs describing which set of data we want and how we sort it.

Here is a very simple query, but let's take it as an example.

create table Persons(id int, firstName varchar(50), lastName varchar(50))
go
create procedure GetPersons @pageNumber int = 1, @pageSize int = 20, @orderBy varchar(50) = 'id', @orderDir varchar(4) = 'desc'
as

declare @sql varchar(max)
set @sql = 'select id, firstName, lastName
from (
    select id, firstName, LastName, row_number() over(order by '+@orderBy+' '+@orderDir+') as rn
    from Persons
    ) t
where rn > ('+cast(@pageNumber as varchar)+'-1) * '+cast(@pageSize as varchar)+'
        and rn <= '+cast(@pageNumber as varchar)+' * '+cast(@pageSize as varchar)+' 
order by '+@orderBy+' '+@orderDir

exec(@sql)

It is supposed to be used like that:

exec GetPersons @pageNumber = 1, @pageSize = 20, @orderBy = 'id', @orderDir = 'desc'

But a smart guy could launch:

exec GetPersons @pageNumber = 1, @pageSize = 20, @orderBy = 'id)a from Persons)t;delete from Persons;print''', @orderDir = ''

... and drop data

That's obviously not a secure situation. And how could we prevent it ?

Note: this question is not about "is it a good way to do pagination ?" nor "is it a good thing to do dynamic sql?". The question is about preventing code injection when building sql queries dynamicaly in order to have some guidelines to make the code a bit cleaner if we have to do similar stored procedures again in the future.

Some basic ideas:

Validate inputs

create procedure GetPersons @pageNumber int = 1, @pageSize int = 20, @orderBy varchar(50) = 'id', @orderDir varchar(4) = 'desc'
as

if @orderDir not in ('asc', 'desc') or @orderBy not in ('id', 'firstName', 'lastName')
begin
    raiserror('Cheater!', 16,1)
    return
end

declare @sql varchar(max)
set @sql = 'select id, firstName, lastName
from (
    select id, firstName, LastName, row_number() over(order by '+@orderBy+' '+@orderDir+') as rn
    from Persons
    ) t
where rn > ('+cast(@pageNumber as varchar)+'-1) * '+cast(@pageSize as varchar)+'
        and rn <= '+cast(@pageNumber as varchar)+' * '+cast(@pageSize as varchar)+' 
order by '+@orderBy+' '+@orderDir

exec(@sql)

Pass ids instead of strings as inputs

create procedure GetPersons @pageNumber int = 1, @pageSize int = 20, @orderBy tinyint = 1, @orderDir bit = 0
as

declare @orderByName varchar(50)
set @orderByName =  case @orderBy when 1 then 'id'
                        when 2 then 'firstName'
                        when 3 then 'lastName'
                    end 
                +' '+case @orderDir 
                        when 0 then 'desc' 
                        else 'asc' 
                    end

if @orderByName is null
begin
    raiserror('Cheater!', 16,1)
    return
end

declare @sql varchar(max)
set @sql = 'select id, firstName, lastName
from (
    select id, firstName, LastName, row_number() over(order by '+@orderByName+') as rn
    from Persons
    ) t
where rn > ('+cast(@pageNumber as varchar)+'-1) * '+cast(@pageSize as varchar)+'
        and rn <= '+cast(@pageNumber as varchar)+' * '+cast(@pageSize as varchar)+' 
order by '+@orderByName

exec(@sql)

Any other suggestions?

9

In your example code, you are passing three categories of "things" into your dynamic SQL.

  1. You pass @OrderDir, which is a keyword to signify ASC or DESC.
  2. You pass @OrderBy, which is a column name (or potentially a set of column names, but based on the way #1 is implemented, I assume you expect a single column name.
  3. You pass @PageNumber and @PageSize, which become literals in the generated string.

Keywords

This is really straightforward--you just want to validate your input. You're spot-on that this is the right thing for this option. In this case, you are expecting either ASC or DESC, so you can either check that the user passes one of those values, or you can switch to a different parameter semantic, where you have a parameter that is a toggle switch. Declare your parameter as @SortAscending bit = 0, then within your stored procedure, translate the bit into either ASC or DESC.

Column names

Here, you should use the QUOTENAME function. Quotename will ensure that objects get properly [quoted], ensuring that if someone tries to pass in a "column" of "; TRUNCATE TABLE USERS", it will get treated as a column name, and not an arbitrary piece of injected code. This will fail, rather than truncating the USERS table:

SELECT [; TRUNCATE TABLE USERS]...
FROM...

Literals & parameters

For @PageNumber and @PageSize, you should be using sp_executesql to pass parameters properly. Properly parameterizing your dynamic SQL allows you to not only pass values in, but also to get values back out.

In this example, @x and @y would be variables scoped to your stored procedures. They aren't available within your dynamic SQL, so you pass them into @a and @b, which are scoped to the dynamic SQL. This allows you to have properly typed values both inside and outside the dynamic SQL.

DECLARE @i int,
 @x int,
 @y int,
 @sql nvarchar(1000),
 @params nvarchar(1000);


SET @x = 10;
SET @y = 5;
SET @params = N'@i_out int OUT, @a int, @b int';
SET @sql    = N'SELECT @i_out = @a + @b';


EXEC sp_executesql @sql, @params, @i_out = @i OUT, @a = @x, @b = @y; 
SELECT @i;

Even with varchar values, keeping the value as a variable prevents someone from arbitrarily passing code that gets executed. This example ensures that the user input gets SELECTed, and not arbitrarily executed:

DECLARE @UserInput varchar(100),
         @params nvarchar(1000) = N'@value varchar(100)',
         @sql nvarchar(1000)    = N'SELECT Value = @value';

SET @UserInput = '; TRUNCATE TABLE USERS;'
EXEC sp_executesql @sql, @params, @value = @UserInput;  

My code

Here's my version of your stored procedure, with table definition & a few sample rows:

CREATE TABLE dbo.Persons
       (
        id INT,
        firstName VARCHAR(50),
        lastName VARCHAR(50)
       );
GO

INSERT INTO dbo.Persons(id, firstName,lastName)
VALUES (1,'George','Washington'),
       (2,'John','Adams'),
       (3,'Thomas','Jefferson'),
       (4,'James','Madison'),
       (5,'James','Monroe')


ALTER PROCEDURE dbo.GetPersons
       @pageNumber INT = 1,
       @pageSize INT = 20,
       @orderBy VARCHAR(50) = 'id',
       @orderDir VARCHAR(4) = 'desc'
AS
       SET NOCOUNT ON;

--validate inputs
IF NOT EXISTS ( SELECT   1 FROM     sys.columns
                WHERE    object_id = OBJECT_ID('dbo.Persons')
                AND name = @orderBy )
BEGIN
        RAISERROR('Order by column does not exist.', 16,1);
        RETURN;
END;

IF (@orderDir NOT IN ('ASC', 'DESC'))
BEGIN
        RAISERROR('Order direction is invalid. Must be ASC or DESC.', 16,1);
        RETURN;
END;

--Now do stuff
--sp_executesql takes in nvarchar as a datatype
DECLARE @sql NVARCHAR(MAX);

SET @sql = N'SELECT id, firstName, lastName
FROM (
    SELECT id, firstName, LastName, ROW_NUMBER() OVER(ORDER BY '
           + QUOTENAME(@orderBy) + N' ' + @orderDir + N') AS rn
    FROM dbo.Persons
    ) t
WHERE rn > ( @pageNumber-1) * @pageSize
        AND rn <= @pageNumber * @pageSize 
ORDER BY ' + QUOTENAME(@orderBy) + N' ' + @orderDir;

EXEC sys.sp_executesql @sql, N'@pageNumber int, @pageSize int',
                   @pageNumber = @pageNumber, @pageSize = @pageSize;

GO

You can see here, that the code is functional & gives you the proper ordering & pagination:

EXEC dbo.GetPersons @OrderBy = 'id', @orderDir = 'DESC';
EXEC dbo.GetPersons @OrderBy = 'id', @orderDir = 'ASC';
EXEC dbo.GetPersons @OrderBy = 'firstName';
EXEC dbo.GetPersons @OrderBy = 'lastName';
EXEC dbo.GetPersons @PageNumber = 2, @PageSize = 1, @OrderBy = 'lastName', @orderDir = 'ASC';

And also see how the input handling protects against someone trying to do strange stuff:

EXEC dbo.GetPersons @OrderBy = 'lastName', @orderDir = 'UP';
EXEC dbo.GetPersons @OrderBy = ';TRUNCATE TABLE Persons;';

Additional Reading

sp_executesql example

Aaron Bertrand's Bad Habits to Kick : Using EXEC() instead of sp_executesql

Aaron Bertrand's kitchen sink procedure

  • Validating the column name exists helps you ensure that you can bubble up a useful error message, and short-circuit back to the user straight away, rather than waiting for the compiler to barf on it, and returning the compiler's error. Or if you have a multi-statement stored procedure, doing 30 seconds (or 30 minutes) worth of work before failing at the end is a crummy UX. – AMtwo Apr 14 '17 at 21:52
  • Depending on the scenario, it could be overkill, or it could be useful. I figured I'd include it in my answer in case it's useful. – AMtwo Apr 14 '17 at 21:52
3

A common method to mitigate SQL injection is to use QUOTENAME around variables that are passed into the stored procedure.

So, in your example, the code could be modified like this:

declare @sql varchar(max)
set @sql = 'select id, firstName, lastName
from (
    select id, firstName, LastName, row_number() over(order by '+@orderBy+' '+@orderDir+') as rn
    from Persons
    ) t
where rn > ('+cast(@pageNumber as varchar)+'-1) * '+cast(@pageSize as varchar)+'
        and rn <= '+cast(@pageNumber as varchar)+' * '+cast(@pageSize as varchar)+' 
order by '+ Quotename(@orderBy)+' '+@orderDir

If someone tried to pass in the extra 'delete' command, the execution would error out because the resulting dynamic SQL looks like this:

select id, firstName, lastName
from (
    select id, firstName, LastName, row_number() over(order by id)a from Persons)t;delete from Persons;print' ) as rn
    from Persons
    ) t
where rn > (1-1) * 20
        and rn <= 1 * 20 
order by [id)a from Persons)t;delete from Persons;print'] 

resulting in this error:

Msg 102, Level 15, State 1, Line 27
Incorrect syntax near ']'.

Also, Aaron Bertrand has a great blog about Bad Habits to Kick : Using EXEC() instead of sp_executesql

  • quotename is indeed a good advice, but not good enough in this case if we move the quote: exec GetPersons @pageNumber = 1, @pageSize = 20, @orderBy = 'id)a from Persons)t;delete from Persons;print', @orderDir = '''' – irimias Apr 14 '17 at 13:31
2

An obvious solution is not to use dynamic SQL. I think your task can be accomplished with regular, non-dynamic T-SQL code, which also gives you other advantages in terms of security (like ownership chaining).

So instead of:

declare @sql varchar(max)
set @sql = 'select id, firstName, lastName
from (
    select id, firstName, LastName, row_number() over(order by '+@orderByName+') as rn
    from Persons
    ) t
where rn > ('+cast(@pageNumber as varchar)+'-1) * '+cast(@pageSize as varchar)+'
        and rn <= '+cast(@pageNumber as varchar)+' * '+cast(@pageSize as varchar)+' 
order by '+@orderByName

exec(@sql)

You could, for instance..

SELECT id, firstName, lastName
FROM (
    SELECT id, firstName, lastName, ROW_NUMBER() OVER (
        ORDER BY (CASE @OrderByName
                  WHEN 1 THEN id END),
                 --- different datatypes, I'm assuming
                 (CASE
                  WHEN 2 THEN firstName
                  WHEN 3 THEN lastName END)) AS rn
    FROM persons
    ) AS t
WHERE rn > (@pageNumber-1) * @pageSize
  AND rn <= @pageNumber * @pageSize
ORDER BY rn;

Read up on,

  • I totally agree, but it was just an example and as I said this question is not about "is it a good way to do pagination ?" nor "is it a good thing to do dynamic sql?" The question is about preventing code injection when building sql queries dynamicaly – irimias Apr 14 '17 at 10:07
  • I did read the question. You're looking for a pattern for dynamic kitchen-sink-type queries, and this is the pattern I normally use. – Daniel Hutmacher Apr 14 '17 at 10:12
0

@Scott Hodgin's answer touches on this, but basically the best approach when generating Dynamic SQL Strings that are client/app facing is to utilize sp_executesql.

While not entirely foolproof for eliminating SQL Injection attacks, sp_executesql is likely the best you're going to get. The article Scott links to by Aaron Bertrand is pretty straight forward, but to quickly summarize the benefits of sp_executesql over other approaches is that it:

  1. Utilizes strongly-typed variables within the string
  2. Has better chance of query plan reuse

The first point is what I feel is the most important as related to your question because you can limit your parameters' length, type, etc. This makes is incredibly more difficult to inject nasty code.

As to provide an even more complete answer, I've updated your sp accordingly. Interesting enough, in your case because you're trying to parameterize column literals, you'll need to nest sp_executesql statements, so the first nested statement sets column names as literals and the second execution passes in the pagination values, as follows:

create procedure GetPersons @pageNumber int = 1, @pageSize int = 20, @orderBy varchar(50) = 'id', @orderDir varchar(4) = 'desc'
as

if @orderDir not in ('asc', 'desc') or @orderBy not in ('id', 'firstName', 'lastName')
begin
    raiserror('Cheater!', 16,1)
    return
end

declare @sql nvarchar(max), @sql_out nvarchar(max)
set @sql = 'SELECT @sql_out = ''select id, firstName, lastName
from (
    select id, firstName, LastName, row_number() over(order by '' + @oB + '' '' + @oD + '') as rn
    from Persons
    ) t
where rn > (@pN -1) * @pS
        and rn <= @pN * @pS
order by '' + @oB + '' '' + @oD + '''''

--PRINT(@sql)
EXEC sp_executesql @sql, N'@oB varchar(50), @oD varchar(4), @sql_out nvarchar(max) OUTPUT', @oB=@orderBy, @oD=@orderDir, @sql_out=@sql_out OUTPUT
EXEC sp_executesql @sql_out, N'@pN int, @pS int', @pN=@pageNumber, @pS=@pageSize
0

Simple option - join to sys.columns to make sure it's a valid column name, and use CASE to default to ASC if anything other than DESC is passed in.

(oh, and use nvarchar(max) for @sql and sp_executesql)

declare @sql nvarchar(max)
select @sql = 'select id, firstName, lastName
  from (
    select id, firstName, LastName, row_number() over(order by '+QUOTENAME(c.name)+' '+ CASE WHEN @orderDir = 'DESC' THEN 'DESC' ELSE 'ASC' END +') as rn
    from Persons
    ) t
  where rn > ('+cast(@pageNumber as varchar)+'-1) * '+cast(@pageSize as varchar)+'
    and rn <= '+cast(@pageNumber as varchar)+' * '+cast(@pageSize as varchar)+' 
order by '+QUOTENAME(c.name)+' '+ CASE WHEN @orderDir = 'DESC' THEN 'DESC' ELSE 'ASC' END
FROM sys.columns c
WHERE c.name = @orderby
AND c.object_id = OBJECT_ID('dbo.Persons');

EXEC sp_executesql @sql;

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.