-2

I have code that inputs data into db using OLEDB. Let's say it looks like this

var commandText = $"INSERT into {tableName}
                ({columnName1}, {columnName2})
                VALUES ({value1, value2});"
var command = new OleDbCommand(commandText, connection);
command.ExecuteNonQuery();

Suggestion is to use OLEDB parameters, something like this

var commandText = $"INSERT into {tableName}
                ([{columnName1}], [{columnName2}])
                VALUES (?, ?);"
var command = new OleDbCommand(commandText, connection);
command.Parameters.Add(new OleDbParameter(columnName1, value1));
command.Parameters.Add(new OleDbParameter(columnName2, value2));
command.ExecuteNonQuery();

What are the benefits of using parameters here?

Does it really improve security if the values are validated before?

Roman
  • 23
  • 5
  • `Does it really improve security if the values are validated before?` - ***yes***. because there will always be an attacker who's smarter than your validation (except for whitelisting). you should also really try to refactor your software so you don't have to inject table/column names that way. parameterised statements also potentially help improve performance and prevent _myriads_ of syntax errors. (always expect your users to be stupid and your attackers smart) – Franz Gleichmann Nov 22 '21 at 08:31
  • In short, yes. Connect to a SQL Server, set `value` to `1, 2); DROP DATABASE MyDb; --` and you've got a classic SQL injection attack, with many ways to rewrite it and escape detection. What's more, values like `NULL`, strings with quotes in them and date/time literals all require special attention when you're interpolating values. The idea of values being "validated" is all nice and good, but parameters form an absolute barrier to injection attacks, while validation does not. – Jeroen Mostert Nov 22 '21 at 08:31
  • Imagine you have the following query: `string query = "SELECT Id FROM Users WHERE Name = '" + userName + "' AND password = '" + password + "'";` and then someone inputs the username value `' OR Name = 'SysAdmin' ; -- comment`. The query that gets executed would be combined as this: `"SELECT Id FROM Users WHERE Name = '' OR Name = 'SysAdmin' ; -- comment' AND password = 'somevalue';`. Just like that, the user is logged in as the admin! Or worse, their "username" is `' ; DROP TABLE User; -- comment` and you just lost your users table. Validated, sure. Confident that you didn't miss something? – ProgrammingLlama Nov 22 '21 at 08:31
  • [some fundamental infos about SQL Injection attacks](https://bobby-tables.com/) – Franz Gleichmann Nov 22 '21 at 08:31
  • 1
    It's not just security. Parameters are strongly-typed binary values. You avoid the type conversion and localization issues in your code (what date format? what decimal separator?) *and* save space. Using parameters allows reusing execution plans which results in *significant* performance improvements, especially for simple frequently executed queries – Panagiotis Kanavos Nov 22 '21 at 08:49
  • For example, your code would break in most of the world if you passed decimal values - except perhaps China, Australia and the US which use `.` as the decimal separator. And even there the code could fail if the locale requires thousand separators – Panagiotis Kanavos Nov 22 '21 at 08:51

2 Answers2

1

With this code, no amount of parameters can help you.

The fact that you're concatenating the table name and the columns names makes your query vulnerable to SQL Injection attacks, which is the primary (but not only) reason to use parameters.

The way SQL injection works is by replacing parts of the string sent to the database with sql code. Parameters prevent that from happening because the database treats them as placeholders for data, which means it will not run any sql code that was passed through them.
However, since your table name and column names are also c# variables, they can be replaced with SQL code that the database will try to run.

Suppose the following code:

var tableName = "table (Col1) VALUES (null);DROP TABLE table;--";

// declare and populate columnName1, columnName2 with whatever values you want here, even just nulls

var commandText = $"INSERT into {tableName}
                ([{columnName1}], [{columnName2}])
                VALUES (?, ?);"
// run SQL command here

This will insert a single record into table, and then drop the table from the database.

For more information, read my blog post entitled Back to basics: SQL Injection

Zohar Peled
  • 79,642
  • 10
  • 69
  • 121
0

Actually, yes. We all make mistakes and the extra safety added by the parameter object is like a second set of eyes. Also, if you always use the parameter object, you run less of a risk in introducing errors down the line.

John
  • 819
  • 1
  • 9
  • 20
  • 1
    That's not it. No amount of manual checking and escaping can prevent the SQL injection problems, type conversion or localization errors that are eliminated by using parameters. Parameters improve performance by using cached execution plans too, which simply can't be done using dynamic SQL. When you pass a `DateTime` parameter value, you pass an unambiguous binary `DateTime` value, not a localized string. – Panagiotis Kanavos Nov 22 '21 at 08:46
  • @PanagiotisKanavos Thanks. You said it much better than I did but the point is still true. IMHO, never use SQL w/o the parameter object. – John Nov 22 '21 at 16:22