SQL Injection flaw

Question

I am working on a project where the client has reported an SQL injection flaw in the code. Here is my code…

1 public int ExecuteNonQuery(string query, SqlParameter[] parameters)
2 {
3   using (SqlCommand command = CreateCommand(query, parameters))
4   {
5       int rowsAffected = command.ExecuteNonQuery();
6       return rowsAffected;
7   }
8 }

And the CreateCommand method goes as

private SqlCommand CreateCommand(string commandText, SqlParameter[] parameters)
{
    SqlCommand retVal = this.connection.CreateCommand();
    retVal.CommandText = commandText;
    retVal.CommandTimeout = this.commandsTimeout;
    retVal.Parameters.AddRange(parameters);
    return retVal;
}

The flaw is reported at line number 3. I am unable to understand what kind of attack an happen here as this is a console application. But I have to fix the flaw and I don't know how to fix it.

Query is

@"delete from {0} where runId in 
( select runId from {0}
  inner join 
    ( select sId as sId_last,
        wfId as wfId_last,
        max(runId) as runId_last from {0} where endTime is NULL
        group by sId, wfId ) t1
  on endTime is NULL and sId = sId_last and wfId = wfId_last
  and (runId <> runId_last or startTime < @aDateTime)
)";

Help appreciated. Thanks.

Answer 1

that code is injection-free... But note that the methods that call ExecuteNonQuery could build the query by composing strings.

An injection attack happens when you do something like:

string name = ...; // A name selected by the user.
string query = "SELECT * FROM MyTable WHERE Name = '" + name + "'";

so when you compose a query using pieces of text that are of external origin.

Note that a more subtle injection attack could be multi-level:

string name = // The result of a query to the db that retrieves some data
              // sadly this data has been manipulated by the attacker

string query = "SELECT * FROM MyTable WHERE Name = '" + name + "'";

In general you don't need a user interface to cause an injection attack...

You could query something from a web site/from the db, and use the unsanitized result to query the db (as in the last example), causing an injection attack... Or even using the content of the configuration file could cause an injection attack: the priviledges needed to modify the configuration file could be different than the ones needed to do something on the DB, and a malicious user could have the priviledges to modify the configuration file but not have direct access to the DB. So he could use the program as a trojan horse against the DB.

about the query

The weak point of that query (that is a composition of strings) is in how the {0} is calculated. Is it a string chosen in a group of fixed strings? Something like:

string tableName;

if (foo)
   tableName = "Foo";
else if (bar)
   tableName = "Bar";

or is it something more user controlled?

If the table names are fixed in code, then there shouldn't be any injection attack possible. If the table names are "extracted" from some user input/some other table the user could have access, we return to the problem I showed before.

Answer 2

You've exposed a public method which can be accessed by any code that allows any SQL expression to be executed.

I would look at changing that method to being internal or private instead so that not just any code can call that method.