Validating user supplied input

Question

I was assigned a task to fix the SQL injection flaw reported by a code analysis tool. I am not the original author of the code. I am fairly knowledgeable with SQL.

 public int ExecuteNonQuery(string query, SqlParameter[] parameters)
 {
     using (SqlCommand command = CreateCommand(query, parameters))
     {
         int rowsAffected = command.ExecuteNonQuery();
         log.Debug("rows affected: {0}", rowsAffected);
         return rowsAffected;
     }
 }

and the tool has reported the injection flaw at the line number 3. The ExecuteNonQuery is being called from so many places.

Here is one such sample:

public void AddDataFrom(IWorkflowStepExecutionStorage storage, bool inverseSourceAndTargetSide)
{
    WorkflowStepExecutionStorage storageImpl = storage as WorkflowStepExecutionStorage;
    if (storageImpl == null)
    {
        throw new InvalidOperationException();
    }
    const string queryTemplate =
        @"insert into [$mappingDictionaryTable](sourceObjectId, sourceObject, targetObjectId, targetObject, linkedObjectTypesPairId)
          select sourceTable.objectId, sourceTable.systemEntry, targetTable.objectId, targetTable.systemEntry, @linkedObjectTypesPairId from
            [$mappingTable] mt
            inner join [$sourceImportStorageTable] sourceTable on mt.$sourceIdColumn = sourceTable.id
            inner join [$targetImportStorageTable] targetTable on mt.$targetIdColumn = targetTable.id
        ";
    QueryBuilder queryBuilder = new QueryBuilder(queryTemplate);
    queryBuilder.AddParameter("$mappingDictionaryTable", this.mappingDictionaryTable.TableName);
    queryBuilder.AddParameter("$mappingTable", storageImpl.MappedTable.TableName);
    queryBuilder.AddParameter("$sourceImportStorageTable", inverseSourceAndTargetSide ?
        (storageImpl.TargetImportStorage as ImportExecutionStorage).ImportTable.TableName :
        (storageImpl.SourceImportStorage as ImportExecutionStorage).ImportTable.TableName);
    queryBuilder.AddParameter("$targetImportStorageTable", inverseSourceAndTargetSide ?
        (storageImpl.SourceImportStorage as ImportExecutionStorage).ImportTable.TableName :
        (storageImpl.TargetImportStorage as ImportExecutionStorage).ImportTable.TableName);
    queryBuilder.AddParameter("$sourceIdColumn", inverseSourceAndTargetSide ? MappedTable.TargetIdColumn.Name : MappedTable.SourceIdColumn.Name);
    queryBuilder.AddParameter("$targetIdColumn", inverseSourceAndTargetSide ? MappedTable.SourceIdColumn.Name : MappedTable.TargetIdColumn.Name);

    string query = queryBuilder.GetParametrizedQuery();
    SqlParameter[] sqlParams = new[] { new SqlParameter("@linkedObjectTypesPairId", storageImpl.RunConfiguration.LinkedTypesPairConfiguration.Id) };

    using (IQcDbConnection dbConnection = this.connectionProvider.CreateDbConnection())
    {
        dbConnection.ExecuteNonQuery(query, sqlParams);
    }
}

Here is the CreateCommand code:

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;
}

Is there any injection risk in this code? If yes, how do I recognize it?

Answer 1

There's only so much a code analysis tool can see.

using (SqlCommand command = CreateCommand(query, parameters))

You're using a parameterized SqlCommand, and given the example you appear to have everything under your own control, strongly typed and all.

Depends who is calling this ExecuteNonQuery method, and where query is coming from.

It appears the query is coming from a QueryBuilder, out of a QueryTemplate.

The thing is, this QueryBuilder has been given the ability to perform replacement of "tokens" in the SQL command string, and it seems to be taking in a string for the replacement - if I were a code analysis tool, I'd raise a flag right there.

I don't see any apparent specific problem with the example usage you've supplied. But how the API is used is the responsibility of the programmer using it, and the API takes a SQL command string, and actually sends another to the server. The API could easily be misused to misplace these "tokens" and pass a user-supplied value as the replacement value, and bingo, you have SQL injection happening.

In other words, I think there is a potential SQL injection issue with the API, if it's misused. But if it's used everywhere the way you've shown, in my opinion you can put that warning onto the "false positives" pile.

Using an Object Relational Mapper, or any current technology for that matter (Entity Framework comes to mind), could significantly reduce all that boilerplate - and risk of misuse. But no API is shielded from carelessness.

---

I can't help but mention this one:

SqlCommand retVal = this.connection.CreateCommand();

retVal reads like VBA code from 1998. You're using readable identifiers everywhere else; would returnValue kill anyone? Personally I use result all the time. I can live with params, but seeing retVal in my code base would automatically trigger Ctrl+R,R and a new name.