SQL Server and SQL Injection

Question

SQL injection: isn't replace("'", "''") good enough?

I am wonder if replace "'" with "''" can prevent sql injection. I don't like it, but as the person in the original question, i have inherits a code base where that "bad practice" was used.

I read that post and am not sure if a sql injection for SQL Server is possible or not (seems a bit controversial answer).

So i would ask if someone can write a select ("escaped" in that way), that would finally fail agains a SQL Injection. If not possible i would assume it's safe.

---EDIT (added example extrapolated from real code, names are fictional due NDA but structure is the same) :

C# Code

string sql = $@"SELECT [FIELD1] ,[FIELD2], [FIELD3] 
                FROM [MY_TABLE]
                WHERE [FIELD1] = '{UtilityBase.ChkString.(field1, "'")}'";

sql the is used here

using (System.Data.SqlClient.SqlDataAdapter xDtAdpt = new System.Data.SqlClient.SqlDataAdapter(StrSql, Conn))
{
    RSDataSet = new System.Data.DataSet();
    RSDataSet.EnforceConstraints = false;
    xDtAdpt.Fill(RSDataSet);
    RSDataSet.EnforceConstraints = true;
    xDtAdpt.Dispose();
}

The check string is :

public static string ChkString(object xString, string xSeparator = "")
{
    try
    {
        if (string.isNullOrEmpty(xString))
        {
            return "NULL";
        }
        else
        {
            return xSeparator + xString.ToString().Replace("'", "''") + xSeparator;
        }
    }
    catch
    {
        return "";
    }
}

It stop injection in *some* places, not all. For example replacing single quotes (`'`) when injecting dynamic objects would do nothing to stop injection. If you have parameters, you should be parametrising them; that is always the safest way. — Thom A, Mar 08 '21 at 13:53
USE SQL parameters, there are tons of articles on how to avoid SQL injection and many are super easy to implement especially if starting something from scratch. — Brad, Mar 08 '21 at 13:57
i would only know if it's safe to use with user input, when user input is string.I know is bad practice, and i don't like it, but i can't make a request for software rewrite (with it's cost) if there is a concrete security risk. So i need a POC to ask a rewrite of that legacy code. If you can write that POC please answer my question, i'm glad to accept it — Skary, Mar 08 '21 at 14:09
Apart from specific issues surrounding weird Unicode conversions, I suppose the simple answer is: the risk is real, and how can we guarantee that `Replace` was used everywhere in order to mitigate that risk? The only way to verify that is to check all the code, at which point it's not that much more effort to parameterize at the same time. — Charlieface, Mar 08 '21 at 14:19
If you have a question about a specific example, please post that. How is your SQL being executed? Are you concatenating parameters into a string within a procedure? Are you building a string dynamically in code and directly executing? Examples please. — Stu, Mar 08 '21 at 15:21
@sTTU : you right, i have added piece of the code involved with query. I have anonymized real query and posted tiny piece of all the actual code due to NDA — Skary, Mar 09 '21 at 11:02
What you are doing is typically referred to an anti pattern, I would recommend parameterising your sql query or moving the code to a sql procedure and passing parameters using something like Dapper. https://xkcd.com/327/ — Stu, Mar 09 '21 at 11:10
Also, there is no point returning null for an equality comparision, your SQL needs to handle that with `is null` - unless your data literally has NULL stored as a string! — Stu, Mar 09 '21 at 11:16
@sTTu is pretty clear to me that's bad and i would change that legacy coda that now i have to mantain (but that i have not written). My boss allow me to spend time changing the code only if i can prove it has security problem, then my question. Did you see a security problem? — Skary, Mar 09 '21 at 11:26
The answer to your question is yes. I would suggest you employ the services of a penetration testing company, and be prepared for some ugly surprises ;) — Stu, Mar 09 '21 at 11:30
Maybe my eyes are crossing, but it looks like `ChkString` doubles any apostrophes (`'`) and adds the specified `xSeparator` as a prefix and suffix. `UtilityBase.ChkString.(field1, "'")` would take `; drop table users; --` and change it to `'; drop table users; --'`. Insert that in `WHERE [FIELD1] = '{UtilityBase.ChkString.(field1, "'")}'` and you get `WHERE [FIELD1] = ''; drop table users; --''`. Is there a problem yet? — HABO, Mar 09 '21 at 22:42
@HABO you are right, thanks for the help, exactly what i was looking for. Sorry but i was not good in find SQL Injection and have totally miss that. Now i have an argument. — Skary, Mar 24 '21 at 08:52

score 0 · Answer 1 · answered Mar 09 '21 at 11:36

You should recommend to your boss to utilise the expertise of a security penetration testing company. This is something that should be done on a regular basis as a best practice anyway, I guarantee they will find many vulnerabilities that you are not aware of, including some cunning sql injection attacks.

Trust me, however good you think your string sanitization is, from experience there is only one safe way to pass data to the database.

score -2 · Answer 2 · answered Mar 08 '21 at 20:07

-2

"i have inherits a code base where that "bad practice" was used."

This is not a "bad" practice. It is a FATAL practice. SQL injection is STILL the number one vulnerability.

You MUST take the time to parameterize your sql. Otherwise, even the script kiddies (novice hackers) will be able to access your site. Or, very possibly, already have.

Just my opinion. I could be wrong.

answered Mar 08 '21 at 20:07

jim

401
4
10

I am reading a lot about that topic in that days. I have the suspicious that SQL injection are lastly not possible with that "bad" practice. This seems to me only a bad practice because the programmer need to be very carefull to "sanitize" each input. So i need to be sure about the security and for that reason i made that question. What i am looking for is an example of injection (in my scenario) or otherwise some documentation that clearly state that this approach is faseable. – Skary Mar 09 '21 at 11:10
@Skary. I have to live with a fire breathing, spine snapping security officer. So, I don't even consider not using parameterized queries. We have scanners on our code looking for sql injection vulnerabilities as well as other vulnerabilities. Microsoft has recommended the practice of parameterizing for a minimum of 16 years. And, with all of that, I was shocked to find that SQL injection is STILL the leading -- i.e. most hacked -- vulnerability. – jim Mar 09 '21 at 21:58
@Skary Google "sql smuggling". Hackers have nothing better to do than to figure out how to bypass attempts to stop them. – jim Mar 09 '21 at 22:17

SQL Server and SQL Injection

2 Answers2