Improper Neutralization of special elements used in an sql command

Stephen Holdorf

This is very similar to a previous post but with different code. I have to eliminate a SQL injection error from within a method. Now, with only minor modifications this error must be eliminated. Here is the description from the scan: Attack vector: system_data.system.data.IDbCommand.ExecuteReader Description: The database query contains a sql injection flaw. The call to system_data_dll.System.Data.IDbCommand.ExecuteReader constructs a dynamic sql query using a variable derived from user-supplied input. An attacker could exploit this flaw to execute arbitrary sql queries against the database. ExecuteReader was called on the command object, which contains tainted data. The tainted data originated from earlier calls to system_data_dll.data.common.dbcommand.executereader, System_web_dll.system.web.httprequest.get_params, system_web_dll.data.common.dbadapter_fill, system_data_dll.system.data.common.dbwommand.executescarar and system_web_dll.system.web.httprequest.get_form Code:

    protected DataTable ExecuteDataTable(DbCommand command, ParamData\[\] pDataArr)
    {
        DataTable returnValue = null;
        try
        {

            if (\_connection == null)
                OpenConnection();
            else
            {
                if (\_connection.State == ConnectionState.Closed)
                    OpenConnection();
            }

            command.Connection = \_connection;
            command.CommandType = CommandType.Text;
            command.CommandTimeout = 12000;

            //add Parameter
            for (int i = 0; i < pDataArr.Length; i++)
            {
                DbParameter parameter = command.CreateParameter();
                parameter.ParameterName = pDataArr\[i\].pName;
                parameter.DbType = pDataArr\[i\].pDataType;
                parameter.Value = pDataArr\[i\].pValue;
                command.Parameters.Add(parameter);
            }

            // Create a DataTable
            returnValue = new DataTable();

            DbDataReader reader;
            reader = command.ExecuteReader();

            using (reader)
            {
                // Fill DataTable 
                returnValue.Load(reader, LoadOption.OverwriteChanges);
            }

            reader.Close();

            if (!KeepAlive && \_connection.State == ConnectionState.Open)
            {

Sascha Lefevre

I assume that it's the same thing as in your previous question. Though there are SQL-parameters used in this method, it gets its command-object passed as an argument with the command-text apparently already assigned. I guess the calling code concatenates some values (other than there are in pDataArr) as literals into the query string.

If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson

PIEBALDconsult

There is no substitute for doing it the right way.

Steve Holdorf

I saw a very good answer to my question about parameterizing a string with the actual parameters; how would I know whether the command was an UPDATE, INSERT, or a DELETE? SqlCommand cmd = new SqlCommand(commandText, connection); SqlParameterCollection sp = cmd.Parameters; List sp = new List() { new SqlParameter() {ParameterName = "@CmpyCode", SqlDbType = SqlDbType.NVarChar, Value= CV.Global.CMPYCODE}, new SqlParameter() {ParameterName = "@Code", SqlDbType = SqlDbType.NVarChar, Value = codeName}, new SqlParameter() {ParameterName = "@DisplayCode", SqlDbType = SqlDbType.NVarChar, Value = codeName + "-"}, new SqlParameter() {ParameterName = "@TotalDigit", SqlDbType = SqlDbType.Int, Value = CV.Global.PARAMTOTALDIGIT} }; insertData(CV.Sps.SP_INSERT_PARAM_TABLE, sp); SqlCommand cmd = new SqlCommand(); cmd.Parameters.AddRange(parameterPasses.ToArray());

Sascha Lefevre

Steve Holdorf wrote:

how would I know whether the command was an UPDATE, INSERT, or a DELETE?

I'm a bit confused - what is the context for this question? I don't see how it is related to your previous questions. And I don't see why you posted that code, which appears to be three separate fragments?

If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson

Steve Holdorf

OK. Let me explain. I have found some code that I think will work but how would I know what sql command it would be. See code below: SqlCommand cmd = new SqlCommand(commandText, connection); SqlParameterCollection sp = cmd.Parameters; List sp = new List() { new SqlParameter() {ParameterName = "@CmpyCode", SqlDbType = SqlDbType.NVarChar, Value= CV.Global.CMPYCODE}, new SqlParameter() {ParameterName = "@Code", SqlDbType = SqlDbType.NVarChar, Value = codeName}, new SqlParameter() {ParameterName = "@DisplayCode", SqlDbType = SqlDbType.NVarChar, Value = codeName + "-"}, new SqlParameter() {ParameterName = "@TotalDigit", SqlDbType = SqlDbType.Int, Value = CV.Global.PARAMTOTALDIGIT} }; insertData(CV.Sps.SP_INSERT_PARAM_TABLE, sp); SqlCommand cmd = new SqlCommand(); cmd.Parameters.AddRange(parameterPasses.ToArray());

Sascha Lefevre

You didn't explain a whole lot :-D Am I right in assuming that you want to change your code to use stored procedures instead of 'text-statements' (CommandType.Text) and in order to execute the right one, need to know if it should be an UPDATE, INSERT, or DELETE ?

If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson

Steve Holdorf

No. I still have to use the static command but eliminate the hard coded parameters. That being the case I want to use the technique above for passing in the parameters, use the loop, which is the problem to begin with, to fill in a parameterized string like so: command.CommandText = ("INSERT INTO TABLE (result, title, des) values(@store_result, @store_title, @store_des)");

Sascha Lefevre

I still can't follow you completely. You're omitting some steps of your train of thought :) The method in your original post takes a DbCommand and ParamData[], already has some kind of parameter-filling-loop and then runs an ExecuteReader(). The DbCommand apparently already has a SELECT-statement assigned and my assumption was that this statement already contains some values as literals, which is why the method failed your SQL-injection test, despite the rest of the values are added via parameters. As you're now quoting an INSERT-statement, you must be talking about some completely different method that I've not seen yet. Please post that method and also the calling code and maybe elaborate on why using a loop to create the parameters is a problem.

If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson

Steve Holdorf

Lefevre, You make a good point about me being confused. I really can't show you anything that doesn't require a loop. I have three other findings that are simple hard coded commands with parameters as part of the string. This is what I showed you in my first post that you answered before this one. In every case a loop is required to add the sql parameters to the command. I have been looking for some kind of Lambda expression to add the values to the sql command parameter list which I can not find.

Sascha Lefevre

How about we deal first with the method you posted in your original question? ;) To help you further with that, I would like to see the calling code - can you post that? /Sascha

If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson

Stephen Holdorf

I think I have a solution. Can someone review the solution and let me know what they think? The Singleton created with it's _id property that is passed in from the calling function: public class QueryContainer { private static List Container; private static QueryContainer instance; private int _id; public int _searchID; private string _query; private QueryContainer () { } public static QueryContainer Instance { get { if (Instance == null) { instance = new QueryContainer(); } return instance; } } public string Query { get { return Container.Find(instance => instance._id == _searchID).Query; } set { Container.Query = value; _id =+ 1; } } } } } public int ID { get { return _id; } } } The calling code that passes the id to access the query string from the singleton: protected object ExecuteScaler(int id) { object returnValue = null; Container Instance = new Container (); Instance.searchID = id; DbCommand command = _provider.CreateCommand(); command.Connection = _connection; command.CommandText = Instance.Query; command.CommandType = CommandType.Text; if (_useTransaction) { command.Transaction = _transaction; } try { returnValue = command.ExecuteScalar(); } ...

Sascha Lefevre

It's awful. It really is, sorry. - A singleton is completely unneccessary for what you're trying to do. - It's the worst possible implementation of the singleton pattern. - It won't compile because of the setter of the Query-property. - The getter of the Query-property will cause a stack overflow. - There's no need for a "Query-Container-List" for what you're trying to do. - It doesn't address the actual problem of SQL-injection / SQL-parameters at all. I suggested a way for further course of action in my last message - it's up to you :)

If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson