?
Solved

SQL Injection

Posted on 2013-06-25
8
Medium Priority
?
244 Views
Last Modified: 2013-06-27
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
Comment
Question by:tommychiu
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 2
  • 2
  • 2
  • +2
8 Comments
 
LVL 84

Assisted Solution

by:Dave Baldwin
Dave Baldwin earned 1000 total points
ID: 39276901
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
 
LVL 21

Expert Comment

by:Dale Burrell
ID: 39276904
Whats the problem?
0
 

Author Comment

by:tommychiu
ID: 39279677
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
What is SQL Server and how does it work?

The purpose of this paper is to provide you background on SQL Server. It’s your self-study guide for learning fundamentals. It includes both the history of SQL and its technical basics. Concepts and definitions will form the solid foundation of your future DBA expertise.

 
LVL 40
ID: 39280174
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
 

Author Comment

by:tommychiu
ID: 39280306
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
 
LVL 84

Expert Comment

by:Dave Baldwin
ID: 39280354
What is telling you that?  It may be because it can't examine the SQL statements when they are not hard coded.
0
 
LVL 40
ID: 39280500
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
 
LVL 11

Accepted Solution

by:
b_levitt earned 1000 total points
ID: 39281224
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

Featured Post

Containers & Docker to Create a Powerful Team

Containers are an incredibly powerful technology that can provide you and/or your engineering team with huge productivity gains. Using containers, you can deploy, back up, replicate, and move apps and their dependencies quickly and easily.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Recently while returning home from work my wife (another .NET developer) was murmuring something. On further poking she said that she has been assigned a task where she has to serialize and deserialize objects and she is afraid of serialization. Wha…
Wouldn’t it be nice if you could test whether an element is contained in an array by using a Contains method just like the one available on List objects? Wouldn’t it be good if you could write code like this? (CODE) In .NET 3.5, this is possible…
Add bar graphs to Access queries using Unicode block characters. Graphs appear on every record in the color you want. Give life to numbers. Hopes this gives you ideas on visualizing your data in new ways ~ Create a calculated field in a query: …
How to fix incompatible JVM issue while installing Eclipse While installing Eclipse in windows, got one error like above and unable to proceed with the installation. This video describes how to successfully install Eclipse. How to solve incompa…
Suggested Courses

752 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question