Take the 2-minute tour ×
Information Security Stack Exchange is a question and answer site for Information security professionals. It's 100% free, no registration required.

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.

share|improve this question

migrated from stackoverflow.com Jan 10 at 13:20

This question came from our site for professional and enthusiast programmers.

8  
What is this check for? Are you concatenating the values in @securityChk into a string and EXEC-ing this? If so that won't be sufficient to prevent SQL injection. Just use parameterised queries. Also what if my surname is "wardrop"? Will this stop me registering for your service? –  Martin Smith Dec 23 '13 at 17:15
    
@MartinSmith that's what I was thinking..this is being used for SQL injection checks....but is this good enough or am I'm checking everything I need to check at the beginning of my code? –  Chad Sellers Dec 23 '13 at 17:31
4  
Don't bother. Just parameterise your queries and be done with it. There is absolutely no need to do this if your insert statement is of the lines INSERT INTO T VALUES (@p1,@p2,@p3) - as long as the rest of the app also uses parameterised queries correctly. –  Martin Smith Dec 23 '13 at 17:34
    
Thats's all this app is doing and noting special just inserts....Just not sure if I was overlooking something. Thank's @MartinSmith –  Chad Sellers Dec 23 '13 at 17:40

4 Answers 4

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

share|improve this answer

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.

share|improve this answer
    
I'll add to that a little explanations on why blacklists won't work: there's plenty of attacks which don't require those keywords, example: websec.files.wordpress.com/2010/11/sqli2.pdf In many cases, it'd be hard to even whitelist the input. Parametrized SProcs are much better. –  DarkWanderer Dec 23 '13 at 18:45
    
Thanks Bill..I have to do my research some more on this topic and choose the right approach..It's plus that I'm part of all decision making when it's just you and your buddy starting a company. –  Chad Sellers Dec 23 '13 at 19:22

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.

share|improve this answer
    
So how can I fix my query and vb script above...to prevent the SQL injections attacks you mention in this post... –  Chad Sellers Jan 14 at 18:03
    
@ChadSellers - I'm not familiar with ASP/VB and don't use a microsoft OS. However, it appears you already are using bound parameters (aka parameterized queries). The important fact is when you talk to the database (cmd.ExecuteNonQuery()) that you keep the queryString / CommandText (set by your application) separate from the parameters (potentially with unsafe user input). By doing it as Using cmd As New SqlCommand(command, con) where command is a simple string (with no user input), and then doing cmd.Parameters.AddWithValue("@Location", Request.QueryString("Location")) you are fine. –  dr jimbob Jan 14 at 19:32
    
@ChadSellers - It may be helpful to contrast this with the wrong way to do it would be something like Dim command = "Execute dbo.web_Insert_usr_Comments " & Request.QueryString("Location"); where you in the SQL string fed into the SqlCommand you mix your logic ("execute dbo.web ...") with the user-provided values of the parameters. See also, Polynomial's excellent introduction to SQLi: security.stackexchange.com/a/25710/2568 –  dr jimbob Jan 14 at 19:36
    
@ChadSellers - Granted you don't provide how the SQL statement is ultimately run in the Stored Procedure. As long as its not made by string processing like in the paper you linked to (where you declare a string; and then add on user input to that string, and then run EXEC(@SQL) to that string. Instead you simply have a call like INSERT INTO your_db_table("Location", "Full name") VALUES (@location, @full_name). –  dr jimbob Jan 14 at 19:49

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.

share|improve this answer
    
Which he actually is by default since it's a stored procedure. –  Andrew Barber Dec 23 '13 at 18:25
1  
Homer voice: DOH! –  danish Dec 23 '13 at 21:22
    
hehehe.... bwahaha! :) –  Andrew Barber Dec 23 '13 at 21:22

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.