none
ADO.net and SLQ Injection RRS feed

  • Question

  • Could you please let me know, if there are any cons and any sql injection related issues with the ado.net c# code below-

     public long InsertData(string tableName, Dictionary<string, string> insertKeyValPairs)
            {
                long primaryKeyNumber;
                StringBuilder sqlParams = new StringBuilder();
                StringBuilder sql = new StringBuilder();
                sql.Append("INSERT INTO dbo.");
                sql.Append(tableName);
                sql.Append("(");

                foreach (var item in insertKeyValPairs)
                {
                    sql.Append((string.IsNullOrEmpty(sql.ToString()) == false) ? "," : "");
                    sql.Append(item.Key);

                    sqlParams.Append((string.IsNullOrEmpty(sqlParams.ToString()) == true) ? "@" : ",@");
                    sqlParams.Append(item.Key);
                }
                sql.Append(") VALUES(");
                sql.Append(sqlParams.ToString());
                sql.Append(")");

                using (SqlConnection cn = new SqlConnection(mConString))
                {
                    using (SqlCommand cmd = new SqlCommand(sql.ToString(), cn))
                    {  // define parameters and their values
                        foreach (var item in insertKeyValPairs)
                        {
                            cmd.Parameters.Add(item.Value);
                        }
                        // open connection, execute INSERT, close connection
                        cn.Open();
                        cmd.ExecuteNonQuery();

                        cmd.CommandText = "SELECT SCOPE_IDENTITY() AS 'Identity'";
                        DataTable dtData = new DataTable();
                        dtData.Load(mSqlCommand.ExecuteReader());
                        primaryKeyNumber = Convert.ToInt64(dtData.Rows[0][0]);

                        cn.Close();
                    }
                }
                return primaryKeyNumber;
            }

    Tuesday, April 16, 2019 7:02 PM

Answers

  • Yes your code is a candidate for SQL injection attacks. The easiest way to determine if this is true is to ask the simple question "do you at any point in your query building put a "user provided" value directly into your SQL command. If the answer is yes then you're code is opening the door.

    Take a look at your first foreach loop. It is enumerating through the user provided dictionary of key-value pairs. For each pair it is inserting the key and then the value. Since the user is providing these values they can put in anything they want. Note that it is completely irrelevant that you're putting their values into the middle of a clause because a single SQL command can have any # of clauses. So imagine your final SQL command as this:

    INSERT INTO dbo.{table} ( @{key1}, @{key2}, @{key3} ) VALUES (@{key1}, @{key2}, @{key3})

    Now imagine what happens if I set `table` to this `.ValidTable (Name) VALUES ('Hello'); DELETE FROM ValidTable; --`. This would be the SQL generated

    INSERT INTO dbo.ValidTable (Name) VALUES ('Hello'); DELETE FROM ValidTable; -- ( @{key1}, @{key2}, @{key3} ) VALUES (@{key1}, @{key2}, @{key3})
    I just wiped your table.


    Michael Taylor http://www.michaeltaylorp3.net

    • Marked as answer by Aryaa Thursday, April 18, 2019 10:12 PM
    Wednesday, April 17, 2019 2:33 PM
    Moderator

All replies

  • As long as you're using parametrized T-SQL, it midigates SQL injection attack.

    However, a better alternative to prevent SQL injection attacks is to use Linq with an ORM such as ADO.NET Entity Framework.

    http://www.devx.com/dotnet/Article/34653

    And getting the primary-key of a table using Identity is much more simpler with other benefits of using an ORM as opposed the dataset and datatable, like in general using a custom type and a collection is a better alternative.

    https://dzone.com/articles/reasons-move-datatables

    Tuesday, April 16, 2019 9:19 PM
  • Yes your code is a candidate for SQL injection attacks. The easiest way to determine if this is true is to ask the simple question "do you at any point in your query building put a "user provided" value directly into your SQL command. If the answer is yes then you're code is opening the door.

    Take a look at your first foreach loop. It is enumerating through the user provided dictionary of key-value pairs. For each pair it is inserting the key and then the value. Since the user is providing these values they can put in anything they want. Note that it is completely irrelevant that you're putting their values into the middle of a clause because a single SQL command can have any # of clauses. So imagine your final SQL command as this:

    INSERT INTO dbo.{table} ( @{key1}, @{key2}, @{key3} ) VALUES (@{key1}, @{key2}, @{key3})

    Now imagine what happens if I set `table` to this `.ValidTable (Name) VALUES ('Hello'); DELETE FROM ValidTable; --`. This would be the SQL generated

    INSERT INTO dbo.ValidTable (Name) VALUES ('Hello'); DELETE FROM ValidTable; -- ( @{key1}, @{key2}, @{key3} ) VALUES (@{key1}, @{key2}, @{key3})
    I just wiped your table.


    Michael Taylor http://www.michaeltaylorp3.net

    • Marked as answer by Aryaa Thursday, April 18, 2019 10:12 PM
    Wednesday, April 17, 2019 2:33 PM
    Moderator
  • Hi Michael Taylor

    Thank you for the detail explanation.


    • Edited by Aryaa Thursday, April 18, 2019 10:12 PM spelling mistake
    Thursday, April 18, 2019 10:11 PM