Parameterized SQL Injection in Fortify C#

Question

If I execute the following SQL Command I get a Fortify finding with SQL Injection (Notice the AddCommand method accepts a string)

internal void AddCommand()
{
    string cmtTxt = "SELECT * from Users WHERE @ID = 1";
    if (m_dbCon != null && !string.IsNullOrEmpty(cmdTxt))
    {
        m_dbCmd = new SqlCommand(cmdTxt, m_dbCon);
    }
}

If I execute the following method the SQL Injection finding goes away. What's the difference between passing the SQL string in as a parameter vs. a hardcoded string?

internal void AddCommand()
{
    if (m_dbCon != null)
    {
        m_dbCmd = new SqlCommand("SELECT * from Users WHERE @ID = 1", m_dbCon);
    }
}

I build up the parameters like so:

    internal void AddCmdParam(string param, string value)
    {
        if (m_dbCmd != null && !string.IsNullOrEmpty(param) && value != null && Utilities.ValidParameter(param))
        {
            m_dbCmd.Parameters.Clear();
            m_dbCmd.Parameters.Add(param, SqlDbType.NVarChar, 100);
            m_dbCmd.Parameters[param].Value = value;
        }
    }

my guess is probably because 'Fortify' can't verify the text in the 'cmdTxt' variable whereas it can in the hardcoded string. — GreatAndPowerfulOz, Oct 26 '15 at 20:16
based on if you are passing the `cmdTxt` in the top it's appears that it can't resolve `1` as an Integer where as in the second one it knows that the value is a `1` and not `"1"` if you are using the `SqlCommand` object the you should be able to use the `m_dbCmd.Parameters.Add || .AddWithValue` function to utilize parameterized query — MethodMan, Oct 26 '15 at 20:17
Do you beleive this to be a false positive as both are parameratized SQL? — codeg, Oct 26 '15 at 20:18
it's not Parameterized query.. you are nowhere using the `SqlCommand.Parameter` anywhere.. perhaps you need to google `MSDN SqlCommand.Parameters` method. — MethodMan, Oct 26 '15 at 20:20
Because in the first one you are passing in a string and then executing it. That is the definition of how the vulnerability works. — Sean Lange, Oct 26 '15 at 20:20
I've added my method to the question on how I add the parameters. — codeg, Oct 26 '15 at 20:22
try adding to this line ` m_dbCmd.Parameters.Add(param, SqlDbType.NVarChar, 100);` the actual value at the end for example `m_dbCmd.Parameters.Add(@param, SqlDbType.NVarChar, 100, param);` — MethodMan, Oct 26 '15 at 20:23
I tried the suggestion but it still comes up as SQL Injection. — codeg, Oct 26 '15 at 20:43
Why not separate your data layer completely? Use stored procedures and get this type of sql out of the application layer. Sounds like you are trying to build a generic method to handle all data requests. This is kind of like building one method to do everything in your application. — Sean Lange, Oct 26 '15 at 21:07
There is one method to handle all table reads. I could look into converting it into a stored procedure but it's seems there should be a way to mitigate sql injection via validation, paramartized sql, and whitelisting of parameters, as the sql being built up is hardcoded in each form codebehind, each with a single parameter. — codeg, Oct 26 '15 at 21:11

score 1 · Accepted Answer · answered Oct 26 '15 at 20:26

1

Very simply, the first piece of code is the definition of a SQL injection vulnerability. You are taking an unknown string and executing it. For example, lets say the parameter contained the string DROP DATABASE yourDb;. Executing that would be bad.

The second is not because the standard library escapes any strings you pass in using parameters so that it would not allow a SQL injection attack to happen.

answered Oct 26 '15 at 20:26

Becuzz

6,846
26
39

With that said, it means I cannot call a single method to handle all SQL Commands in hundreds of places throughout my codebase. Shouldn't this line: m_dbCmd.Parameters.Add(@param, SqlDbType.NVarChar, 100, param); esentially do the same thing? – codeg Oct 26 '15 at 20:36
@larryb The parameters thing changes how user inputted strings get added to the SQL statement. Check [this SO answer](http://stackoverflow.com/a/33033576/550062) for a better example of why the parameters fix injection attacks. I'm guessing that Fortify is looking at an unknown string and saying it could contain malicious code (imagine if you built up a SQL string yourself and just blindly threw user input in there). That is why it complains, because it could be bad and it can't verify that it is ok. Parameterizing solves that and makes Fortify happy. – Becuzz Oct 26 '15 at 20:51

Parameterized SQL Injection in Fortify C#

1 Answers1