CODEWITHSUNDEEP
c#.netsqlado.net
cdub
cdub

Reputation: 25741

SQL Injection Prevention in .NET

I typically write my SQL as so in .NET

sql.Append("SELECT id, code, email FROM mytable WHERE variable = @variable ");

Then do something like this:

using (SqlConnection conn = new SqlConnection(ConfigurationManager.ConnectionStrings[ConfigurationManager.AppSettings["defaultConnection"]].ConnectionString))
{
    using (SqlCommand myCommand = new SqlCommand(sql.ToString(), conn))
    {
        myCommand.Parameters.AddWithValue("@variable", myVariableName");
        ...

But should I also do this addParameter when the data I got comes directly from the database like so?

likesql.Append(string.Format("SELECT group_id, like_text FROM likeTerms ORDER BY group_id ASC "));

DataTable dtLike = SqlHelper.GetDataTable(likesql.ToString());

foreach (DataRow dr in dtLike)
{
    buildsql.Append(".... varId = " + dr["group_id"].ToString() + "...");

    ...

Is this acceptable? What is best practice?

Upvotes: 6

Views: 3234

Answers (4)

KeithS
KeithS

Reputation: 71591

When you use DbCommands with parameters, the parameters are never "inlined" into the query. Instead, the query and the parameter data are passed to a special system stored procedure, sp_executesql. When it's done this way, whatever parameter data you have is treated as exactly that, and isn't parsed out of the query string; therefore, an injected command that may have gotten past your validation is never executed.

Using parameters is the best practice for ADO.NET-based data access layers, no matter where the data comes from, and is IMO the only way it should ever be done at this level (if you don't use an ORM). You should NEVER concatenate values you retrieve from a web or windows form into a SQL statement, and if you're follow that rule, why would you implement it any differently just because you're confident or even sure that the information isn't directly from the user? Follow the same pattern, and if and when you expose that method to persist user-specified data you won't get burned.

Upvotes: 5

Tass
Tass

Reputation: 1248

You should always use parameters:

  • Where are the values in your database coming from?
  • Can you trust, in your example, that 'group_id' wasn't modified to be something you're not expecting?

Trust noone

Can someone with limited database access inject directly into a field used elsewhere?

Performance

Also, it helps performance. Cached execution plans will disregard the value of the parameter, meaning you're saving the server from recompiling the query every time the parameters change.

Upvotes: 17

wandos
wandos

Reputation: 1619

i recommend using stored procedure, but this is acceptable i also recommend that you sanitize your parameters before assigning them to the query

Upvotes: 0

Alexei Levenkov
Alexei Levenkov

Reputation: 100600

It is acceptable (in sense if you know restrictions your may sometimes work correctly).

Is it good practice to use values from database and build SQL query with string concatenation - no.

I.e. in your sample what if "group_id" is "'--"?

Upvotes: 4

Related Questions