1

I'm having a problem. I want this to work, but it doesn't:

SqlDataSource.SelectCommand = "SELECT blah1, blah2 FROM myTable WHERE @ColumnName = @Value";

SqlDataSource.SelectParameters.Add("ColumnName", System.Data.DbType.String, "one");
SqlDataSource.SelectParameters.Add("Value", System.Data.DbType.String, "two");

It won't substitue the first paramter "ColumnName." If I remove that parameter and place the column name in it like this, it will work:

SqlDataSource.SelectCommand = "SELECT blah1, blah2 FROM myTable WHERE one = @Value";

SqlDataSource.SelectParameters.Add("Value", System.Data.DbType.String, "two");

I have a UI where the user can select the DB column name to search on. I want to protect myself from any sort of injection attacks. Any ideas how I can make this work?

One idea I read about was to use a look-up table to take the index from the DropDownList and pull column names that way. I could make that work, but I'd rather get parameterization working if possible since that seems more natural to me.

Thank you in advance for any help you can provide.

Mr. Spock
  • 645
  • 1
  • 9
  • 21
  • 1
    This simply isn't supported. You'll need to build the SQL text by hand, checking to make sure the column names are valid (perhaps using a `enum`).. – Mike Christensen Mar 14 '13 at 21:11
  • If this method works, then parameterised queries become unsafe. – Kaf Mar 14 '13 at 21:51

3 Answers3

2

Since query parameters are resolved after the SQL is parsed and an execution plan is generated, you can't actually dynamically build SQL with parameters. I would recommend building the SQL string itself, in a safe way of course. Perhaps first create an enum of valid column names:

enum DbColumns { One, Two, Three };

And then build the SQL string like so:

DbColumns colName = (DbColumns)Enum.Parse(typeof(DbColumns), "One");
SqlDataSource.SelectCommand = String.Format("SELECT blah1, blah1 FROM myTable WHERE {0} = @Value", colName);

Another idea would be to validate the column name using a regular expression, perhaps only allowing [a-z].

Mike Christensen
  • 88,082
  • 50
  • 208
  • 326
  • oh you put your answer too just at the moment i updated mine=) – vittore Mar 14 '13 at 21:26
  • Heh that's fine.. Both answers are for sure worth considering. – Mike Christensen Mar 14 '13 at 21:29
  • The solution I came up with was essentially the same - use the selected index of the dropdownlist as a look-up into the column names that were bound to the dropdownlist initially. If the index is not within bounds or of the right type (a.k.a not valid) I return no results. – Mr. Spock Mar 15 '13 at 03:23
1

I'm afraid you can't do that, what you can do instead is a little trick:

SELECT blah1, blah1 FROM myTable 
WHERE (@blah1 is null or blah1 = @blah1)
   or (@blah2 is null or blah2 = @blah2)

and provide all params @blah1, @blah2 but only assign those you need.

NB solution that Mike Christensen offering you is basically building string with right condition, which in simplest case would be

public bool BuildQueryWithCondition(string fieldName, string fieldValue) {
   var queryTemplate = "SELECT blah1, blah1 FROM myTable WHERE {0} = @Value"
     , query = string.Format(queryTemplate, fieldName)
   SqlDataSource.SelectCommand = query;
   SqlDataSource.SelectParameters.Add("Value", System.Data.DbType.String, fieldValue);
}
vittore
  • 17,449
  • 6
  • 44
  • 82
  • I'm kinda curious if this could have any major perf impacts or generate non-optimal execution plans. – Mike Christensen Mar 14 '13 at 21:14
  • Well as you potentially can execute query with any conditions listed it is not really. Moreover, I suspect that preparing one statement like that is better than executed unprepared statements with the only one condition specified. But you curiousity is totaly healthy, go ahead and run some tests! – vittore Mar 14 '13 at 21:22
  • The SQL parsing and compilation is gonna be really fast either way. I'm worried that it might touch a bunch of unnecessary indexes, or build giant bit-mapped indexes in memory or something. But yea, all speculation and I'd have to first have a good idea of the type of data involved, the size of the database, and all sorts of other variables not specified in the question. – Mike Christensen Mar 14 '13 at 21:28
  • That SQL is considered best practice for providing "All" value to users in reporting logic like crystal & SSRS, I'd recommend not speculating too much about it esp. if you can't discern why it would or wouldn't have performance problems. – RandomUs1r Mar 14 '13 at 21:29
  • Yeap, we actually hit exactly in the case you just described. but it is not problem of this approach, we just did have proper indexes on one table. – vittore Mar 14 '13 at 21:30
  • We use vittore's first approach and our general rule is that more than 5 or 10 of those branches will start to have a slight impact on performance, but it optimizes fairly well. I think it helps if you have it in a stored proc that will maintain its execution plan better than SQL text. Either way, if you are dynamically generating a SQL statement based on the the field of choice, you query will not be optimized well. – Chris Porter Mar 15 '13 at 05:20
0

I have figured out a way to include a work around for parametrized column names. I had the same problem but came up with a different way and since I would be the only one using the column names then I believe this is still a safe bet.

            String sqlcomm = "SELECT * FROM Asset WHERE " + assetColName + " = ";
            command.CommandText = sqlcomm + "$assetColValue";

            //command.CommandText = @"SELECT * FROM Asset WHERE $assetColName = '$assetColValue'";
            //command.Parameters.AddWithValue("$assetColName", assetColName);

            command.Parameters.AddWithValue("$assetColValue", assetColValue);

As you can see from the code above. I tried almost what you did which I then had to comment out. I then concatenated strings together and was able to use my paramterized column name and value which then the value is securely added. The column name however is not secured but this is a method that only I will be using so its still somewhat safe. You can add regular expressions if you want to be more secure but you get the idea of the fix.

Tyler Buchanan
  • 311
  • 1
  • 4
  • 18