Is there a SQL Injection risk with this query? If so, how can I avoid it?

Question

I usually create parameterized queries in order to avoid SQL Injection attacks. However, I have this particular situation where I haven't been totally able to do it:

public DataSet getLiveAccountingDSByParameterAndValue(string parameter, string value)
{
    string sql = "select table_ref as Source, method as Method, sip_code as Code, " +
        " from view_accountandmissed " +
        " where " + parameter + " like @value " +
        " order by time DESC ";
    MySqlCommand cmd = commonDA.createCommand(sql);
    cmd.Parameters.Add("@value", MySqlDbType.String);
    cmd.Parameters["@value"].Value = "%" + value + "%";

    MySqlDataAdapter objDA = commonDA.createDataAdapter(cmd);
    DataSet objDS = new DataSet();
    objDA.Fill(objDS);
    return objDS;
}

As you can see, I am creating @value as a parameter but if I tried to do the same with parameter the query would fail.

So, is there a risk of SQL Injection with this query? Also, take into account that parameter is set by a DropDownList's SelectedValue (not a TextBox, so the input is limited). If so, how can I improve this query?

There's definitely a risk. Imagine some malicious user modifies your input form via some browser dev tool and enters something like 1=1; DROP TABLE important_table; ... as parameter value... — Sergii Kudriavtsev
– Sergii Kudriavtsev, Commented Feb 9, 2012 at 10:01
With websites (as it's tagged asp.net) do not trust anything from a client as it can be modified with JS (including dropdown lists). — Stuart Blackler
– Stuart Blackler, Commented Feb 9, 2012 at 10:01
+1 for checking. There's definitely a SQL Injection risk with this. — Curtis
– Curtis, Commented Feb 9, 2012 at 10:02
ASP.NET checks if the items have changed when EnableEventValidation is set to true since 2.0. You would get an exception: "Invalid postback or callback argument". odetocode.com/blogs/scott/archive/2006/03/20/… — Tim Schmelter
– Tim Schmelter, Commented Feb 9, 2012 at 10:09

Peter · Accepted Answer · 2012-02-09 10:06:23Z

4

Yes there is:

" where " + parameter + " like @value " +

The value in parameter is your risk. In the postback you should check if the selected value is in the set of start values of the dropdown list.

Make the parameter an enum and pass the enum to your function. That will eliminate the risk (something like: not tested):

public DataSet getLiveAccountingDSByParameterAndValue(ParameterEnum parameter, string value)
.....
    " where " + parameter.ToString() + " like @value " +

The ParameterEnum contains a list of all possible values in your dropdown list. In your code behind, parse the selected value to the enum.

edited Feb 9, 2012 at 10:06

answered Feb 9, 2012 at 10:00

Peter

28k8 gold badges69 silver badges85 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Tim Schmelter Over a year ago

Since 2.0 ASP.NET checks if the items have changed when EnableEventValidation is set to true. You would get an exception: "Invalid postback or callback argument". odetocode.com/blogs/scott/archive/2006/03/20/…

Peter Over a year ago

You are correct, by default. But if you are someone else does change the default, you would have a risk.

Tim Schmelter Over a year ago

Yes, but i thought it is worth noting. Most times people complain about this feature and ask how to get rid of it, but here is a good example when it's useful.

m0skit0 · Accepted Answer · 2012-02-09 20:58:25Z

2

So, is there a risk of SQL Injection with this query?

I think yes, it's vulnerable to SQL injection. For example, parameter = "1=1 OR value"

Also, take into account that parameter is set by a DropDownList's SelectedValue (not a TextBox, so the input is limited)

Doesn't really matter. A malicious user can inject any value on the executable itself or on the network packet (and thus send a value that doesn't exist on the DropDown).

If so, how can I improve this query?

You should check parameter argument and compare with DropDown values. For more generic data, I think there should be libraries that check such things (but I have no C# idea...).

edited Feb 9, 2012 at 20:58

answered Feb 9, 2012 at 10:02

m0skit0

26k13 gold badges84 silver badges131 bronze badges

Comments

Adrian Iftode · Accepted Answer · 2012-02-09 10:13:19Z

1

var columns = new [] {"column1", column2", ....};
if (!columns.Contains(parameter))
   return or do something else

EDIT
The only SQL injection risk is by passing the column name in the where clause using string concatenation. There is no other way. The truly shield is to check that the column name is a valid one, it exists in the table.
Even ASP .Net has event validation (checks that the posted value is one of the dropdowns), you can't base on this since this protection can be disabled.
The parameter used with like is not object to SQL injection

edited Feb 9, 2012 at 10:13

answered Feb 9, 2012 at 10:02

Adrian Iftode

15.7k4 gold badges51 silver badges73 bronze badges

3 Comments

m0skit0 Over a year ago

How is this answering the question?

Adrian Iftode Over a year ago

the column names are wellknown so check that the parameter is a valid column name

m0skit0 Over a year ago

Still not good, because you might not want all columns to be accessed. And also you're using Contains, which allows stuff like "column1; DROP TABLE..."

Tim Schmelter · Accepted Answer · 2012-02-09 15:46:45Z

1

Since 2.0 ASP.NET automatically validates postback and callback arguments to see if they differ. So this is a good example when it's useful to EnableEventValidation.

http://odetocode.com/blogs/scott/archive/2006/03/20/asp-net-event-validation-and-invalid-callback-or-postback-argument.aspx

You'll get following exception then:

"Invalid postback or callback argument"

~~You could ensure that it's set to true by explicitely setting it in codebehind, for example in Page's Init event:~~

~~protected void Page_Init( object sender, EventArgs e ) { // don't remove this Page.EnableEventValidation = True; }~~

Edit: Oops, actually this setting cannot be changed from codebehind, it compiles but throws following runtime error:

The 'EnableEventValidation' property can only be set in the page directive or in the configuration section.

edited Feb 9, 2012 at 15:46

answered Feb 9, 2012 at 10:24

Tim Schmelter

462k79 gold badges719 silver badges980 bronze badges

3 Comments

aleafonso Over a year ago

+1 for making us aware of this. Ironically, I used to complain about this property too. Still, I have given the right answer to @peer because I might have disabled the page event validation in some particular cases

Tim Schmelter Over a year ago

@aleafonso: Yes, therefore I've showed how to ensure that it's set to true for this page and will not be changed(even when someone sets EnableEventValidation=false in the page directive or in web.config).

aleafonso Over a year ago

I wish I could mark both answers as correct since both of them are right. Thanks a lot

Collectives™ on Stack Overflow

Is there a SQL Injection risk with this query? If so, how can I avoid it?

4 Answers 4

3 Comments

Comments

3 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

3 Comments

Comments

3 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related