ASP.NET, C# How to Pass a StringQuery to a custom SQL Command

Question

I have a slight issue, I have a ASP.NET Webforms application. I'm sending over a url?id=X were X is my database index or id.

I have a C# class file to run my SQL connection and query. Here is the code:

public DataTable ViewProduct(string id)
{
    try
    {
        string cmdStr = "SELECT * Products WHERE Idx_ProductId = " + id;

        DBOps dbops = new DBOps();
        DataTable vpTbl = dbops.RetrieveTable(cmdStr, ConfigurationManager.ConnectionStrings["MyDatabase"].ConnectionString);
        return vpTbl;
    }
    catch (Exception e)
    {
        return null;
    }
}

So as you can see my problem lies within string cmdStr = "SQL Query" + variable;

I'm passing over my index or id through the URL then requesting it and turning it into a string then using ViewProduct(productId).

I don't know what syntax or how to add the id into my C# string sql query. I've tried:

string cmdStr = "SELECT * Products WHERE Idx_ProductId = @0" + id;
string cmdStr = "SELECT * Products WHERE Idx_ProductId = {0}" + id;

also what I have currently to no avail.

You desperately need to read about, understand, and start parameterizing your queries immediately if not sooner. What you have here is a textbook definition of sql injection. It is fine to pass your parameter values via querystring but you need to use parameterized queries or you vulnerable. — Sean Lange, Feb 24 '16 at 22:10
What you are looking for is called `ADO.NET`, [Google that](https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=how+to+c%23+ado.net) and you will come up with plenty of examples of how to create database statements in c#. Here is an example (first on google results): [ADO.NET Code Examples](https://msdn.microsoft.com/en-us/library/dw70f090(v=vs.110).aspx). — Igor, Feb 24 '16 at 22:10
The link that @Igor posted is a good one but it also suggests using AddWithValue. This is problematic when using pass through sql like this. It will sometimes guess the datatypes incorrectly. http://blogs.msmvps.com/jcoehoorn/blog/2014/05/12/can-we-stop-using-addwithvalue-already/ — Sean Lange, Feb 24 '16 at 22:11
All great information thanks all! The reason why I have not used the ADO.NET example is because my SqlConnection and SqlCommands are handled within the DBOps class file and would be ridiculous to add in special parameters for all sorts of queries in there so I will change the structure but before I do I will check out parameterizing queries — 702cs, Feb 24 '16 at 22:18
@Sean Large or anyone else who can help, by parameterizing my queries you are referring to instead of SELECT * (everything) to pick which params to SELECT ProductId, ProductName Etc? — 702cs, Feb 24 '16 at 22:25
No. That is selecting only the columns you need. Consider what would happen if somebody changed the query string to be "delete Products". The way you have this coded your entire table would be emptied. Look at the first example in the link that Igor posted. It demonstrates how to use parameters. But instead of using AddWithValue you should look at how to add the parameter from the link I posted. Or you could try google "dotnet parameterized query example". That should get you thousands of examples. — Sean Lange, Feb 24 '16 at 22:34
While we are on the topic...you really should do something in your catch blocks. Simply returning NULL is an anti-pattern I call try/squelch. What you have done is trap an error and then just continue on as if nothing happened. This is dangerous to most applications and downright horrid for developers. How can you fix an error you don't even know is happening? — Sean Lange, Feb 24 '16 at 22:36
My last suggestion would be to use the native database controls instead of whatever that DBOps class is. From what you posted it looks like it might be problematic to add parameters or use a stored procedure. — Sean Lange, Feb 24 '16 at 22:37
Thanks for all the info all, the return null; is just for testing I was to lazy to add a few lines to add a column with the exception I've added it in now. Also I must have mistakenly misunderstood asp.net you guys say instead of SELECT someone with the use of sql injection could change it to "DELETE". The sql query command and connections are handled within a .cs file in my project. No sql commands are being passed through the url or should be able to be changed or used correct? The only sql injection they could do is changed ?id=X then change the 'X' to any number they want? — 702cs, Feb 24 '16 at 22:55

score 4 · Accepted Answer · edited May 23 '17 at 12:17

I was so sure this would be a duplicate of some canonical question about parameterized queries in C#, but apparently there isn't one (see this)!

You should parameterize your query - if you don't, you run the risk of a malicious piece of code injecting itself into your query. For example, if your current code could run against the database, it would be trivial to make that code do something like this:

// string id = "1 OR 1=1"
"SELECT * Products WHERE Idx_ProductId = 1 OR 1=1" // will return all product rows
// string id = "NULL; SELECT * FROM UserPasswords" - return contents of another table
// string id = "NULL; DROP TABLE Products" - uh oh
// etc....

ADO.NET provides very simple functionality to parameterize your queries, and your DBOps class most assuredly is not using it (you're passing in a built up command string). Instead you should do something like this:

public DataTable ViewProduct(string id)
{
    try
    {
        string connStr = ConfigurationManager.ConnectionStrings["MyDatabase"].ConnectionString;
        using (SqlConnection conn = new SqlConnection(connStr))
        {
            conn.Open();
            using (SqlCommand cmd = conn.CreateCommand())
            {
                // @id is very important here!
                // this should really be refactored - SELECT * is a bad idea
                // someone might add or remove a column you expect, or change the order of columns at some point
                cmd.CommandText = "SELECT * Products WHERE Idx_ProductId = @id";
                // this will properly escape/prevent malicious versions of id
                // use the correct type - if it's int, SqlDbType.Int, etc.
                cmd.Parameters.Add("@id", SqlDbType.Varchar).Value = id;
                using (SqlDataReader reader = cmd.ExecuteReader())
                {
                    DataTable vpTbl = new DataTable();
                    vpTbl.Load(reader);
                    return vpTbl;
                }
            }
        }
    }
    catch (Exception e)
    {
        // do some meaningful logging, possibly "throw;" exception - don't just return null!
        // callers won't know why null got returned - because there are no rows? because the connection couldn't be made to the database? because of something else?
    }
}

Now, if someone tries to pass "NULL; SELECT * FROM SensitiveData", it will be properly parameterized. ADO.NET/Sql Server will convert this to:

DECLARE @id VARCHAR(100) = 'NULL; SELECT * FROM SensitiveData';
SELECT * FROM PRoducts WHERE Idx_ProductId = @id;

which will return no results (unless you have a Idx_ProductId that actually is that string) instead of returning the results of the second SELECT.

Some additional reading:

Thank you this helped a lot! Kinda covered everything including an example that helped a lot to thanks again everyone. — 702cs, Feb 24 '16 at 23:08
I spent almost as much time looking for a question about this as I did writing it. Glad to help. — Dan Field, Feb 24 '16 at 23:11

score 0 · Answer 2 · answered Feb 24 '16 at 22:36

0

What type Products.Idx_ProductId is?

Probably it is string, than you need to use quotes: "... = '" + id.Trim() + "'";

answered Feb 24 '16 at 22:36

Eckd

358
1
10

1

This would be more approriate as a comment. – Dan Field Feb 24 '16 at 22:44
I think this is the correct path for the solution but we might have some misplaced ' ". And yes the "id" is a string. – 702cs Feb 24 '16 at 23:04
1

And this is still not a really good idea - see this question for more info: http://stackoverflow.com/questions/910465/avoiding-sql-injection-without-parameters – Dan Field Feb 24 '16 at 23:15

ASP.NET, C# How to Pass a StringQuery to a custom SQL Command

2 Answers2

Linked