CODEWITHSUNDEEP
c#mysqlsql-injection
Thiago Ruiz
Thiago Ruiz

Reputation: 162

This code for preventing MySQL injection is good?

Found this code for preventing some basic MySql injections using HTTPModules

public class SampleSqlInjectionScreeningModuleCS : IHttpModule
{
    //Defines the set of characters that will be checked.
    //You can add to this list, or remove items from this list, as appropriate for your site
    public static string[] blackList = {"--",";--",";","/*","*/","@@","@",
                                       "char","nchar","varchar","nvarchar",
                                       "alter","begin","cast","create","cursor","declare","delete","drop","end","exec","execute",
                                       "fetch","insert","kill","open",
                                       "select", "sys","sysobjects","syscolumns",
                                       "table","update"};

    public void Dispose()
    {
        //no-op 
    }

    //Tells ASP.NET that there is code to run during BeginRequest
    public void Init(HttpApplication app)
    {
        app.BeginRequest += new EventHandler(app_BeginRequest);
    }

    //For each incoming request, check the query-string, form and cookie values for suspicious values.
    void app_BeginRequest(object sender, EventArgs e)
    {
        HttpRequest Request = (sender as HttpApplication).Context.Request;

        foreach (string key in Request.QueryString)
            CheckInput(Request.QueryString[key]);
        foreach (string key in Request.Form)
            CheckInput(Request.Form[key]);
        foreach (string key in Request.Cookies)
            CheckInput(Request.Cookies[key].Value);
    }

    //The utility method that performs the blacklist comparisons
    //You can change the error handling, and error redirect location to whatever makes sense for your site.
    private void CheckInput(string parameter)
    {
        for (int i = 0; i < blackList.Length; i++)
        {
            if ((parameter.IndexOf(blackList[i], StringComparison.OrdinalIgnoreCase) >= 0))
            {
                //
                //Handle the discovery of suspicious Sql characters here
                //
                HttpContext.Current.Response.Redirect("~/About.aspx");  //generic error page on your site
            }
        }
    }

}

Is it a good code or do you think I need to add more things in the blacklist, or forget this and try another way to prevent injection?

Upvotes: 0

Views: 2218

Answers (4)

David Hoerster
David Hoerster

Reputation: 28701

Why perform string inspection when parameterized queries will do that work for you (and more)?

Use Parameters.Add() or Parameters.AddWithValue() on your SQL statements that you're issuing from code.

Upvotes: 4

David
David

Reputation: 73554

Blacklist approach to santizing/filtering data is never the best approach to santizing data. (Although it is appropriate in some cases depending on the trade-offs)

A simple explanation exists here: http://www.testingsecurity.com/whitelists_vs_blacklists

A Blacklist is testing a desired input against a list of negative input's. Basically you would compile a listing of all the negative or bad conditions, then verify that the input received is not one of the bad or negative conditions. A Whitelist is testing a desired input against a list of possible correct input's. To do this you would compile a list of all the good input values/conditions, then verify that the input received IS one of this correct conditions.

Which would you think is better? An attacker will use any means possible to gain access to your web based application. This includes trying all sorts of negative or bad conditions, various encoding methods, and appending malicious input data to valid data. Do you think you can think of every possible bad permutation that could occur? A Whitelist is the best way to validate input. You will know exacty what is desired and that there is not any bad types accepted. Typically the best way to create a whitelist is with the use of regular expression's. Using regular expressions is a great way to abstract the whitelisting, instead of manually listing every possible correct value.

You're better off using the standard, tried-and-true defenses: parameterized queries or parameterized stored procedures.

Upvotes: 3

mfanto
mfanto

Reputation: 14418

No, blacklisting doesn't work to stop SQL injection. See the OWASP page for methods of getting around blacklists. You should just use parameterized queries

Upvotes: 2

Alexei Levenkov
Alexei Levenkov

Reputation: 100527

No it is not good.

It will block valid inputs and in no way protects code that constructs queries from bad/invalid data.

Just construct queries correctly assuming incoming data is bad and you'll be much better off.

Upvotes: 2

Related Questions