Prevent dynamic SQL search from SQL injection

Question

How can I make the following code SQL injection safe? I know that the problem is the following line:

SET @sqlCommand = @sqlCommand + 'Event.Name LIKE ' + '''%' + @name + '%'''

But I don't know how to make it SQL injection safe. I heard something about REPLACE but this doesn't solve the problem as a whole.

CREATE PROCEDURE searchEvents @name VARCHAR(50), @location VARCHAR(20), @postcode CHAR(4), @address VARCHAR(40), @startDate DATETIME, @endDate DATETIME
    AS
    DECLARE
    @sqlCommand NVARCHAR(MAX) = 'SELECT Event.Name, Description, Location.Name AS Location, Postcode, Address, StartDate, EndDate, Website FROM Event JOIN Location ON Event.LocationID = Location.LocationID',
    @parameters NVARCHAR(MAX),
    @whereIncluded BIT = 0
    BEGIN
    IF @name IS NOT NULL
        BEGIN
            IF @whereIncluded = 0
            BEGIN
                SET @sqlCommand = @sqlCommand + ' WHERE '
                SET @whereIncluded = 1
            END
            ELSE
                SET @sqlCommand = @sqlCommand + ' AND '

            SET @sqlCommand = @sqlCommand + 'Event.Name LIKE ' + '''%' + @name + '%'''
        END

    -- It's the same if clause for all parameters like above

    SET @parameters = '@p_name VARCHAR(50), @p_location VARCHAR(20), @p_postcode CHAR(4), @p_address VARCHAR(40), @p_startDate DATETIME, @p_endDate DATETIME'

    EXEC sp_executesql
    @sqlCommand,
    @parameters,
    @p_name = @name,
    @p_location = @location,
    @p_postcode = @postcode,
    @p_address = @address,
    @p_startDate = @startDate,
    @p_endDate = @endDate
    END

Short answer, you would parameterize name just like the other parameters in your dynamic sql. But for a more complete solution for this type of thing you read this article on the topic of catch all queries which this most certainly is an example. sqlinthewild.co.za/index.php/2009/03/19/catch-all-queries — Sean Lange
– Sean Lange, Commented May 10, 2018 at 14:31
Well............the article I referenced explains it. I would also point out that you have a bunch of other parameters here but you are not actually doing anything with them. I am guessing you want to search for any of those criteria. Again, look at the article. It explains how to do this exactly. — Sean Lange
– Sean Lange, Commented May 10, 2018 at 15:28

Thom A · Accepted Answer · 2018-05-10 14:59:43Z

Parametrise your SQL. Statements like SET @sqlCommand = @sqlCommand + 'Event.Name LIKE ' + '''%' + @name + '%''' are awful.

I'm not going to go too indepth here, there are 100's of example on how to make your SQL "safe(r)". HOwever, her's a few pointers...

Parametrisation:

Concatenating strings for variables is a sure way to leave yourself open to injection. Take the simple example below:

DECLARE @SQL nvarchar(MAX);
DECLARE @name varchar(1000);

SET @SQL = N'
SELECT *
FROM MyTable
WHERE [Name] = ''' + @Name + ''';';

EXEC (@SQL);

This is an awful example of Dynamic SQL. If you set the value of @name to ''; DROP TABLE MyTable;--' then SQL statement becomes:

SELECT *
FROM MyTable
WHERE [Name] = ''; DROP TABLE MyTable;-- ';

Oh good! Your table, MyTable has been dropped. The correct way would be:

DECLARE @SQL nvarchar(MAX);
DECLARE @name varchar(1000);

SET @SQL = N'
SELECT *
FROM MyTable
WHERE [Name] = @dName;';

EXEC sp_executesql @SQL, N'@dname varchar(1000)', @dName = @Name;

Dynamic Objects:

This is another common mistake people make. They have a query along the lines of:

DECLARE @SQL nvarchar(MAX);
DECLARE @Table varchar(1000);

SET @SQL = N'SELECT * FROM ' + @Table;
EXEC (@SQL);

This suffers exactly the same problem as above. You can't pass a variable as an object name, so you need to do this a little differently. This is by preferred method:

DECLARE @SQL nvarchar(MAX);
DECLARE @Table sysname; --notice the type here as well.

SELECT @SQL = N'SELECT *' + NCHAR(10) +
              N'FROM ' + (SELECT QUOTENAME(t.[name]) --QUOTENAME is very important
                          FROM sys.tables t
                          WHERE t.[name] = @Table) + N';';
PRINT @SQL;
EXEC sp_executesql @SQL;

Why am I querying sys.tables? Well, this means if you do pass a nonsense table name in, the value of @SQL will be NULL; meaning that the dynamic SQL is completely harmless.

Like I said, this is just the basics; SO isn't the right place for a full answer here. There are 100's of articles on this subject, and you'll learn far more via your own research.

Branko Dimitrijevic · Accepted Answer · 2018-05-10 15:35:51Z

0

You can just...

SELECT
    ...
WHERE
    @name IS NULL
    OR Event.Name LIKE '%' + @name + '%'

You don't even need dynamic SQL in this case, so you can execute the above SQL directly, instead of through sp_executesql.

Be careful about the performance though - a LIKE operand starting with % may cause a full table (or clustered index) scan.

answered May 10, 2018 at 15:35

Branko Dimitrijevic

52.3k12 gold badges103 silver badges174 bronze badges

Collectives™ on Stack Overflow

Prevent dynamic SQL search from SQL injection

2 Answers 2

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related