The secure way to return some value from database (SQL SERVER & ASP.NET)

Question

Is the code below implementing the secure way to retrieve the data from database? help me please, I don't understand about SQL Injection. Someone told me this code can easily get injected. If yes, can somebody explain it? Thank you.

public int CheckID(string column, string table, string wheres)
    {
        int i = 0;
        sqlcon = ConnectToMain();
        string sqlquery = "SELECT "+column+" FROM "+table+" "+wheres+"";
        using (sqlcon)
        {
            sqlcon.Open();
            SqlCommand sqlcom = new SqlCommand(sqlquery, sqlcon);
            using (sqlcom)
            {
                SqlDataReader dr = sqlcom.ExecuteReader();
                dr.Read();
                if (dr.HasRows)
                {
                    i = dr.GetInt32(0);
                }
                else
                {
                    i = 0;
                }
            }
            sqlcon.Close();
        }
        return i;
    }

Start here: http://stackoverflow.com/questions/332365/how-does-the-sql-injection-from-the-bobby-tables-xkcd-comic-work — Steve, Nov 03 '16 at 09:52
This code has a lot of bugs, and won't even run (you forgot to open the connection). It also hints at a connection leak - `sqlcon` isn't defined which means it's a field that doesn't get cleared. It probably doesn't get closed propely in other methods either. — Panagiotis Kanavos, Nov 03 '16 at 09:57
@PanagiotisKanavos what do you mean about "you forgot to open the connection" ? — Fityan Aula, Nov 03 '16 at 10:04
I missed the `Open()` in the noise. This snippet is actually worse for reusability. You end up passing unnecessary parameters - are you really going to search against the User table? Or Payments? What is the actual query you want to execute? If you want to search against a list of IDs, use an `IN (1,2,3)` clause. Or use EF or a micro ORM like Dapper to avoid all this. — Panagiotis Kanavos, Nov 03 '16 at 10:14

Panagiotis Kanavos · Accepted Answer · 2016-11-03T10:21:48.067

This code has far too many problems.

Table, column and criteria are passed as strings and concatenated, which means that the code is prone to SQL injection.
Database details like table, column criteria are spilled into the function's caller. Are you going to use this method to query anything other than a Visitor table?
A reader is used when only a single value is wanted.
The connection is created outside the using block and stored in a field. This is definitelly a memory leak and probably a connection leak as well. Just create the connection locally.

A simple command call fixes all of these problems:

public int CheckIDVisitor(visitorName)
{
    string query = "SELECT ID FROM Visitors where Name=@name";
    using (var sqlConn=new SqlConnection(Properties.Default.MyDbConnectionString))
    using( var cmd=new SqlCommand(query,sqlConn))
    {
        var cmdParam=cmd.Parameters.Add("@name",SqlDbType.NVarChar,20);
        cmdParam.Value=visitorName;
        sqlConn.Open();

        var result=(int?)cmd.ExecuteScalar();
        return result??0;
    }
}

You could also create the command in advance and store it in a field. You can attach the connection to the command each time you want to execute it:

public void InitVisitorCommand()
{
    string query = "SELECT ID FROM Visitors where Name=@name";
    var cmd=new SqlCommand(query,sqlConn);
    var cmdParam=cmd.Parameters.Add("@name",SqlDbType.NVarChar,20);
    _myVisitorCommand=cmd;
}

...

public int CheckIDVisitor(visitorName)
{
    using (var sqlConn=new SqlConnection(Properties.Default.MyDbConnectionString))
    {
        _myVisitorCommand.Parameters.["@name"]Value=visitorName;
        _myVisitorCommand.Connection=sqlConn;
        sqlConn.Open();

        var result=(int?)cmd.ExecuteScalar();
        return result??0;
    }
}

An even better option would be to use a micro-ORM like Dapper.Net to get rid of all this code:

public int CheckIDVisitor(visitorName)
{
    using (var sqlConn=new SqlConnection(Properties.Default.MyDbConnectionString))
    {
        string sql = "SELECT ID FROM Visitors WHERE name=@name"
        var result = conn.Query<int?>(sql, new { name = visitorName);
        return result??0;
    }
}

Or

public int[] CheckIDVisitors(string []visitors)
{
    using (var sqlConn=new SqlConnection(Properties.Default.MyDbConnectionString))
    {
        string sql = "SELECT ID FROM Visitors WHERE name IN @names"
        var results = conn.Query<int?>(sql, new { names = visitors);
        return results.ToArray();
    }
}

Actually I am using sql parameter to avoid that injection, but it bothers me and I always should write the same code. For example to check some ID in database, I have to check ID 1, ID 2, and ID 3. And I must write the same structure of the code three times. But with that example code I just put the necessary stuffs there and just write one time code. Is there any suggestion to solve this problem? Or you are doing the same way Sorry for my bad, I forgot to change the method name hehe. Actually this code should be covered many things that related with checking ID. — Fityan Aula, Nov 03 '16 at 10:14
Post the *actual* query you want to execute. And as you see, creating the commands you want in advance is trivial. — Panagiotis Kanavos, Nov 03 '16 at 10:16
Thank you for suggesting me to use something like Dapper.Net. It's very help me (y) — Fityan Aula, Nov 03 '16 at 10:28

The secure way to return some value from database (SQL SERVER & ASP.NET)

1 Answers1

Linked