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.


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

    End Using                           
End Using 


 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

        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 )
        INSERT INTO dbo.contacts(e_mail_address, company, nick_name)
        VALUES (@emailAddr, @location, @full_name)
          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)

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.

  • 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? Commented Dec 23, 2013 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
    Commented Dec 23, 2013 at 17:31
  • 5
    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. Commented Dec 23, 2013 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
    Commented Dec 23, 2013 at 17:40
  • bobby-tables.com now has a page for avoiding SQL injection in ADO.NET.
    – Zev Spitz
    Commented Sep 16, 2018 at 8:06

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


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.

  • 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. Commented Dec 23, 2013 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
    Commented Dec 23, 2013 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.

  • So how can I fix my query and vb script above...to prevent the SQL injections attacks you mention in this post...
    – AAH-Shoot
    Commented Jan 14, 2014 at 18:03
  • 1
    @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
    Commented Jan 14, 2014 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
    Commented Jan 14, 2014 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
    Commented Jan 14, 2014 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 [email protected].

Try using parameterized queries here.

  • Which he actually is by default since it's a stored procedure. Commented Dec 23, 2013 at 18:25
  • 1
    Homer voice: DOH!
    – danish
    Commented Dec 23, 2013 at 21:22
  • hehehe.... bwahaha! :) Commented Dec 23, 2013 at 21:22

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .