Are these SQL injection checks a bad idea?

Question

Reading Preventing SQL Injection Attacks in Stored Procedures started this debate between my partner and I---and looking for the right approach is not as straight forward.

I'm no security expert--but what do a person do when you take a look at the first few lines of this stored procedure....and you see this.

We haven't had any significant security issues, but I don't won't to assume that this approach is working...If so leave it alone or make it better.

ASP:

Dim conStr = ConfigurationSettings.AppSettings("WebUser")
Dim command = ("Execute dbo.web_Insert_usr_Comments @Location," & _
                    "@userexp, @full_name, @emailAddr, @comments")

Using con As New SqlConnection(conStr)
    Using cmd As New SqlCommand(command, con)
        cmd.Parameters.AddWithValue("@Location", Request.QueryString("Location"))
        cmd.Parameters.AddWithValue("@userexp", Request.QueryString("usrexp"))
        cmd.Parameters.AddWithValue("@comments", Request.QueryString("comments"))
        cmd.Parameters.AddWithValue("@full_name", Request.QueryString("full_name"))
        cmd.Parameters.AddWithValue("@emailAddr", Request.QueryString("emailAddr"))

        con.Open()
        cmd.ExecuteNonQuery()
    End Using                           
End Using 

TSQL:

 ALTER PROCEDURE [dbo].[web_Insert_usr_Comments] 
        @Location varchar(255), 
        @usrexp int = NULL,
        @full_name varchar(255) = NULL,
        @emailAddr varchar(320) = NULL,
        @comments varchar(125) = NULL

    AS
    BEGIN
        DECLARE @securityChk varchar(255),  @haveContact INT

        SET @securityChk = (SELECT @location + CAST(@usrexp AS VARCHAR(2)) + @full_name + @emailAddr + @usr_comments )  

        IF @securityChk LIKE '%SELECT%' RETURN;
        IF @securityChk LIKE '%DROP%'   RETURN;
        IF @securityChk LIKE '%INSERT%' RETURN;
        IF @securityChk LIKE '%DELETE%' RETURN;
        IF @securityChk LIKE '%EXE%'    RETURN;


    SET @haveContact =   ( SELECT  (count(*))
                FROM dbo.contacts 
                WHERE ( @emailAddr = E_mail_address AND  @location = company))
    --> Check and see if we have this person contact info                
    IF ( @haveContact = 0 )
       BEGIN 
        INSERT INTO dbo.contacts(e_mail_address, company, nick_name)
        VALUES (@emailAddr, @location, @full_name)
       END
    ELSE
        BEGIN
          INSERT INTO web_comments_dtl(timeGMT, form_id, contact_id, comment, user_exp_ranking, IP_Addr)
          SELECT GETDATE()
           , 1 --> TODO: Need better logic for form_id  
           , ( SELECT MAX(id) 
               FROM dbo.contacts
               WHERE  ( @emailAddr = E_mail_address AND  @location = company)
            OR ( @emailAddr = E_mail_address AND @full_name  = nick_name )
            OR ( @location = company))
          , @RFXcomments
          , @usrexp
          , dbo.fnBinaryIPv4(@ip_Addr)
        END
    END

What I would like to do, but not sure which option is the most efficient.

1. Should spend a week or two and investigate the security issues (if there is one) and the user input form that calls this SP
2. Rewrite the whole thing including the SP, code behind, and reconfigure IIS
3. Add more clauses to the statement above.

I don't think there is a security problem...but if the guy before decides to put this in his code--what do a young DBA is supposed to do.

Answer 1

Doing this sort of blacklisting of words in your arguments is unnecessary with properly implemented stored procedures, and will only lead to bad user experience in the event that this misfires on benign content.

If you are using a stored procedure and structuring your query with the parameters that are passed into the stored procedure, you do not need to do these checks.

If, however, you are concatenating strings together and passing them into an EXEC call, you are opening yourself up to sql injection. My advice is to simply use stored procedures for what they are meant to do, and rely on that for protection.

(As an aside - 25 characters for full name and email address seems a little too short to me.)

Answer 2

As other have pointed out, the SQL injection checks in this procedure are a bad idea. They fail to protect against SQL injection, and they do block some legitimate input.

But to your point:

[what is a young DBA supposed to do?]

Raise this issue with a senior DBA, or your manager. Express your concerns about the design of this procedure and other procedures like it.

Then suggest that query parameters would be a better solution (easier, more effective, industry standard, fewer chances for mistakenly blocking legitimate input). Suggest that the team should make it a coding standard to use query parameters as a means of defending against SQL injection.

For cases of dynamic content that cannot be inserted as a parameter (for example, a dynamic column name), use whitelisting instead of blacklisting.

The reason I say you need to bring this up to managers or team leads is that rewriting procedures and reconfiguring IIS will interrupt their current schedule for getting work done. As a junior member of the team, you may not be aware of some other project constraints or requirements.

The people who are responsible for the project schedule make the decision about how and when to rewrite everything. If you take it upon yourself to initiate that, you could cause trouble for them. Open communication is the best plan.

Answer 3

The proper way to prevent SQL injection is to never construct SQL queries by string manipulation of user-controlled input, but to simply use bound parameters.

That is in your application never do:

db.execute_sql("SELECT * FROM users WHERE user_name = '" + user_name + 
               "' AND password_hash = HASH('" + password + "');"
              )

where user_name could be a variable with a value like '; DROP TABLE users; -- (so the string to be executed by the database is now SELECT * FROM users WHERE user_name = ''; DROP TABLE users; -- which drops the users table and ignores everything after the comment symbol.

Instead do something like

db.execute_sql_with_params("SELECT * FROM users WHERE user_name = '?'" + 
                           " AND password_hash = HASH('?');", [user_name, password]);

where regardless of the values of the variables [user_name, password] the structure of the SQL statement will never change -- it will always search for a username with the contents of the user_name variable.

So yes, any sort of check for certain terms (that may be valid and will not fully capture all attacks) is a bad idea.

This is slightly different than saying only used stored procedures -- stored procedures -- are just stored SQL functions. While it would be dumb, a stored procedure could build an SQL query in an unsafe manner by manipulating strings of user input, just like how you could do it in your application code.

Answer 4

You should more or less handle this in application code rather than in the stored procedure. A user may have an email like drop_me_mail@somewhere.com.

Try using parameterized queries here.