Building Dynamic SQL-Server Where Clause

Question

Let us review this dba.exchange Oracle question for SQL-Server.

This is SaUce's code, after a little formatting:

CREATE PROCEDURE GetCustomer 
    @FirstN nvarchar(20) = NULL, 
    @LastN nvarchar(20) = NULL, 
    @CUserName nvarchar(10) = NULL, 
    @CID nvarchar(15) = NULL 
as
begin 
    DECLARE @sql nvarchar(4000)

    SELECT @sql = 'C_FirstName, C_LastName, C_UserName, C_UserID 
FROM CUSTOMER
WHERE 1=1 '

    IF @FirstN IS NOT NULL 
        SELECT @sql = @sql + ' AND C_FirstName like @FirstN ' 

    IF @LastN IS NOT NULL 
        SELECT @sql = @sql + ' AND C_LastName like @LastN ' 

    IF @CUserName IS NOT NULL 
        SELECT @sql = @sql + ' AND C_UserName like @CUserName ' 

    IF @CID IS NOT NULL 
        SELECT @sql = @sql + ' AND C_UserID like @CID ' 

    EXEC sp_executesql @sql, N'@C_FirstName nvarchar(20), @C_LastName nvarchar(20), @CUserName nvarchar(10), @CID nvarchar(15)', @FirstN, @LastN, @CUserName, @CID
end
go

SaUce mentioned in his second note, that he had 1000 lines of code to prevent SQL-Injection. My feeling is that he needn't do this to shield his stored procedure.

My Question

• is this SQL-Server Procedure immune against SQL-Injection by itself
• is it as satisfying solution with respect to performance.

Answer 1

According to Erland's Sommarskog's article about dynamic sql, the use of parameterized queries should be the best point in fighting against sql injection. About performance, I also see no problem, because the execution plan of dynamic sql is reused since MSSQl 2005, if I remember correctly. Only in sql 2000 and older there was a problem with plan reuse.

His points in taking over sql injection are: "

* Never run with more privileges than necessary. Users that log into an application with their own login should normally only have EXEC permissions on stored procedures. If you use dynamic SQL, it should be confined to reading operations so that users only need SELECT permissions. A web site that logs into a database should not have any elevated privileges, preferably only EXEC and (maybe) SELECT permissions. Never let the web site log in as sa!

* For web applications: never expose error messages from SQL Server to the end user.

* Always used parameterised statements. That is, in a T-SQL procedure use sp_executesql, not EXEC().

The first point is mainly a safeguard, so that if there is a injection hole, the intruder will not be able to do that much harm. The second point makes the task for the attacker more difficult as he cannot get feedback from his attempts."

Answer 2

Yes, this procedure is safe from SQL Injection, provided that it is called from a properly parametrized .NET application.

Performance will be fine, provided that you have all the needed indexes in place.

Answer 3

I disagree with performance. Since this dynamic sql is string concatenation, optimizer needs to store every combination. There are 4 parameters therefore it needs to store 2^4=16 branches.

While without using string concatenation like answer of Leigh Riffel on Building Dynamic Oracle Where Clause. Optimizer will need to store only 1 path. Optimizer may avoid this with trivial sql like this. But nonetheless it is good practice to avoid this type of string concatenation even with trivial sql. One mistake may lead to sql injection. Even developer writing this is experienced it is not guaranteed that next maintainer will be as knowledgeable.

Also normally sp_executesql is not safe from sql injection. You call it with parameters like below.

EXEC sp_executesql @sql, N'@C_FirstName nvarchar(20), @C_LastName nvarchar(20), @CUserName nvarchar(10), @CID nvarchar(15)', @FirstN, @LastN, @CUserName, @CID

This calling with parameters makes it immune.