2

There is a developer that is ostinated to say that he has developed a function that sanitize any input from SQL injection. this is the method he has done:

public static string SanitizeInput(string sqlString, bool addQuotes)
{
    string output= "";
    if (addQuotes)
        output= sqlString.Replace("'", "''");
    else
        output= val;
    if (output== "")
        output= "NULL";
    else if (addQuotes)
        output= "'" + output+ "'";
    return output;
}

and it's used like the following:

string sqlCmd = string.Format("EXEC myStoredProc @paramX={0}",Utils.SanitizeInput(sqlInputFromUser,true));

DataTable dt = new DataTable();

using (SqlConnection sqlConn = new SqlConnection())
{
    sqlConn.ConnectionString = ConfigurationManager.ConnectionStrings[ConnectionString].ToString();
    sqlConn.Open();
    SqlCommand cmd = new SqlCommand(sqlCmd, sqlConn);
    SqlDataAdapter sda = new SqlDataAdapter(cmd);
    sda.Fill(dt);
}

Am I correct to say that this is, in any case, a bad practice because there is no way of foreseeing what the users will pass as parameters and they can find a way to break it and inject malicious SQL code?

**** EDIT: Additional infos ****

Some additional replies from the developers that maybe be worth mentioning:

  1. The Azure WAF does not allow anything SQL related including punctuation marks
  2. All SQL that involves user input is queried from the database using strongly typed stored procedures
  3. All user input is sanitized by a function

Points No. 2 and 3 are wrong, as already discussed in the comments. I could just add a string like "'; DROP database ..." to bypass his sanitization and strong typed stored procedures

But regarding point 1, is it possible that there is an Azure WAF that block on a web page (it's an MVC web application) if the user inputs a string like the one above where I've put a ";" to break the query?

Giox
  • 4,785
  • 8
  • 38
  • 81
  • 6
    Erm... That does nothing to stop an injection. Use named parameters, it's what they're there fore – canton7 Dec 22 '21 at 08:59
  • 5
    There's more to parametrising than avoiding Injection. For example, plans can be cached for the query, rather than a new one being generated **every time** a statement is run. Even *if* your developer has found some "magic" way to stop injection, that doesn't mean it's the right solution. Tell them to parametrise, and if they don't like it... Well in my team all of their PR's are going to get rejected, and they could well find themselves not working on my team any more. – Thom A Dec 22 '21 at 09:00
  • 3
    there are only two ways to safely protect against SQL injection. parameterised queries, and whitelisting parameters for the few things you can't parameterise. everything else is just occupational therapy. (plus: mention the _other_ benefits of parameterised queries. less weird syntax errors, cleaner code, better performance... while the above method will result in the _opposite_) – Franz Gleichmann Dec 22 '21 at 09:02
  • thats is really too short blacklist aproach to counter-sqlinjection, that firstly is a bad idea (always is a clever way to bypass a blacklist) As everyone would say the RIGHT aproach is parametrise, and if you want to extra security you can ADD a black list above it (or better,a White list) – J.Salas Dec 22 '21 at 09:06
  • In the case of executing procedures, it's actually *easier* to use parameters, as you don't need to write any `EXEC` code, you just do `new SqlCommand(procName, sqlConn) {CommandType = CommandType.StoredProcedure}` Let's not get into the obvious lack of `using` blocks to ensure the connection is closed, or question why you would want to use an adapter and datatable anyway – Charlieface Dec 22 '21 at 09:26
  • 2
    The point 1 makes me laugh! WAF stopping things like "O'Ryordan" (the single quote) o "Select your color" or "Drop tha bomb" or ... thats nonsense – J.Salas Dec 22 '21 at 10:03
  • 2
    @Glox instead of trying to find extra tools to cover up SQL injection, avoid it to begin with. Parameterized queries are *easier* to write than all this string concatenation. You'd need *less* code than what you already used to execute a parameterized query. And if you use a library like [Dapper](https://github.com/DapperLib/Dapper) all this code can be replaced by a single line. – Panagiotis Kanavos Dec 22 '21 at 10:05

0 Answers0