ASP.NET

15-May-15 2:10

I have a static class that needs to pass a generic List of strings to a function using an integer as a index to the List in the class. The problem is the static class doesn't have a List collect and I don't have a proper index to access the class in the function it is passed to. The class, the calling code, and the receiving function are below.

My Class:

public class QueryContainer
{

    public static QueryContainer Instance = new QueryContainer();

    private int _id;

    private string _query = "";

    private int _searchID;

    public QueryContainer() { }

    public string Query
    {
        get
        {
            if (Instance != null)
                return Instance._query;
            else
                return "";
        }

        set { _query = value; _id =+ 1; }
    }

    public int ID { get { return _id; } }

    public int SearchID
    {
        set { _searchID = value; }
        get { return _searchID; }

    }        

}

The calling code:

public int GetAccountSortByAccountCode(int account)
{
    int Id = 0;

    QueryContainer.Instance.Query = "SELECT ac_sort_order FROM lkup_account_codes where ac_code = " + account.ToString();


    return Convert.ToInt32(ExecuteScaler(Id));
}

The function that the static class is passed to:

public int GetAccountSortByAccountCode(int account)
{
    int Id = 0;

    QueryContainer.Instance.Query = "SELECT ac_sort_order FROM lkup_account_codes where ac_code = " + account.ToString();


    return Convert.ToInt32(ExecuteScaler(Id));
}

Corrected <pre> tags to recognise C#, terminators, and indentation.

modified 15-May-15 11:19am.

Richard MacCutchan15-May-15 3:02

Richard MacCutchan

15-May-15 3:02

Please use <pre> tags around your code, so it is more readable. Also, is this an ASP.NET issue?

Richard Deeming15-May-15 3:56

15-May-15 3:56

As Sascha told you two days ago[^], that class does not resolve your SQL Injection[^] vulnerabilities.

Rather than wasting time trying to fix this class, concentrate on fixing the calling code. You can either do that by writing raw ADO.NET code using properly parameterized queries; or you can use something like Dapper[^]; or switch to one of the many available .NET ORM solutions.

Correct ADO.NET code would look something like this:

public int GetAccountSortByAccountCode(int account)
{
    using (var connection = new SqlConnection("YOUR CONNECTION STRING"))
    using (var command = new SqlCommand("SELECT ac_sort_order FROM lkup_account_codes where ac_code = @account", connection))
    {
        command.Parameters.AddWithValue("@account", account);

        connection.Open();
        return Convert.ToInt32(command.ExecuteScalar());
    }
}

"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer

Stephen Holdorf15-May-15 4:09

15-May-15 4:09

The problem is that I don't know what the Hard coded SQL expression is. All I am trying to do it run that hard coded statement without breaking out the statement's parameters. If this wont work is that way to pass a generic list of parameter strings and use a foreach statement to add the parameters to the string's parameters?

Richard Deeming15-May-15 4:11

15-May-15 4:11

You don't have a choice - you MUST look at every single SQL command issued by your program to "break out" the parameters. If you don't, then your code will still be vulnerable to SQL Injection.

You have a major security vulnerability in your code, and you need to fix it. There's no short-cut to doing that.

"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer

Stephen Holdorf15-May-15 6:52

15-May-15 6:52

Below is the best I could come up with. Now the SQL injection error was showing up in the actual ExecuteScaler function using the SQL string being passed in as so:

The old version my code replaced: protected object ExecuteScaler(string queryString)

Now my version of the function is as so: protected Object ExecuteScaler(QueryContainer Instance)

The calling function is now converted as so:

public int GetAccountSortByAccountCode(int account)
        {
            QueryContainer Instance = new QueryContainer("SELECT ac_sort_order FROM lkup_account_codes where ac_code = " + account.ToString());

            return Convert.ToInt32(ExecuteScaler(Instance.Query));
        }  <pre>

And here is my new class:

<pre>
public class QueryContainer
    {
        string _query;

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

        public string Query
        {
            get
            {
                return _query;
            }

            set { _query = value;  }
        }
    }
<pre>

So what do you think. Does someone that I will still get SQL injection errors?

Richard Deeming15-May-15 7:04

15-May-15 7:04

holdorf wrote:
QueryContainer Instance = new QueryContainer("SELECT ac_sort_order FROM lkup_account_codes where ac_code = " + account.ToString());

No, no, no!!!

Your code is STILL vulnerable to SQL Injection.

You are STILL using string concatenation, rather than parameterized queries.

All you've done is store the compromised query in a field on a class, and then executed it. Like shutting the stable door after the horse has bolted, that provides precisely zero protection.

If you want to fix the SQLi vulnerability in your code, you MUST use parameterized queries. That means finding every part of your code which issues a query, and updating it to use parameters instead of string concatenation.

When you've finished, you should be able to mark all of the string variables containing your queries as const. If you can't, then you've almost certainly missed a parameter, and left your code vulnerable.

"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer

Sascha Lefèvre15-May-15 8:23

15-May-15 8:23

Maybe you have the completely wrong idea about what SQL-injection is.

It does not mean that your query string, after being concatenated (wrong approach) can somehow still be modified by someone and that you therefore have to "hide" it somehow (in your QueryContainer).

It means that the values you get from user input (or from somewhere else outside your controllable domain) can already contain malicious SQL-statements. And currently you blindly concatenate them into your SQL-statement.

While SQL-Parameters can not prevent that your values can contain malicious SQL-statements, they do prevent that these will be executed by your database when you're executing your SQL-statement. Because the values will be treated just as values and not potentially as SQL-statements.

If you have understood this, then you will realize that your current approach to fixing this makes no sense.

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

Stephen Holdorf18-May-15 1:47

Re: Help with a static class [modified]

18-May-15 1:47

Sascha,

Ok. I do understand. Know my problem is how do I break the strings like "delete 'Name' from table where x = 1 and y = 2" or "select * from table where x = 1 and y = 2" with the least possible work? Remember I have only one function to fix and handle this problem. My boss is upset and is giving me only a few days to fix this. Please help.

Thanks,

Steve Holdorf

modified 18-May-15 8:00am.

Sascha Lefèvre18-May-15 2:07

18-May-15 2:07

removed: was premature advice. Even a SQL-parser won't help, see Richard's reply.

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

modified 19-May-15 15:24pm.

Sascha Lefèvre18-May-15 2:10

18-May-15 2:10

holdorf wrote:
My boss is upset and is giving me only a few days to fix this.

If you were the one writing the whole thing, then you should be able to modify the code that builds these sql-statements instead of replacing the literal values afterwards. If you were not the one writing the whole thing you should tell your boss that it takes more time to fix sh!t than to produce it.. maybe in other words Big Grin | :-D

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

Richard Deeming18-May-15 2:36

18-May-15 2:36

holdorf wrote:
Remember I have only one function to fix and handle this problem.

Then your project is doomed.

Once you have used string concatenation to inject parameters into a query, there is absolutely no way to undo that. Even if you manage to write or find a routine to parse a SQL string, it's too late; the damage has already been done.

What you are asking for is essentially the same as saying you've baked a cake, but now you've realised the eggs were off, and you want to go back and replace the eggs without re-making the cake. It can't be done.

The ONLY solution for this problem is to re-visit every piece of code which generates a SQL query and update it to use a parameterized query.

If your boss doesn't understand that, then point him to Troy Hunt's explanation[^], or Bobby Tables[^].

"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer

Stephen Holdorf18-May-15 3:48

18-May-15 3:48

Can someone please give me a small piece of code of how this should be implemented?

Richard Deeming18-May-15 3:50

18-May-15 3:50

holdorf wrote:
Can someone please give me a small piece of code of how this should be implemented?

How many times do we have to give you the same information? Mad | :mad:

I gave you a perfectly good example of correct ADO.NET code earlier in this thread:
http://www.codeproject.com/Messages/5058941/Re-Help-with-a-static-class.aspx[^]

"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer

jkirkerx18-May-15 12:19

jkirkerx

18-May-15 12:19

Interesting Richard.

After reading that, I feel like I just wrote code to lower the level of knowledge needed to use one of my programs.

You were pretty clear in the 3rd post.
Now the several days deadline he had, was pissed away not understanding really bad coding practices.
I was wondering why there was so many posts here.

Stephen Holdorf20-May-15 3:02

20-May-15 3:02

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:

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:

<pre>
        public int GetAccountSortByAccountCode(int account)
        {
            QueryContainer Instance = new QueryContainer("SELECT ac_sort_order FROM lkup_account_codes where ac_code = <a href="http://www.codeproject.com/Members/account">@account</a>");

            MyParam myParam = new MyParam();

            myParam.name = "@account";
            myParam.value = account.ToString();

            Instance.parameterList.Add(myParam);

            return Convert.ToInt32(ExecuteScaler(Instance, 1));
        } 
<pre>

The implementing code:

<pre>
                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;
                        //if (_useTransaction == true)
                        //_transaction.Rollback();
                        RollBack();

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

                        _iserror = true;
                    }

<pre>

modified 20-May-15 9:38am.

Sascha Lefèvre20-May-15 4:22

20-May-15 4:22

For which method do you get the security warning? I don't think it will be the one you posted here (GetAccountSortByAccountCode(..)).

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

Stephen Holdorf20-May-15 4:28

20-May-15 4:28

The scan shows the command.ExecuteScalar() is where the security flaw is:

Here is the function and the line highlighted where the error occurs:

        protected Object ExecuteScaler(QueryContainer Instance, int i)
        {

            object returnValue = null;


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

                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;
                        //if (_useTransaction == true)
                        //_transaction.Rollback();
                        RollBack();

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

                        _iserror = true;
                    }
<pre>

Sascha Lefèvre20-May-15 4:34

20-May-15 4:34

Then it's a dumb security report in that it's not able to pinpoint a more helpful location (not in that it's wrong in the first place). The highlighted line must be executed and it can't be executed in any other way. Doesn't the security message mention a method name (other than ExecuteScaler(..)) ?

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

Stephen Holdorf20-May-15 4:40

20-May-15 4:40

Here is what the report printed out and line 445 is the returnValue = command.ExecuteScaler(). There has to be a reason it flags line 445 which is the code above.

2054 App_Code.dll tamis-root-bak/.../data/loader.cs 445 V.Likely

Sascha Lefèvre20-May-15 4:43

20-May-15 4:43

Which tool are you using for these security checks?

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

Stephen Holdorf20-May-15 4:45

20-May-15 4:45

Veracode web scan

Sascha Lefèvre20-May-15 4:54

20-May-15 4:54

It should be able to show you somewhere the complete code path that was executed before it reached that line. Is there maybe a "Details"-link/button or similar?

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

Stephen Holdorf20-May-15 5:21