MVC 4 SQL string injection Security

Question

Hi I am fairly new to MVc and C# i was wondering if i could use the model security to prevent SQL injections? I created a model that contains variables that we are recieving from client input then forming a SQL statement from them. I was wondering if the built in security in MVC would be enough to prevent SQL injections? Please have a look at the code any suggestions are greatly appreciated.

Model

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;

namespace DataBaseTest.Models
{
   public class BedroomModel
   {
    public string YrBlt1 { get; set; }
    public string YrBlt2 { get; set; }
    public string TotLivArea1 { get; set; }
    public string TotLivArea2 { get; set; }
    public string LotArea1 { get; set; }
    public string LotArea2 { get; set; }
    public string Bedrooms { get; set; }
    public string SalePrice1 { get; set; }
    public string SalePrice2 { get; set; }
    public string SaleDate { get; set; }
    public string AssesVal1 { get; set; }
    public string AssesVal2 { get; set; }
    public string Style { get; set; }
    public string ArchStyle { get; set; }
    public string TaxUnit { get; set; }

    // Criteria added during SQL Queries
    public string YearBuilt { get; set; }
    public string LivingArea{ get; set; }
    public string LotArea { get; set; }
    public string SalePriceA { get; set; }
    public string SaleDateA { get; set; }
    public string AssesVal { get; set; }
    public string StyleA { get; set; }
    public string ArchStyleA { get; set; }
    public string ParcelId { get; set; }
    public string QuickRefId { get; set; }
    public string TaxunitA { get; set; }
    public string Address { get; set; }
    public string ValCode { get; set; }
    public string BedroomA { get; set; }

Then our SQL

  [HttpPost]
    public ActionResult Index(DataBaseTest.Models.BedroomModel user,DataTable dtFindResults)
    {
        StringBuilder sbSQL = new StringBuilder();
        //// define a list of CustomerModel objects
        DataSet tempDS = new DataSet();

        //string xSQL = "SELECT PropertyAddress,PropertyTypeDesc,PropertyID FROM KDOR_vwPropertyGeneral ORDER BY PropertyAddress";
        System.Data.SqlClient.SqlDataAdapter DbCmd = new System.Data.SqlClient.SqlDataAdapter();
        string sqlWhereCont = " WHERE ";
        sbSQL.Append("SELECT ");
        //sbSQL.Append(SessionHandler.AddressPointsPointsIDColumn + " AS PointsID,");
        sbSQL.Append("pg.PropertyNumberSearch,");
        sbSQL.Append("pg.QuickRefID,");
        sbSQL.Append("pg.PropertyAddress,");
        sbSQL.Append("crb.fmsstyle,");
        sbSQL.Append("srb.farchstyle,");
        sbSQL.Append("pt.TransferValidityCode,");
        sbSQL.Append("pg.TaxingUnitGroupCode,");
        sbSQL.Append("pt.Price,");
        sbSQL.Append("pt.SaleDate,");
        sbSQL.Append("crb.fyrblt,");
        sbSQL.Append("crb.vResBldgDep_tla_value,");
        sbSQL.Append("lm.facres,");
        sbSQL.Append("crb.frmbed");
        sbSQL.Append(" FROM KDOR_vwPropertyGeneral pg ");
        sbSQL.Append(" Left Join cama_ResBldg crb ON pg.PropertyID = crb.PropertyID And pg.AdHocTaxYear = crb.AdHocTaxYear ");
        sbSQL.Append(" Left Join sales_ResBldg  srb ON pg.PropertyID = srb.PropertyID");
        sbSQL.Append(" Left Join KDOR_vwPropertyTransfer pt On pg.PropertyID = pt.PropertyID");
        sbSQL.Append(" Left Join cama_LandMkt lm ON pg.PropertyID = lm.PropertyID And pg.AdHocTaxYear = lm.AdHocTaxYear");
        if (!string.IsNullOrEmpty(user.YrBlt1)||!string.IsNullOrEmpty(user.YrBlt2))
        {
            //sbSQL.Append(sqlWhereCont +"PropertyAddress = '" + user.Address + "'");
            //sqlWhereCont = "AND ";
            sbSQL.Append(sqlWhereCont +"crb.fyrblt >="+ user.YrBlt1+ " And crb.fyrblt <=  " + user.YrBlt2 );
            sqlWhereCont = "AND ";
        }
        if (!string.IsNullOrEmpty(user.TotLivArea1) || !string.IsNullOrEmpty(user.TotLivArea2))
        {
            //sbSQL.Append(sqlWhereCont +"PropertyAddress = '" + user.Address + "'");
            //sqlWhereCont = "AND ";
            sbSQL.Append(sqlWhereCont + "crb.vResBldgDep_tla_value >=" + user.TotLivArea1 + " And crb.vResBldgDep_tla_value <=  " + user.TotLivArea2 );
            sqlWhereCont = "AND ";
        }
        if (!string.IsNullOrEmpty(user.LotArea1) || !string.IsNullOrEmpty(user.LotArea2))
        {
            //sbSQL.Append(sqlWhereCont +"PropertyAddress = '" + user.Address + "'");
            //sqlWhereCont = "AND ";
            sbSQL.Append(sqlWhereCont + "lm.facres >=" + user.LotArea1 + " And lm.facres <= " + user.LotArea2 );
            sqlWhereCont = "AND ";
        }

        if (!string.IsNullOrEmpty(user.Bedrooms))
        {
            sbSQL.Append(sqlWhereCont + "crb.frmbed = '" + user.Bedrooms + "'");
            sqlWhereCont = "AND ";
        }
        if (!string.IsNullOrEmpty(user.SalePrice1) || !string.IsNullOrEmpty(user.SalePrice2))
        {
            //sbSQL.Append(sqlWhereCont +"PropertyAddress = '" + user.Address + "'");
            //sqlWhereCont = "AND ";
            sbSQL.Append(sqlWhereCont + "pt.Price >=" + user.SalePrice1 + " And pt.Price <=  " + user.SalePrice2 );
            sqlWhereCont = "AND ";
        }
        if (!string.IsNullOrEmpty(user.SaleDate))
        {
            sbSQL.Append(sqlWhereCont + "pt.SaleDate = '" + user.SaleDate + "'");
            sqlWhereCont = "AND ";
        }
        if (!string.IsNullOrEmpty(user.AssesVal1) || !string.IsNullOrEmpty(user.AssesVal2))
        {
            //sbSQL.Append(sqlWhereCont +"PropertyAddress = '" + user.Address + "'");
            //sqlWhereCont = "AND ";
            sbSQL.Append(sqlWhereCont + "crb.vResBldgDep_tla_value >=" + user.AssesVal1 + " And crb.vResBldgDep_tla_value <= " + user.AssesVal2 );
            sqlWhereCont = "AND ";
        }
        if (!string.IsNullOrEmpty(user.Style))
        {
            sbSQL.Append(sqlWhereCont + "crb.fmsstyle = '" + user.Style + "'");
            sqlWhereCont = "AND ";
        }
        if (!string.IsNullOrEmpty(user.ArchStyle))
        {
            sbSQL.Append(sqlWhereCont + "srb.farchstyle = '" + user.ArchStyle + "'");
            sqlWhereCont = "AND ";
        }
        if (!string.IsNullOrEmpty(user.TaxUnit))
        {
            sbSQL.Append(sqlWhereCont + "pg.TaxingUnitGroupCode = '" + user.TaxUnit + "'");
            sqlWhereCont = "AND ";
        }
        sbSQL.Append(" ORDER BY ");
        sbSQL.Append(" pg.QuickRefID ");


        //// populate a list of CustomerModel objects from database
        string MyConnectionString = ConfigurationManager.ConnectionStrings["WLConnection"].ConnectionString;
        System.Data.SqlClient.SqlConnection cnn = new System.Data.SqlClient.SqlConnection(MyConnectionString);
        System.Data.SqlClient.SqlCommand cmd = new System.Data.SqlClient.SqlCommand(sbSQL.ToString(), cnn);
        cmd.CommandTimeout = 30000;
        DbCmd.SelectCommand = cmd;
        DbCmd.Fill(tempDS, "ResultSet");
        DataTable resultSet = tempDS.Tables["ResultSet"];
        var vm = new List<BedroomModel>();
       foreach (DataRow dr in tempDS.Tables[0].Rows)
        {
            vm.Add(new BedroomModel 
            {
                BedroomA = dr.ItemArray[12].ToString(),
                YearBuilt = dr.ItemArray[9].ToString(),
                LivingArea = dr.ItemArray[7].ToString(),
                LotArea = dr.ItemArray[3].ToString(),
                SaleDateA = dr.ItemArray[8].ToString(),
                SalePriceA = dr.ItemArray[10].ToString(),
                AssesVal = dr.ItemArray[5].ToString(),
                StyleA = dr.ItemArray[3].ToString(),
                ArchStyleA = dr.ItemArray[4].ToString(),
                ParcelId = dr.ItemArray[0].ToString(),
                QuickRefId = dr.ItemArray[1].ToString(),
                TaxunitA = dr.ItemArray[6].ToString(),
                Address = dr.ItemArray[2].ToString(),
                ValCode = dr.ItemArray[5].ToString(),


             });
          }
        //DbCmd.Fill(dtFindResults);
        //var x = dtFindResults.Rows.Count;
        cnn.Close();
       return View("Result",vm);
        //// return the list of CustomerModel objects to our View
        //return View("Result", resultSet);
        //return View(ViewBag.data);
    }

In a word, *no*. Use parameterized queries. Always. You might build the query itself dynamically based on user input, but the values coming from the user should always be passed as parameters, not concatenated into the query directly. — Tim M., Jan 07 '15 at 21:58
You are building a SQL string on the fly based on user input. This is the de facto example of how *not* to guard against SQL injection. — Preston Guillot, Jan 07 '15 at 21:58
any suggestions on how to build it dynamically and secure? @PrestonGuillot — Logan Jones, Jan 07 '15 at 22:01

score 5 · Accepted Answer · answered Jan 07 '15 at 22:10

5

Parameterized queries are a must. However, that doesn't prevent you from being able to build dynamic queries, you just need to approach it differently.

It's fine to make decisions based on user input; it's not fine to concatenate those values into a query.

A quick example:

using( IDbCommand cmd = GetCommand() )
{
    string lotSize = "12345";
    bool includeLotSize = !string.IsNullOrWhiteSpace( lotSize );

    var sb = new StringBuilder();
    sb.AppendLine( "SELECT Col1, Col2 FROM dbo.Foo" );

    // you might also vary the columns returned based on what the user asked for

    if( includeLotSize )
    {
        sb.AppendLine( "WHERE LotSize = @LotSize" );

        // The query will expect the lot size, so add a parameter here to pass 
        // the lot size value.
        cmd.Parameters.Add( new SqlParameter( "LotSize", lotSize ) );
    }
}

Note that many of your string properties look like they could be a more specific type (int, float, an int pointing to a database lookup, etc). This does not prevent SQL injection but it can be used for validation (as well as making your view models cleaner).

Also note that there are many different ways to connect to a database in .Net, but be sure that you are disposing of your resources properly (note the using statement I've added).

answered Jan 07 '15 at 22:10

Tim M.

53,671
14
120
163

Thank you Tim, this is the first time im working out on the web and i dont think we will be attacked but i dont want to take the chance ,can i still use the model to input the sql? like string lotsize = user.lotsize; – Logan Jones Jan 07 '15 at 22:14
1

You can continue to use the model to collect user input. The point is to only pass user input to the database as a parameter, never as a raw string. You should be able to use the pattern in my answer to replace code like `sbSQL.Append(sqlWhereCont +"crb.fyrblt >="+ user.YrBlt1+ " And crb.fyrblt <= " + user.YrBlt2 );` – Tim M. Jan 07 '15 at 22:16
Tim if i use the a string within the method, wouldnt that still allow sql injections since i would still get user input? @Tim Medora – Logan Jones Jan 08 '15 at 16:57
1

@LoganJones - SQL injection comes from using untrusted user input in a query. When you pass it as a parameter, it is made safe by the framework. However, it typically is safe to read even a malicious string in c# (which resists [buffer overflows](http://stackoverflow.com/questions/9343665/are-buffer-overflows-possible-in-c) and such). – Tim M. Jan 08 '15 at 18:28
awesome thanks tim ill post later to make sure i am getting all this correctly – Logan Jones Jan 09 '15 at 17:39

MVC 4 SQL string injection Security

1 Answers1