How to generate strings that are not vulnerable to SQL injection

Question

I have a utility that generates query strings, but the static code analyzers (and my coworkers) are complaining because of risk of "SQL Injection Attack".

Here is my C# code

    public static string[] GenerateQueries(string TableName, string ColumnName)
    {
        return new string[] {
            "SELECT * FROM " + TableName,
            "SELECT * FROM " + TableName + " WHERE 1=2",
            "SELECT * FROM " + TableName + " WHERE [" + TableName + "Id] = @id",
            "SELECT * FROM " + TableName + " WHERE [" + TableName + "Id] = IDENT_CURRENT('" + TableName + "')",
            "SELECT * FROM " + TableName + " WHERE [" + ColumnName + "] = @value"
        };
    }

In the code I always call it only with constant strings, such as

var queryList = GenerateQueries("Person", "Name");

Is there any way to rewrite this function so that it is "safe"? For example, if I were using C instead of C#, I could write a macro to generate the strings safely.

At the moment, the only choice I have is to copy/paste this block of SELECT statements for every single table, which is ugly and a maintenance burden.

Is copy/paste really my only option?

EDIT:

Thank you for the replies, esp William Leader. Now I see that my question is wrong-headed. It isn't just the fact that I am concatenating query strings, but also storing them in a variable. The only proper way to do this is to construct the SqlDataAdapter using a constant such as,

var adapter = new SqlDataAdapter("SELECT * FROM PERSON");

There is no other choice. So yes, there will be a lot of copy/paste. I'm starting to regret not using EF.

Create a `HashSet` with a white-listed set of table names. Throw an exception if `TableName` is not in the `HashSet`. Repeat for `ColumnName`. — mjwills, Sep 24 '18 at 21:46
"I always call it only with constant strings" <- This right here is a problem. Just because you only do it that way doesn't mean someone else won't do it that way. I'm not just talking about co-workers either. Secure thinking means you need to assume that all other security protections have been broken, which means you might have malicious code that is calling your function in ways you never intended. The idea that 'I don't use it that way' is not the same as 'No one will ever use it that way.' — William Leader, Sep 24 '18 at 22:02
If you were actually calling the queries instead of passing back the strings I would suggest using `QUOTENAME` to escape the table and column names. [Here is a answer I wrote](https://stackoverflow.com/questions/40511346/allowing-a-user-to-pass-table-name-and-column-name-whilst-preventing-sql-injecti/40512080#40512080) that uses it for another question. — Scott Chamberlain, Sep 25 '18 at 03:16

score 2 · Accepted Answer · edited Jun 20 '20 at 09:12

2

I was shocked at first, but on reflection this is no different than having an SQL statement already in your code that looks like this:

"SELECT * FROM Person"

We do that kind of thing all the time.

IF

There's an important caveat here. That only remains true if you can control how the function is called. So if this method is a private member of a data layer class somewhere, you might be okay. But I also wonder how useful this really is. It seems like you're not saving much over what you'd get from just writing the queries.

Additionally, it's not good to be in the habit of ignoring your static analysis tools. Sometimes they give you stuff you just know is wrong, but you change it anyway so that when they do find something important you're not conditioned to ignore it.

edited Jun 20 '20 at 09:12

Community

1
1

answered Sep 24 '18 at 22:10

Joel Coehoorn

399,467
113
570
794

I'm personally in agreement with this when read bottom to top (sort of) I can think of half a dozen better ways to generically access the data through say EF, but I can recall one example where building a giant SQL insert command was all that worked for batch inserting into an XML Column with EF Core for something like 40k records. In short, try everything else first (imo) bt sometimes it might just make the most sense when you have ruled out more common practices. – Austin T French Sep 24 '18 at 22:54
This does not address the issue of SQL injection vulnerability – Bill Richards Apr 10 '19 at 00:41

Bill Richards · Answer 2 · 2018-09-24T23:33:42.920

What your Code analyser is telling you is that you should most likely be calling a procedure with some parameters instead of sending SQL across the wire.

It does not mater a single bit whether or not you use a macro to generate your SQL statements, if you are sending raw SQL across the wire you are open to SQL Injection Attacks

Sending SQL commands to an endpoint making a non sanctioned call. If we fire up a network packet sniffer, we can see that you have a database configured to allow SQL commands to be sent, so we can inject illegal SQL into the system

You could still rely on a single procedure for calling your updates, but if you elect to move to procedures, why would you want to do that?

EDITED to provide an example

create PROC sp_CommonSelectFromTableProc @tableName varchar(32)
AS
   -- code to check the tableName parameter does not contain SQL and/or is a valid tableName

   -- your procedure code here will probable use 
   --  exec mydynamicSQLString  
   -- where mydynamicSQLString is constructed using @tableName  
END;

or maybe a table specific procedure

create PROC sp_SelectFromSpecificTableProc 
AS
   SELECT * FROM SpecificTable
END;

What is important to remember is that SQL injection is independent of the technology used for the underlying application.

It is just overt when the application contains such constructs as

return new string[] {
            "SELECT * FROM " + TableName,
            "SELECT * FROM " + TableName + " WHERE 1=2",
            "SELECT * FROM " + TableName + " WHERE [" + TableName + "Id] = @id",
            "SELECT * FROM " + TableName + " WHERE [" + TableName + "Id] = IDENT_CURRENT('" + TableName + "')",
            "SELECT * FROM " + TableName + " WHERE [" + ColumnName + "] = @value"

SQL Injection must be addressed at both ends of the data channel.

Here is a pretty good starting point for understanding how to mitigate for SQL Injection attacks

How to generate strings that are not vulnerable to SQL injection

2 Answers2

IF