What will be the best practices in my code to prevent sql injection?

Question

What will be the best practices to prevent sql injection? My client asked me to prevent sql injection. I used this structure for data inserting or updating

public bool Add(GreenItem aGreenItem, Employee emp)
        {
            aGreenItem.GreenItemCode = new CommonBLL().GetMaxId("[GreenItemCode]", "[Processing].[GreenItem]", "GTM");
            using (SqlConnection objConnection = Connection.GetConnection())
            {
                SqlTransaction transaction = objConnection.BeginTransaction("SampleTransaction");
                try
                {
                    string query = aGreenItem.GreenItemId == 0 ? "GreenItem_Create" : "GreenItem_Update";
                    SqlCommand sqCmd = new SqlCommand(query, objConnection, transaction);
                    sqCmd.CommandType = CommandType.StoredProcedure;
                    if (aGreenItem.GreenItemId > 0)
                    {
                        sqCmd.Parameters.AddWithValue("@GreenItemId", aGreenItem.GreenItemId);
                    }
                    else
                    {
                        sqCmd.Parameters.AddWithValue("@GreenItemCode", aGreenItem.GreenItemCode);
                    }

                    sqCmd.Parameters.AddWithValue("@GreenItemName", aGreenItem.GreenItemName);
                    sqCmd.Parameters.AddWithValue("@MeasurementUnitId", aGreenItem.MeasurementUnitId);                    
                    sqCmd.Parameters.AddWithValue("@Description", aGreenItem.Description);
                    sqCmd.Parameters.AddWithValue("@IsActive", aGreenItem.IsActive);
                    sqCmd.Parameters.AddWithValue("@GLTId", emp.GLTId);
                    sqCmd.Parameters.AddWithValue("@CreatorId", emp.EmployeeId);
                    sqCmd.ExecuteNonQuery();
                    transaction.Commit();
                    return true;
                }
                catch
                {
                    transaction.Rollback();
                    return false;
                }
            }
        }

I used this function to get the Max ID Which is called from the function above

public string GetMaxId(string coloumName, string tableName, string prefix)
        {
            string maxId = ""; string selectQuery = "SELECT '" + prefix + "'+RIGHT('0000000000'+ CONVERT(VARCHAR,ISNULL(MAX(RIGHT(" + coloumName + ",10)), 0)+1,10),10) maxID FROM " + tableName + " ";
            using (SqlConnection objConnection = Connection.GetConnection())
            {
                SqlCommand sqCmd = new SqlCommand(selectQuery, objConnection); sqCmd.CommandType = CommandType.Text;
                using (IDataReader dataReader = sqCmd.ExecuteReader())
                {
                    while (dataReader.Read())
                    {
                        maxId = dataReader["maxID"].ToString();
                    }
                }
                objConnection.Close();
            }

            return maxId;
        }

What needs to be added to for best output?

You are already preventing SQL Injection, by using SqlParameters instead of manipulating the string directly. Good Job. We can't see what CommonBLL().GetMaxId() does though, maybe post that too — Milney, Sep 01 '20 at 10:20
Using parametrised queries and **Safely** quoting dynamic objects (if you have them) is the best way. On a separate note, [Can we stop using AddWithValue() already?](https://blogs.msmvps.com/jcoehoorn/blog/2014/05/12/can-we-stop-using-addwithvalue-already/) & [AddWithValue is Evil](https://www.dbdelta.com/addwithvalue-is-evil/) — Thom A, Sep 01 '20 at 10:21
OK - there is definitely scope for SQL injection in GetMaxId, since you're assembling the SQL statement with strings passed through as parameters without any escaping, It would be better to use parameters as you have in Add(). However if you're only ever going to call GetMaxId() with hard-coded strings in the app, and never with strings the user can influence, you might get away with this. — Rup, Sep 01 '20 at 10:30
I always called this function on another function and passed fixed parameters @Rup — touhidalam69, Sep 01 '20 at 10:33
That statement, however, is wide open to injection, @touhidalam69 . You have a dynamic table name that you are not sanitising, and are injecting values instead of parametrising them.. — Thom A, Sep 01 '20 at 10:41
Do we already have a canonical question that explains why `catch { /* throw away all exception information */ ... return false; }` is a terrible crime (committed against the poor soul who will have to provide customer support for that application)? — Heinzi, Sep 01 '20 at 10:44

score 3 · Accepted Answer · answered Sep 01 '20 at 10:28

With SQL Server, avoiding SQL injection comes down to one simple thing

use parameters, rather than concatenation, for all inputs

You're already doing that in the code that we can see, so: great job so far.

People often mistakenly say "use stored procedures" to avoid SQL injection, but "stored procedures" and "SQL injection" are actually entirely orthogonal - you can avoid SQL injection without stored procedures, and you can cause SQL injection inside stored procedures. We can't see what GreenItem_Create / GreenItem_Update do internally - they're probably fine if they are simple INSERT / UPDATE operations. As long as they don't do EXEC (@somethingYouConcatenated) internally, you should be fine. If you do ever need to build T-SQL inside T-SQL, make sure to use sp_ExecuteSQL to correctly parameterize that dynamic SQL.

What will be the best practices in my code to prevent sql injection?

1 Answers1