locked
SQL Injection error. RRS feed

  • Question

  • User93267240 posted

    I have to eliminate a SQL injection error from within a method. What the code is doing is passing in a SQL querystring as the  command to a DbCommand object, see the code below. Now, with only minor modifications this error must be eliminated. Here is the description from the scan:

    This database query contains a sql injection flaw. the call to system_data_dll.Data.IDbCommand.ExecuteNonQuery constructs a dynamic sql queryusing a variable derived from the user-supplied input.An attacker could exploit this flaw to execute arbitrary sql queries against the database ExecuteNonQuery was called on the command object, which contains tainted data. The tainted data originated from from earlier calls to system_data.system.data.common..dbconnand.execurereader, system_web_dll.wweb.httprequest.get_params, system_data_dll.system.data.system.data.common.dbaadapter.fill.

    Below is the actual function code:

            protected object ExecuteScaler(string queryString)
            {
                object returnValue = null;

                if (!_iserror)
                {
                    if (_trace)
                    { DoTrace("TAMIS.Data.Loader.ExecuteScalar", queryString); }

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

                    DbCommand command = _provider.CreateCommand();
                    command.Connection = _connection;
                    command.CommandText = queryString;
                    command.CommandType = CommandType.Text;
                    if (_useTransaction) { command.Transaction = _transaction; }

                    try
                    {
                        returnValue = command.ExecuteScalar();
                    }
                    catch (Exception ex)
                    {
                        if (ex is EntryPointNotFoundException)
                            throw ex;
                        //if (_useTransaction == true)
                        //_transaction.Rollback();
                        RollBack();

                        LogBLL bll = new LogBLL();
                        bll.WriteErrorLog(ex);

                        _iserror = true;
                    }
                    finally
                    {

                        if ((!KeepAlive && _connection.State == ConnectionState.Open) || _iserror == true)
                        {
                            CloseConnection();
                        }

                    }
                }
                else
                {
                    returnValue = -1;
                }


                return returnValue;
            }

    Thanks in advance for all of your help!

    Tuesday, May 12, 2015 3:45 PM

Answers

  • User-718146471 posted

    Well, you are not using directly called variables and instead are parameterizing the query as far as I can determine; I think you have your solution.  However, I would wait for a few others to weigh in on this just to be sure.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Friday, May 15, 2015 11:19 AM
  • User-821857111 posted

    Whatever tool you are using to scan for security holes is not very good. The error it reports is nonsense, I'm afraid. Ignore the warning, or talk to the vendor of the tool. Your current approach is not susceptible to SQL injection.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Thursday, May 21, 2015 4:37 PM

All replies

  • User-718146471 posted

    Your command here should be using a parameterized query structure for one.  Second, you need to be using stored procedures.  Let me get an example of how this can be re-written that way.  The source of your troubles is here:

                    DbCommand command = _provider.CreateCommand();
                     command.Connection = _connection;
                     command.CommandText = queryString;
                     command.CommandType = CommandType.Text;

    Tuesday, May 12, 2015 4:06 PM
  • User-718146471 posted

    Ok, your code should be more like this:

    command.CommandType = CommandType.StoredProcedure;
    command.CommandText = ("insert_questions") ;
    command.Parameters.AddWithValue("@value", valueHere);
    command.Parameters.AddWithValue("@value2", valueHere);
    Tuesday, May 12, 2015 4:08 PM
  • User-718146471 posted

    Or you can always do this as well:

            command.CommandText = ("INSERT INTO TABLE (result, title, des) values(@store_result, @store_title, @store_des)");
            command.Parameters.AddWithValue("@store_result", store_result);
            command.Parameters.AddWithValue("@store_title", store_title);
            command.Parameters.AddWithValue("@store_des", store_des);

    Tuesday, May 12, 2015 4:10 PM
  • User93267240 posted

    My problem is how would you know if the command were an INSERT, UPDATE, or DELETE and how many parameter were being passed. Would you iterate through a parameters collection like SqlParameterCollection and just add another parameter through each iteration?

    Tuesday, May 12, 2015 6:27 PM
  • User281315223 posted

    My problem is how would you know if the command were an INSERT, UPDATE, or DELETE and how many parameter were being passed.

    There really isn't any way to know without actually taking a look at the stored procedure itself. I believe there is a way to pull out all of the parameters that are used within one as mentioned in this related discussion :

    SqlCommandBuilder.DeriveParameters(yourStoredProcedureCommand);
    foreach (SqlParameter p in yourStoredProcedureCommand.Parameters)
    {
           // Access each parameter here
    }

    However, if you are using some type of generic approach and are passing in the name of the procedures dynamically, it could be a bit tough and I suppose you would need to be aware when calling them to pass in the appropriate parameters.

    Tuesday, May 12, 2015 6:49 PM
  • User93267240 posted

    Rion,

    That is the main problem. The current code, which is invoked many times, does not use stored procedures. The SQL is just a string. Now, I understand breaking the command down in to two parts a command string and a List<sqlparameter>. The problem that I am trying to figure out is how the format of the sql would be determined. Would I not need to know whether the command was a update, insert, or delete? Also, there is another twist that I did not think of. Having any kind of loop in the sql may cause a different finding as I stated in my post just posted after this one today. You make a good point about me being confused. I really don't have anything that doesn't require a loop. 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.

    Thanks for all of everyone help but for now I am still stuck.

    Tuesday, May 12, 2015 7:09 PM
  • User93267240 posted

    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<QueryContainer> Container;

            private static QueryContainer instance;

            private int _id;

            private string _query;

           public _searchID

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

                     ...

    Wednesday, May 13, 2015 2:03 PM
  • User-718146471 posted

    Well, you are not using directly called variables and instead are parameterizing the query as far as I can determine; I think you have your solution.  However, I would wait for a few others to weigh in on this just to be sure.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Friday, May 15, 2015 11:19 AM
  • User93267240 posted

    I know there are a lot of posts but I finally understand and did what I was told to do. I broke the query up with parameters and I am still getting the  security error. My code is below the with the parameters removed from the hard coded string, the calling code, and the implementing code: Note: I have included the Varcode error message below as well.

    The 3 classes with the SQL w/ with the parameters broken out, the calling code, and the implementing code:

    Class with the parameters broken out:

     public class MyParam

    {

        public string name { get; set; }

            public string value { get; set; }

     }

    /// <summary>

    /// Summary description for QueryContainer SGH

     /// </summary>

      public class QueryContainer

        {

            string _query;

            public List<MyParam> parameterList = new List<MyParam>();

        public QueryContainer(string query) { _query = query; }

        public string Query

            {

             get { return _query;  }

              set { _query = value;  }

            }

      }

    The calling code:

     public int GetAccountSortByAccountCode(int account)

      {

             QueryContainer Instance = new QueryContainer("SELECT ac_sort_order FROM lkup_account_codes where ac_code = @account");

             MyParam myParam = new MyParam();

              myParam.name = "@account";

             myParam.value = account.ToString();

             Instance.parameterList.Add(myParam);

             return Convert.ToInt32(ExecuteScaler(Instance, 1));

      }

    The implementing code:

              

    if (_connection == null || _connection.State == ConnectionState.Closed)

     {

          OpenConnection();

      }

    DbCommand command = _provider.CreateCommand();

     command.Connection = _connection;

     {

         command.CommandText = Instance.Query;

         command.CommandType = CommandType.Text;

         foreach (var p in Instance.parameterList)

         {

              SqlParameter param = new SqlParameter(p.name, p.value);

              command.Parameters.Add(param);

         }

          if (_useTransaction) { command.Transaction = _transaction; }

     try {

              returnValue = command.ExecuteScalar();

     }

     catch (Exception ex)

     {

           if (ex is EntryPointNotFoundException)

            throw ex;

    89  Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') App_Code.dll loader.cs: 445 1 Open none

    Attack Vector: system_data_dll.System.Data.IDbCommand.ExecuteScalar

    Description: This database query contains a SQL injection flaw.  The call to system_data_dll.System.Data.IDbCommand.ExecuteScalar() 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. ExecuteScalar() was called on the command object, which contains tainted data. The tainted data originated from earlier calls to system_data_dll.system.data.common.dbcommand.executereader, system_web_dll.system.web.httprequest.get_params, system_web_dll.system.web.httprequest.get_form, app_code_dll.tamis.webservice.reportingservice.uicforecastsummaryfybyuic, app_code_dll.tamis.webservice.reportingservice.uicforecastsummaryfy, app_code_dll.tamis.webservice.reportingservice.uicdailyexpended, app_code_dll.tamis.webservice.reportingservice.macomsummarybymonthbyfy, app_code_dll.tamis.webservice.reportingservice.macomsummarybyfy, app_code_dll.tamis.webservice.reportingservice.expendituresbyaccount, app_code_dll.tamis.webservice.reportingservice.currentauthorizations, and app_code_dll.tamis.business.e581.validator.isdocnumvalid.

    Remediation: Avoid dynamically constructing SQL queries.  Instead, use parameterized prepared statements to prevent the database from interpreting the contents of bind variables as part of the query.  Always validate user-supplied input to ensure that it conforms to the expected format, using centralized data validation routines when possible.

      

    Wednesday, May 20, 2015 8:59 AM
  • User-821857111 posted

    Whatever tool you are using to scan for security holes is not very good. The error it reports is nonsense, I'm afraid. Ignore the warning, or talk to the vendor of the tool. Your current approach is not susceptible to SQL injection.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Thursday, May 21, 2015 4:37 PM