Preventing SQL injection in dynamic SQL
In your example code, you are passing three categories of "things" into your dynamic SQL.
- You pass @OrderDir, which is a keyword to signify
ASC
orDESC
. - 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.
- 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 SELECT
ed, 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
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
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,
- Aaron Bertrand's kitchen sink procedure
- Ownership chaining
OFFSET FETCH