Dynamically created SQL vs Parameters in SQL Server

If I were to select a row from a table I basically have two options, either like this

int key = some_number_derived_from_a_dropdown_or_whatever
SqlCommand cmd = new SqlCommand("select * from table where primary_key = " + key.ToString());

or use a parameter

SqlCommand cmd = new SqlCommand("select * from table where primary_key = @pk");
SqlPara开发者_开发百科meter param  = new SqlParameter();
param.ParameterName = "@pk";
param.Value         = some_number_derived_from_a_dropdown_or_whatever;
cmd.Parameters.Add(param);

Now, I know the first method is frowned upon because of possible sql injection attacks, but in this case the parameter is a integer and thus should not really be possible to inject malicious code.

My question is this: Do you use option 1 in production code because you consider the use safe because of ease of use and control over the inserted parameter (like the above, or if the parameter is created in code)? Or do you always use parameters no matter what? Are parameters 100% injection safe ?

I'll skip the SQL Injection argument, that is just too well known and just focus on the SQL aspect of parameters vs. non parameters.

When you send a SQL batch to the server, any batch, it has to be parsed to be comprehended. Like any other compiler, the SQL compiler has to produce an AST from the text and then operate on the syntax tree. Ultimately the optimizer transforms the syntax tree into an execution tree and finally produces an execution plan and that is actually run. Back in the dark ages of circa 1995 it made a difference if the batch was an Ad-Hoc query or a stored procedure, but today it makes absolutely none, they all the same.

Now where parameters make a difference is that a client that sends a query like select * from table where primary_key = @pk will send exactly the same SQL text every time, no matter what value is interested in. What happens then is that the entire process I described above is short-circuited. SQL will search in memory an execution plan for the raw, unparsed, text it received (based on a hash digest of the input) and, if found, will execute that plan. That means no parsing, no optimization, nothing, the batch goes straight into execution. On OLTP systems that run hundreds and thousands of small requests every second, this fast path makes a huge performance difference.

If you send the query in the form select * from table where primary_key = 1 then SQL will have to at least parse it to understand what is inside the text, since the text is likely a new one, different from any previous batch it seen (even a single character like 1 vs. 2 makes the entire batch different). It will then operate on the resulted syntax tree and attempt a process called Simple Parameterisation. If the query can be auto-paremeterized, then SQL will likely find a cached execution plan for it from other queries that run previously with other pk values and reuse that plan, so at least your query does not need to be optimized and you skip the step of generating an actual execution plan. But by no mean did you achieve the complete short-circuit, the shortest possible path you achieve with a true client parameterized query.

You can look into the SQL Server, SQL Statistics Object performance counter of your server. The counter Auto-Param Attempts/sec will show many times per second SQL has to translate a query received without parameters into an auto-parameterized one. Each attempt can be avoided if you properly parameterize the query in the client. If you also have a high number of Failed Auto-Params/sec is even worse, it means the queries are going the full cycle of optimization and execution plan generation.

Always use option 2).

The first one is NOT safe regarding SQL Injection attacks.

The second one is not only far safer, but will also perform better because the query optimizer has better chances to create a good query plan for it because the query looks the same all the time, expect the parameters of course.

Why should you avoid Option 1

Even though it seems you are getting this value from select element, someone might fake an HTTP request and post whatever they want inside certain fields. Your code in option 1 should at least replace some dangerous characters/combinations (ie. single quotes, square brackets etc.).

Why are you encouraged to use Option 2

SQL query engine is able of caching execution plan and managing statistics for various queries thrown at it. So having unified queries (the best one would of course be to have a separate stored procedure) speeds up execution in the long run.

I have yet to hear of any example where parameters can be hijacked for SQL injection. Until I see it proven otherwise, I'd consider them safe.

Dynamic SQL should never be considered safe. Even with "trusted" input, it's a bad habit to get into. Note that this includes dynamic SQL in stored procedures; SQL injection can occur there as well.

Because you have control that the value is an integer, they are pretty much equivalent. I generally do not use the first form, and typically, I don't allow the second either, because I usually don't allow table access and require use of stored procedures. And although you can do SP execution without using the parameters collection, I still recommend using SPs AND parameters:

-- Not vulnerable to injection as long as you trust int and int.ToString()
int key = some_number_derived_from_a_dropdown_or_whatever ;
SqlCommand cmd = new SqlCommand("EXEC sp_to_retrieve_row " + key.ToString()); 

-- Vulnerable to injection all of a sudden
string key = some_number_derived_from_a_dropdown_or_whatever ;
SqlCommand cmd = new SqlCommand("EXEC sp_to_retrieve_row " + key.ToString()); 

Note that although option 1 is safe in your case, what happens when someone sees and uses that technique with a non-integer variable - now you are training people to use a technique which could be open to injection.

Note that you can proactively harden your database to avoid injection being effective even when your application code is faulty. For accounts/roles that applications use:

• Only allow access to tables as absolutely necessary
• Only allow access to views as absolutely necessary
• Don't allow DDL statements
• Allow execute to SPs on role basis
• Any dynamic SQL in SPs should be reviewed as absolutely necessary

Personally, I would always use option 2 as a matter of habit. That being said:

Since you are forcing the conversion from the drop down value to an int, that will provide some protection against sql injection attacks. It someone tries to inject additional information into the posted information, C# will throw an exception when trying to convert the malicious code into an integer value thwarting the attempt.