double-up
To answer your question, you need to experiment with alternatives to using sp_executesql with parameters:
- Using
EXEC (without sp_executesql)
- Using
sp_executesql (without parameters)
Both of which can lead to SQL injection attacks, under the right circumstances.
It's probably worth noting that even totally unparameterized, the code above is relatively low-risk, since the data types being passed are not string types, but it's still possible.
Strings carry a much higher risk of a malicious payload.
The code examples below are from my presentation about using dynamic SQL in a different context, but apply pretty well to your question.
string-safe
You can use code like this safely, because the user input isn't part of the string that gets executed:
DECLARE
@SQLString nvarchar(MAX) = N'',
@TableName sysname = N'Votes';
IF @TableName = N'Votes'
BEGIN
SET @SQLString += N'SELECT COUNT_BIG(*) AS records FROM dbo.Votes AS v;'
END
IF @TableName = N'Posts'
BEGIN
SET @SQLString += N'SELECT COUNT_BIG(*) AS records FROM dbo.Posts AS p;'
END
EXEC(@SQLString);
GO
unsafe-strings
In this example, user input is concatenated into the string that gets executed, and isn't parameterized. This can cause problems:
DECLARE
@SQLString nvarchar(MAX) = N'',
@Filter nvarchar(MAX) = N'',
@Title nvarchar(250) = N'''
UNION ALL
SELECT
t.object_id, t.schema_id, t.name, SCHEMA_NAME(t.schema_id), t.create_date, t.modify_date, NULL
FROM sys.tables AS t --';
/* This ends the current statement, and adds in some sneaky code */
SET @SQLString += N'
SELECT TOP (5000)
p.OwnerUserId, p.Score, p.Tags, p.Title, p.CreationDate, p.LastActivityDate, p.Body
FROM dbo.Posts AS p
WHERE p.OwnerUserId = 22656 ';
/* This appends the sneaky code onto our harmless query */
IF @Title IS NOT NULL
BEGIN
SET @Filter = @Filter + N'
AND p.Title LIKE ''' + N'%' + @Title + N'%''';
END;
IF @Filter IS NOT NULL
BEGIN
SET @SQLString += @Filter;
END;
SET @SQLString += N'
ORDER BY p.Score DESC;';
/* Check the messages tab... */
RAISERROR('%s', 0, 1, @SQLString) WITH NOWAIT;
/* Check the results -- what's that at the end? */
EXEC (@SQLString);
The end result is a query that gets executed like so, which searches the Title column for a single wildcard, and then an additional result that lists all the tables in the database.
SELECT TOP (5000)
p.OwnerUserId, p.Score, p.Tags, p.Title, p.CreationDate, p.LastActivityDate, p.Body
FROM dbo.Posts AS p
WHERE p.OwnerUserId = 22656
AND p.Title LIKE '%'
UNION ALL
SELECT
t.object_id, t.schema_id, t.name, SCHEMA_NAME(t.schema_id), t.create_date, t.modify_date, NULL
FROM sys.tables AS t --%'
ORDER BY p.Score DESC;
While many people will focus on memes like dropping tables, the real issue with dynamic SQL is usually theft of data. That's where the money is.
still-not-safe
Using sp_executesql is a good first step, but it still needs to be used with parameters. Code like below is still subject to SQL injection in the same way as above.
DECLARE
@SQLString nvarchar(MAX) = N'',
@Filter nvarchar(MAX) = N'',
@Title nvarchar(250) = N'''
UNION ALL
SELECT
t.object_id, t.schema_id, t.name, SCHEMA_NAME(t.schema_id), t.create_date, t.modify_date, NULL
FROM sys.tables AS t --';
/* This ends the current statement, and adds in some sneaky code */
SET @SQLString += N'
SELECT TOP (5000)
p.OwnerUserId, p.Score, p.Tags, p.Title, p.CreationDate, p.LastActivityDate, p.Body
FROM dbo.Posts AS p
WHERE p.OwnerUserId = 22656 ';
/* This appends the sneaky code onto our harmless query */
IF @Title IS NOT NULL
BEGIN
SET @Filter = @Filter + N'
AND p.Title LIKE ''' + N'%' + @Title + N'%''';
END;
IF @Filter IS NOT NULL
BEGIN
SET @SQLString += @Filter;
END;
SET @SQLString += N'
ORDER BY p.Score DESC;';
/* Check the messages tab... */
RAISERROR('%s', 0, 1, @SQLString) WITH NOWAIT;
/* Check the results -- what's that at the end? */
EXEC sys.sp_executesql
@SQLString;
The same query as above will be executed.
back-to-safety
Using code that lines up better with your example, we can avoid SQL injection by assigning the value to a parameter instead of concatenating it directly into the string.
DECLARE
@SQLString nvarchar(MAX) = N'',
@Filter nvarchar(MAX) = N'',
@Title nvarchar(250) = N'''
UNION ALL
SELECT
t.object_id, t.schema_id, t.name, SCHEMA_NAME(t.schema_id), t.create_date, t.modify_date, NULL
FROM sys.tables AS t --';
/* This ends the current statement, and adds in some sneaky code */
SET @SQLString += N'
SELECT TOP (5000)
p.OwnerUserId, p.Score, p.Tags, p.Title, p.CreationDate, p.LastActivityDate, p.Body
FROM dbo.Posts AS p
WHERE p.OwnerUserId = 22656 ';
/* This appends the sneaky code onto our harmless query */
IF @Title IS NOT NULL
BEGIN
SET @Filter = @Filter + N'
AND p.Title LIKE N''%'' + @Title + N''%'' ';
END;
IF @Filter IS NOT NULL
BEGIN
SET @SQLString += @Filter;
END;
SET @SQLString += N'
ORDER BY p.Score DESC;';
/* Check the messages tab... */
RAISERROR('%s', 0, 1, @SQLString) WITH NOWAIT;
/* Check the results -- what's that at the end now? */
EXEC sys.sp_executesql
@SQLString,
N'@Title NVARCHAR(250)',
@Title;
The results of our string-building are different this time. Now it looks like this:
SELECT TOP (5000)
p.OwnerUserId, p.Score, p.Tags, p.Title, p.CreationDate, p.LastActivityDate, p.Body
FROM dbo.Posts AS p
WHERE p.OwnerUserId = 22656
AND p.Title LIKE N'%' + @Title + N'%'
ORDER BY p.Score DESC;
Rather than returning a result, which includes the details from sys.tables, we get zero rows back because the parameter is set to the search string, and no post titles match it.
next-next-next
Hopefully this gives you a better understanding of how parameterized dynamic SQL can help you avoid SQL injection attacks.
There are other good reasons to use it, like better plan caching, but that's outside the scope of this question.
For some more information, check out my posts here:
