SQL Server Security – Preventing SQL Injection in Dynamic SQL

Securitysql server

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?

Best Answer

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

Related Question