Converting multiple query to use parameters to avoid SQL injection

Question

I have some dropdownlist in my aspx page and I am using the choices from them in my SQL query:

query = "";
DataTable taskData = new DataTable();
connString = @""; //connection string
strClause = "";

if (!blOnLoad)
{
    if (ddlTaskName.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND CT.ATTR2739 LIKE '%" + ddlTaskName.SelectedItem.Text + "%'";
        //strClause += string.format();
    }
    else
    {
        strClause += " AND (CT.ATTR2739 LIKE '%' OR CT.ATTR2739 IS NULL)";
    }
    if (ddlService.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND SE.ATTR2821 LIKE '%" + ddlService.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (SE.ATTR2821 LIKE '%' OR SE.ATTR2821 IS NULL)";
    }
    if (ddlStatus.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND CT.ATTR2812 LIKE '%" + ddlStatus.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (CT.ATTR2812 LIKE '%' OR CT.ATTR2812 IS NULL)";
    }
    if (ddlDueDate.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND CONVERT(VARCHAR(14), CT.ATTR2752, 110) LIKE '%" + ddlDueDate.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (CONVERT(VARCHAR(14), CT.ATTR2752, 110) LIKE '%' OR CONVERT(VARCHAR(14), CT.ATTR2752, 110) IS NULL)";
    }
    if (ddlOwner.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND UA.REALNAME LIKE '%" + ddlOwner.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (UA.REALNAME LIKE '%' OR UA.REALNAME IS NULL)";
    }
    if (ddlClient.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND C.ATTR2815 LIKE '%" + ddlClient.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (C.ATTR2815 LIKE '%' OR C.ATTR2815 IS NULL)";
    }
    if (ddlSite.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND SI.ATTR2819 LIKE '%" + ddlSite.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (SI.ATTR2819 LIKE '%' OR SI.ATTR2819 IS NULL)";
    }
    if (ddlPractice.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND PR.ATTR2817 LIKE '%" + ddlPractice.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (PR.ATTR2817 LIKE '%' OR PR.ATTR2817 IS NULL)";
    }
    if (ddlProvider.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND P.ATTR2919 LIKE '%" + ddlProvider.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (P.ATTR2919 LIKE '%' OR P.ATTR2919 IS NULL)";
    }


    if (ddlTaskName.SelectedIndex == 0 && ddlService.SelectedIndex == 0 && ddlStatus.SelectedIndex == 0 && ddlDueDate.SelectedIndex == 0 && ddlOwner.SelectedIndex == 0 && ddlClient.SelectedIndex == 0 && ddlSite.SelectedIndex == 0 && ddlPractice.SelectedIndex == 0 && ddlProvider.SelectedIndex == 0)
    {
        query = strMainQuery + " WHERE CT.ACTIVESTATUS = 0";
    }
    else
    {
        query = strMainQuery + " WHERE CT.ACTIVESTATUS = 0" + strClause;
    }
}
else
{
    query = strMainQuery + " WHERE CT.ACTIVESTATUS = 0";
}

using (SqlConnection conn = new SqlConnection(connString))
{
    try
    {
        SqlCommand cmd = new SqlCommand(query, conn);

        SqlDataAdapter da = new SqlDataAdapter(query, conn);

        myDataSet = new DataSet();
        da.Fill(myDataSet);

        myDataView = new DataView();
        myDataView = myDataSet.Tables[0].DefaultView;

        yourTasksGV.DataSource = myDataView;
        yourTasksGV.DataBind();
    }
}

I am using the dropdownlist text right inside my SQL query which I am sure is prone to SQL injection which I am trying to prevent. I was looking around and found out I can use string.format() along with .Parameters.AddWithValue(); to ensure there is no SQL injection.

I am not sure how to actually take my code above and change it entirety to use Parameters.

How can I achieve the use of parameters instead of taking the dropdownlist text?

Answer 1

The recommended way to avoid SQL injection attacks is to use parameters. Also I can recommend that you create a stored procedure instead of using dynamic SQL.

You can pass your dropdown indexes as parameters to your SP and use the old trick of using it as conditional on the where clause.

Your select can end up like this:

select CT.columnA, CT.columnB
from tableA CT
join tableB SE on SE.idA = CT.id
where
      (@ddlTaskName_SelectedIndex > 0 and (CT.ATTR2739 LIKE '%' + @ddlTaskName_SelectedItemText))
-- or (@ddlTaskName_SelectedIndex = 0 and (CT.ATTR2739 LIKE '%' OR CT.ATTR2739 IS NULL))     -- if you think enough ill see this line is unnecessary
and 
      (@ddlService_SelectedIndex > 0 and (SE.ATTR2821 LIKE '%' + @ddlService_SelectedItemText + '%')
-- This "else" line is unnecessary too, since we don't really filter it because eveything is "like %" or "null"

And so on ...

Those dropdown texts are still prone to SQL injection since a smart hacker may be able to change its value depending on your system. You can fix it to just set variables inside the SP, this mean you will not pass the drop down text, just the indexes and do some swith case to set that varchar variables before the select.

That can add some maintenance issue, since your dropdown texts must match the same texts hardcoded on your SP. A better approach can be to create a domain table for each dropdown:

Create table DropDownClientValues
(
 index int
,text varchar(50)
)

And list your dropdown values directly from those domain tables; that way all your selects/SP can refer the exact same values.

This has the advantage of making adding/removing options from your drop downs very easy and with no impact in your code. Sadly this can be a bit onerous to refactor a big app that way.

Answer 2

You could also use another using statement here

using (SqlConnection conn = new SqlConnection(connString))
{
    try
    {
        SqlCommand cmd = new SqlCommand(query, conn);

        SqlDataAdapter da = new SqlDataAdapter(query, conn);

        myDataSet = new DataSet();
        da.Fill(myDataSet);

        myDataView = new DataView();
        myDataView = myDataSet.Tables[0].DefaultView;

        yourTasksGV.DataSource = myDataView;
        yourTasksGV.DataBind();
    }
}

and make it

using (SqlConnection conn = new SqlConnection(connString))
using (SqlCommand cmd = new SqlCommand(query, conn))
using (SqlDataAdapter da = new SqlDataAdapter(query, conn))
{
        myDataSet = new DataSet();
        da.Fill(myDataSet);

        myDataView = new DataView();
        myDataView = myDataSet.Tables[0].DefaultView;

        yourTasksGV.DataSource = myDataView;
        yourTasksGV.DataBind();
    }
}

I didn't see any declaration of myDataSet or myDataView so I var'd them.

You don't use the SQLCommand so you could get rid of that altogether, so it becomes

using (SqlConnection conn = new SqlConnection(connString))
using (SqlDataAdapter da = new SqlDataAdapter(query, conn))
{
        myDataSet = new DataSet();
        da.Fill(myDataSet);

        myDataView = new DataView();
        myDataView = myDataSet.Tables[0].DefaultView;

        yourTasksGV.DataSource = myDataView;
        yourTasksGV.DataBind();
    }
}

---

This is what you have:

using (SqlConnection conn = new SqlConnection(connString))
{
    try
    {
        SqlCommand cmd = new SqlCommand(query, conn);

        SqlDataAdapter da = new SqlDataAdapter(query, conn);

        myDataSet = new DataSet();
        da.Fill(myDataSet);

        myDataView = new DataView();
        myDataView = myDataSet.Tables[0].DefaultView;

        yourTasksGV.DataSource = myDataView;
        yourTasksGV.DataBind();
    }
}

This is what I would change it to

using (SqlConnection conn = new SqlConnection(connString))
using (SqlDataAdapter da = new SqlDataAdapter(query, conn))
{
        myDataSet = new DataSet();
        da.Fill(myDataSet);

        myDataView = new DataView();
        myDataView = myDataSet.Tables[0].DefaultView;

        yourTasksGV.DataSource = myDataView;
        yourTasksGV.DataBind();
    }
}

The explanation is above, you should make sure that you know how this works before implementing to be certain that it works for your code.