0

In some C# code, table names passed in via an enum are being inserted into SQL queries using string.Format like so:

const string ADD_SQL = "INSERT INTO {0} (ColumnOne) VALUES (@valueOne)";
const string CLEAR_SQL = "DELETE FROM {0}";

var commandText = string.Format(ADD_SQL , _tableName);

But when I run the Veracode tool it shows this query has possibility of SQL injection when executing.

command.ExecuteNonQuery();

I want to avoid the possibility of SQL injection with the above code. I tried adding a tag (@tablename), but that did not work.

const string ADD_SQL = "INSERT INTO @tablename (Data) VALUES (@valueOne)";
var commandText = ADD_MESSAGE_SQL.Replace("@tablename", _tableName);

How do I avoid this?

ErikE
  • 48,881
  • 23
  • 151
  • 196
LahiruD
  • 93
  • 1
  • 12
  • 1
    @DragandDrop why you suggesting duplicate about parameters when you probably know that table names can't be parametrized? – Alexei Levenkov Oct 18 '17 at 06:23
  • You should always use parameterized query, search on parameterized query for more detail – Jameel Hussain Oct 18 '17 at 06:24
  • 4
    Possible duplicate of [SqlParameter does not allows Table name - other options without sql injection attack?](https://stackoverflow.com/questions/17947736/sqlparameter-does-not-allows-table-name-other-options-without-sql-injection-at) – tia Oct 18 '17 at 06:27
  • @jimmi94 showing example would be much more useful than suggesting something that can't be done... https://stackoverflow.com/questions/3128582/table-name-and-table-field-on-sqlparameter-c is probably the best you can do for table names if you must have them inserted... – Alexei Levenkov Oct 18 '17 at 06:27
  • @AlexeiLevenkov, you are right. Flag Retracted as it's not a dupe. I read to fast, But as there is no proof of parameter beeing used, I will add the link as related. Related: [Executing query with parameters](https://stackoverflow.com/questions/11905185). – Drag and Drop Oct 18 '17 at 06:28
  • @DragandDrop Tables names are not known.They are randomly changing according to the several scenarios. – LahiruD Oct 18 '17 at 06:31
  • 2
    @LahiruD, `SELECT table_name FROM INFORMATION_SCHEMA.TABLES WHERE table_name=@Param` now you are pretty sure there is no sql injection. it's from a comment on @Tia dupe target. – Drag and Drop Oct 18 '17 at 06:34

2 Answers2

0

There is a good chance that Veracode does not like you putting SQL queries like your current statements, and instead wants to use it's prescribed way of writing this code. And as visible in the documentation in the Repair section, it wants you to use prepared statement to create a parameterized query.

It's upto your choice now. My take on this would be that stored procedures will be better, but if you have to keep query in C#, just don't try to make one query too generic for all scenarios and tables.

Amit Kumar Singh
  • 4,393
  • 2
  • 9
  • 22
0

Concatenating strings into SQL statements is risky if your strings comes from user input.
While that is not the case you described, I'm guessing the Veracode tool doesn't know where the strings come from, it just see string concatenation and issue a warning.

A better approach would be to write complete SQL statements for each table (Usually I prefer using stored procedures, but that's another topic [you can search for stored procedures vs inline SQL]) and use parameters for values (Identifiers can't be parameterized in SQL, as you already found out).

So instead of 

const string ADD_SQL = "INSERT INTO {0} (ColumnOne) VALUES (@valueOne)";
const string CLEAR_SQL = "DELETE FROM {0}";

and adding the table names at run time, here is a better solution:

const string ADD_tableName = "INSERT INTO TableName (ColumnOne) VALUES(@ValueOne)"
const string CLEAR_tableName = "DELETE FROM TableName";

There are even better solutions out there, but this is the easiest fix for the code you presented.

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