parameterized queries vs. SQL injection

Question

I am new to Asp.net and I'm just starting to work with classes. I recently created a class that will handle most of my SQL queries for me so that I don't have to repeatedly create new connections over all my files.

One of the methods I've created takes in an SQL query as a parameter and returns the result. I know that I should be using parameterized queries to avoid SQL injections. My question is, how can I do this when I'm passing the query as a string parameter?

For example, here's a method I'll be calling:

public static DataTable SqlDataTable(string sql)
{
    using (SqlConnection conn = new SqlConnection(DatabaseConnectionString))
    {
        SqlCommand cmd = new SqlCommand(sql, conn);
        cmd.Connection.Open();
        DataTable TempTable = new DataTable();
        TempTable.Load(cmd.ExecuteReader());
        return TempTable;
    }
}

So from another file I'd like to use this method like so:

DataTable dt = new DataTable();

dt = SqlComm.SqlDataTable("SELECT * FROM Users WHERE UserName='" + login.Text  + "' and Password='" + password.Text + "'");

if (dt.Rows.Count > 0)
{
   // do something if the query returns rows
}

This works but would still be vulnerable to injections right? Is there a way I can pass the variables to the string as parameters? I know I can do this if I create a new SQLCommand object for the query and use Parameters.AddWithValue, but I wanted all my SQL commands to be in the separate class.

Answer 1

This works but would still be vulnerable to injections right?

Yeah, your code is terrifyingly vulnerable to SQL injections.

I know that I should be using parameterized queries to avoid SQL injections.

Oh absolutely yeah.

My question is, how can I do this when I'm passing the query as a string parameter?

You simply shouldn't be passing the query as a string parameter. Instead you should be passing the query as string parameter containing placeholders and the values for those placeholders:

public static DataTable SqlDataTable(string sql, IDictionary<string, object> values)
{
    using (SqlConnection conn = new SqlConnection(DatabaseConnectionString))
    using (SqlCommand cmd = conn.CreateCommand())
    {
        conn.Open();
        cmd.CommandText = sql;
        foreach (KeyValuePair<string, object> item in values)
        {
            cmd.Parameters.AddWithValue("@" + item.Key, item.Value);
        }

        DataTable table = new DataTable();
        using (var reader = cmd.ExecuteReader())
        {
            table.Load(reader);
            return table;
        }
    }
}

and then use your function like this:

DataTable dt = SqlComm.SqlDataTable(
    "SELECT * FROM Users WHERE UserName = @UserName AND Password = @Password",
    new Dictionary<string, object>
    {
        { "UserName", login.Text },
        { "Password", password.Text },
    }
);

if (dt.Rows.Count > 0)
{
   // do something if the query returns rows
}

Answer 2

What you're trying to do makes perfect logical sense and I can understand why you would arrive at this implementation. However, what you're attempting to do is very dangerous and, being new to ASP.NET, you may not be aware that there are a variety of other options available to you that make managing your data much easier and much more secure.

@iamkrillin hinted at one such technology -- Object Relational Mapping (ORM). The .NET framework actually has first class support for an ORM called the Entity Framework. I believe the reason he suggested that you look into an ORM is because your design is actually very similar in principal to the way ORM's work. They are abstracted classes that represent tables in your database that can be easily queried with LINQ. LINQ queries are automatically parameter-ized and relieve you of the stress of managing the security of your queries. They generate SQL on the fly (the same way you are when you pass strings to your data-access class) and are much more flexible in the way they can return data (arrays, lists, you name it).

One drawback to the ORM's, however, is that they have pretty steep learning curves. A simpler option (though a bit older than EF) would be to use Typed Datasets. Typed Datasets are much easier to create than standing up ORM's and are generally much easier to implement. Though not as flexible as ORM's, they accomplish exactly what you're trying to do in a simple, safe,and already solved manner. Fortunately, when ASP.NET first came out training videos focused heavily on typed datasets and as such there are a variety of high quality freely available videos/tutorials to get you up and running quickly.

Answer 3

You are on the right track, and I have actually done what you are looking for myself too. However, instead of just passing in a string to your function, I pass in a SQL Command object... So this way, you can properly build out all your commands and parameters and then say ... here, go run this, it is ready to go. Something like

public static DataTable SqlDataTable(SqlCommand cmd)
{
    using (SqlConnection conn = new SqlConnection(DatabaseConnectionString))
    {  
        cmd.Connection = conn;   // store your connection to the command object..
        cmd.Connection.Open();
        DataTable TempTable = new DataTable();
        TempTable.Load(cmd.ExecuteReader());
        return TempTable;
    }
}

public DataTable GetMyCustomers(string likeName)
{
    SqlCommand cmd = new SqlCommand();
    cmd.CommandText = "select * from SomeTable where LastName like "@someParm%";
    cmd.Parameters.Add( "whateverParm", likeName );  // don't have SQL with me now, guessing syntax

    // so now your SQL Command is all built with parameters and ready to go.
    return SqlDataTable( cmd );
}

Answer 4

My suggestion: use an orm. There are plenty to choose from now a days