Take the 2-minute tour ×
Database Administrators Stack Exchange is a question and answer site for database professionals who wish to improve their database skills and learn from others in the community. It's 100% free, no registration required.

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.
share|improve this question
    
If you are building dynamic where clause for searching the data based on user input I highly recommend the article Dynamic Search Conditions in T-SQL by Erland Sommarskog –  Giorgi Jan 22 '11 at 19:03
    
+1. Erland has some excellent well researched SQL Server detail on his site. –  David Spillett Jan 23 '11 at 12:36

3 Answers 3

up vote 4 down vote accepted

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."

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.