7

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.

Improve this question
11
  • Why are you using inline query ? you can use store procedure to avoid sql injections . store procedure is pre-executed sql query . so it will run faster then inline query . just suggesting to use store procedure . Commented Jul 7, 2013 at 12:24
  • Having been there once, I strongly you delve into learning and ORM (I chose Entity Framework). I spent many hours of writing code like this only to find out how unnecessary that was! Before its too late, learn Entity Framework. If you don't have too complex queries and mainly to simple CRUD operations, its is not that difficult to learn.. Commented Jul 7, 2013 at 12:24
  • Hiren, stored procedures are NOT pre-executed queries.. Do you mean the execution plan? Commented Jul 7, 2013 at 12:28
  • 1
    @HirenDhaduk Stored procedures are outdated! Don't use them if you can somehow avoid it. Commented Jul 7, 2013 at 12:49
  • 3
    @DarinDimitrov we are not suggesting ORMs to solve the problem of injection attacks.. We are suggesting a different approach to his programming where he does not need to deal with such things. Commented Jul 7, 2013 at 13:13

4 Answers 4

Reset to default
15

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
}
Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

Thanks for the help. There's been a lot of helpful information from everyone, but I figured I'd mark this one as the answer since it was related to my original goal directly. However I'm going to look into all suggestions to learn more about this process. Thanks!
@Darin Dimitrov , foreach (string item in values) Is it possible ?
0

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.

Improve this answer

7 Comments

If you don't want to use a crappy ORM like Entity Framework. Take your pick, there's heaps. NHibernate, LightSpeed. And billions of Micro ORMs like PetaPoco, Massive, ServiceStack.OrmLite, Dapper...
@DarinDimitrov why is it bad? Can you support your argument with any resources/articles/tests/etc ??
@Emin, sure, look at the number of questions on StackOverflow about people having problems with Entity Framework. Now look at the number of questions on StackOverflow about people having problems with other ORMs. Then count. The bigger the number, the worst, and believe me EF is a leader in this competition by a great magnitude :-) But I don't even understand why we are arguing about ORMs here. The OP has a specific question that has nothing to do with ORMs. It's about how to avoid SQL injection attacks with ADO.NET.
@DarinDimitrov I do believe you. I started using it recently for a simple project and was thinking what problems I might face in the future with more advanced projects. Since you are against EF I thought you might give me some info about it thats why I asked.
@ajax81 This is a great help. Thanks a lot for the link to the further information as well. I'll definitely look further into all of this.
|
0

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 );
}
Improve this answer

Comments

-3

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

Improve this answer

2 Comments

Why use an ORM when ADO.NET already has everything that's necessary to prevent SQL injection? An ORM could sometimes be an overhead.
@darindimitrov I'm not arguing that point, just pointing out that what he is doing is the exact problem that ORMs solve. Sure there can be a performance penalty (depends on the ORM) but it's nothing compared to the damage that SQL injection can cause - Nevermind all the productivity gains and all that. Lastly since the OP mentioned he was "new" I chose to suggest an option that is more friendly for someone just getting started

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.