Can I still use parameters for sql command?

Question

Im trying to make a generic code for building Insert/Update query. So far i have only created the Update query, but i'mhaving doubts considering SQL injection.

My primary target is trying to create code to decrease the time of retyping the same code over and over.

public SqlConnection SqlConn;
private SqlCommand SqlComm;

public void UpdateRow(string TableName, string UpdateCondition, List<KeyValuePair<string, string>> FieldAndValueList)
{
  SqlOpen();
  try
  {
    string UpdateString = $"UPDATE {TableName} ";
    int counter = 0;
    foreach (KeyValuePair<string, string> FieldAndValue in FieldAndValueList)
    {
      if (counter > 0) { UpdateString += "," };
      UpdateString += $"SET {FieldAndValue.Key} = {FieldAndValue.Value} ";
      Counter += 1;
    }
    if (UpdateCondition.Trim() != "") { UpdateString += $"WHERE {UpdateCondition};"; }
    SqlComm = SqlConn.CreateCommand();
    SqlComm.CommandText = UpdateString;
    SqlComm.ExecuteNonQuery;
  }
  catch { ShowError(); }
  finally { SqlClose(); } 
}

It would then be executed like so:

List<KeyValuePair<string, string>> UpdateValues = new List<KeyValuePair<string, string>>;
UpdateValues.Add(new KeyValuePair<string, string>("age", txtAge.text)); 
UpdateRow("user", "user_id = X", UpdateValues);

Im trying to create it so SQL injection is not possible.

Your code is not safe. Use Parameters, see https://stackoverflow.com/questions/4892166/how-does-sqlparameter-prevent-sql-injection — Ian Mercer, Jun 07 '19 at 08:23
`List> == {"myField", "123; delete from table myTable; --"}` The text created will be `update SomeTable set myField = 123; delete from table myTable; -- UpdateCondition`, you'll update `SomeTable` and clear `MyTable` — Dmitry Bychenko, Jun 07 '19 at 08:26
@Ian, I already had my doubt about it not being safe. But i cant find out how to actually make it safe. This method makes the size of the update statement variable Im looking for a way to handle parameters while keeping the variable size of the update statement. — Roberto, Jun 07 '19 at 08:27
@Roberto: where is the *user input* (i.e. arbitrary, potentially malicious strings) in the routine? Is it `List>` only? — Dmitry Bychenko, Jun 07 '19 at 08:29
The name/values is one thing - that is solvable; however, your `UpdateCondition` is just ... no, you can't do that safely, ever — Marc Gravell, Jun 07 '19 at 08:29
The UpdateCondition would be hardcoded from the actual form. Only the List would be altered by UseInput. (or at least the values will be). The size of list will change depending of form. — Roberto, Jun 07 '19 at 08:30
additional note: `FieldAndValueList` as a `` is *also* probably a very bad idea, unless all the values **are strings**. That isn't the way to do data, basically. If the value is a `float`, **send it as a `float`** - otherwise, you can get into a *lot* of problems with conversions, including things like date formats, and different (international) representations of numbers (commas vs periods, etc) — Marc Gravell, Jun 07 '19 at 08:33
The application I was working on, was only using int, string and bool as input values. — Roberto, Jun 07 '19 at 08:42

score 3 · Accepted Answer · answered Jun 07 '19 at 08:28

3

I'm trying to create it so SQL injection is not possible.

Then you must use parameters; for the individual fields, something like the following would work:

if (counter > 0) { sb.Append(","); }
var pName = "@p" + counter.ToString();
sb.Append("[").Append(FieldAndValue.Key).Append("]=").Append(pName);
cmd.Parameters.AddWithValue(pName, ((object)FieldAndValue.Value) ?? DBNull.Value);
Counter += 1;

where sb is a StringBuilder (to avoid too many string concatenations)

However! Your planned string UpdateCondition is inherently not solvable. That cannot be made injection-safe. You need to think of a better way of doing that.

Or, better: use any off-the-shelf ORM.

answered Jun 07 '19 at 08:28

Marc Gravell

1,026,079
266
2,566
2,900

Hallo Marc, Ill try this. As for the Condition. Its hardcoded from the form that actually calls the function. So no user input. – Roberto Jun 07 '19 at 08:32
2

@Roberto just to emphasize: the last line in the answer is my strong recommendation; IMO you're going to get yourself into a lot of nasty holes trying to re-invent this stuff – Marc Gravell Jun 07 '19 at 08:34
Guess i was looking from the wrong angle as i couldnt find that. But it's more that im trying to get back into C# (havent done anything for 2y as i have been doing Delphi). But i'll have a look into it. – Roberto Jun 07 '19 at 08:41

Can I still use parameters for sql command?

1 Answers1