• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 285
  • Last Modified:

SQL Injection

Can anyone help me to fix the sql injection for the following code

public bool ExecuteSql(string sql, string[] szArray)
 {
            using (SqlConnection oConn = new SqlConnection(cnEForm))
            {
                SqlCommand cmd = new SqlCommand(sql, oConn);

                if (null != szArray)
                {
                    int arrayLength = szArray.Length;

                    // Parameters must have names "@val1", "@val2", "@val3", etc...
                    for (int ii = 0; ii < arrayLength; ++ii)
                    {
                        string S = szArray[ii];

                        cmd.Parameters.Add("@val" + (ii + 1).ToString(), SqlDbType.VarChar,  
                             S.Length).Value = S;
                    }
                }


                cmd.ExecuteNonQuery();  <-- sql injection right here (CWE ID 89)

                return true;
            }
        }
0
tommychiu
Asked:
tommychiu
  • 2
  • 2
  • 2
  • +2
2 Solutions
 
Dave BaldwinFixer of ProblemsCommented:
Actually it is telling you that you have not done anything above that to eliminate SQL commands that could cause problems from the data.  More info: http://msdn.microsoft.com/en-us/library/ms161953%28v=sql.105%29.aspx

You should probably click on "Request Attention" and get the SQL Server zone added to your question.
0
 
Dale BurrellDirectorCommented:
Whats the problem?
0
 
tommychiuAuthor Commented:
I changed the code a little bit:

        public System.Data.DataSet GetDataSet(string sql, string[] szArray)
        {
            using (SqlConnection oConn = new SqlConnection(cnEForm))
            {
                //SqlDataAdapter dataAdapter = new SqlDataAdapter(sql, oConn);
                SqlDataAdapter dataAdapter = new SqlDataAdapter("SELECT * FROM Users  
                     WHERE Firm = @val1 AND LoginID = @val2", oConn);
                DataSet ds = new DataSet();

                // Parameterization of the query string
                if (null != szArray)
                {
                    int arrayLength = szArray.Length;

                   for (int ii = 0; ii < arrayLength; ++ii)
                    {
                        string S = szArray[ii];

                        dataAdapter.SelectCommand.Parameters.Add("@val" + (ii + 1).ToString(),
                           SqlDbType.VarChar, S.Length).Value = S;
                    }
                }

                dataAdapter.Fill(ds);

                return ds;
            }
        }

Now, if I put the "SELECT statement" inside the dataAdapter then the sql injection fixed. Unfortunately, I cannot hard code the "SQL statement". Do you all have any idea?
0
Get expert help—faster!

Need expert help—fast? Use the Help Bell for personalized assistance getting answers to your important questions.

 
Jacques Bourgeois (James Burger)PresidentCommented:
Make sure that S.Length is a reasonable value.

The way you are doing it, you are setting the length of the parameter to the data that is passed. Anything can go in.
0
 
tommychiuAuthor Commented:
Here is the updated code:

        public System.Data.DataSet GetDataSet(string sql, string[] szArray)
        {
            using (SqlConnection oConn = new SqlConnection(cnEForm))
            {
                //SqlDataAdapter dataAdapter = new SqlDataAdapter(sql, oConn);
                SqlDataAdapter dataAdapter = new SqlDataAdapter("SELECT * FROM Users  
                     WHERE Firm = @val1 AND LoginID = @val2", oConn);
                DataSet ds = new DataSet();

                // Parameterization of the query string
                if (null != szArray)
                {
                    int arrayLength = szArray.Length;

                   for (int ii = 0; ii < arrayLength; ++ii)
                    {
                       dataAdapter.SelectCommand.Parameters.Add("@val" + (ii + 1).ToString(),
                           SqlDbType.VarChar).Value =  szArray[ii];
                    }
                }

                dataAdapter.Fill(ds);

                return ds;
            }
        }

Now, if I put the "SELECT statement" inside the dataAdapter then the sql injection fixed. Unfortunately, I cannot hard code the "SQL statement". Do you all have any idea?
0
 
Dave BaldwinFixer of ProblemsCommented:
What is telling you that?  It may be because it can't examine the SQL statements when they are not hard coded.
0
 
Jacques Bourgeois (James Burger)PresidentCommented:
The idea is not to remove the Length. You are not solving SQL injection problems by doing so. It is to make sure that it is not too big.

As an example, if you do not expect any piece Firm to be over 40 characters and the LoginID to be over 10 characters , then check the lengths before setting them in the parameter definitions. If any of them gets bigger than the limit you have set, then either stop the application with a SecurityException or truncate it to number of characters you decided upon.
0
 
b_levittCommented:
I'm not really sure why people still do these wrapper functions that end up hiding nearly all of the functionality that ado.net has to offer.

That said, there's nothing about this particular block of code that's susceptible to injection.  The injection point is more likely to be where the "string sql" parameter is being constructed outside of the method.  Please paste the code that calls this method and more importantly the code creates the "sql" string.
0
Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

Join & Write a Comment

Featured Post

Free Tool: Site Down Detector

Helpful to verify reports of your own downtime, or to double check a downed website you are trying to access.

One of a set of tools we are providing to everyone as a way of saying thank you for being a part of the community.

  • 2
  • 2
  • 2
  • +2
Tackle projects and never again get stuck behind a technical roadblock.
Join Now