How to identify where SQL injection is?

Question

I have been challenged with the task of reducing SQL injection in somebody elses code however the tool that im using to scan my code doesnt give an explanation on where the injections are, as it just points out the method. Take the following code as an example

public static CommandStatus<int> CreateTable(SqlConnection connection, String tableName, SQLColumn[] columns)
{
    string colSQL = columns.Select(f => f.ToString()).Aggregate((f1, f2) => f1 + ",\n" + f2);


    if(columns.FirstOrDefault(e => e.IsPK()) != null){
        string constraint = GenerateTableConstraint(tableName, columns.FirstOrDefault(e => e.IsPK()).ColumnName());
        colSQL += constraint;
    }

    string sql = "CREATE TABLE " + ParseTableName(tableName) + " (" + colSQL + ") ON [PRIMARY]";

    SqlCommand cmd = new SqlCommand();
    cmd.CommandType = CommandType.Text;
    cmd.CommandText = sql;
    cmd.Connection = connection;
    cmd.CommandTimeout = GetTimeout();

    try
    {
        int result = cmd.ExecuteNonQuery();

        return new CommandStatus<int>(result);
    }
    catch (Exception e)
    {
        return new CommandStatus<int>("Failed to create tableName " + tableName + " [SQL: " + sql + "]:" + e.Message, e);
    }
}

The ouput from colSQL is

"[RecordID] [NVARCHAR](255) NULL,\n[Address1] [NVARCHAR](255) NULL,\n[Address2] [NVARCHAR](255) NULL,\n[Address3] [NVARCHAR](255) NULL,\n[Address4] [NVARCHAR](255) NULL,\n[Address5] [NVARCHAR](255) NULL,\n[MongoId] [NVARCHAR](100) NOT NULL"

SQLColumns[] columns is just an array of the following

Record
nvarchar(255)
null

My understanding of the inject is that surrounding a string with "[]" makes it safe. IF this is not the case how would I go about making this method SQL inject free.

Related: [How does SQL-injection work and how do I protect against it](https://stackoverflow.com/q/5721786/1364007); [What is SQL injection?](https://stackoverflow.com/q/2216107/1364007); [What are good ways to prevent SQL injection?](https://stackoverflow.com/q/14376473/1364007). — Wai Ha Lee, Jan 03 '20 at 17:04
Quoting is not as simple as wrapping in `[]`. If the string contains `]`, you're still vulnerable. A [string](https://xkcd.com/327/) like `"Robert]; DROP TABLE Students;--"` would be an injection. Note that I've used the same technique as the comic but with a different closing quote character. (To do it correctly, you have to double the closing quote character, so `"[" + name.Replace("]", "]]") + "]"`.) — madreflection, Jan 03 '20 at 17:07
@madreflection to confirm double quotes eliminate sql injections? — LeahKnowsnoBounds, Jan 03 '20 at 17:11
_"confirm double quotes eliminate sql injections?"_ Oh my no! madreflection suggestion is a best practice for dealing with _one_ form of SQL injection. Plus methods of attack can change.In my opinion, @WaiHaLee suggestion is the correct direction. Another alternative is using stored procedures. In this scenario, the "sanitizing" of user input is done at the db level. — Shai Cohen, Jan 03 '20 at 17:45
Eliminate? No. I refuse to put that thought in your head. And I didn't say "double quotes". I said "double the closing quote character". Those are not the same thing. Note that the square brackets are quote characters in SQL Server. No, you need to analyze every input and every concatenation, and even then you might not get it right. My comment was simply a myopic note about what you were doing, a warning. — madreflection, Jan 03 '20 at 17:46
Unfortunately, the syntax you're using is not parameterizable, and stored procedures would still have to use dynamic SQL to do it, which then needs to go through injection mitigation using the T-SQL language. You're on thin ice either way. Be very careful. Consider using SMO (server management objects). — madreflection, Jan 03 '20 at 17:50
There is a possibility for injection in `string sql` because any string can be passed for `tableName` and `columns`. I don't know what `GenerateTableConstraint` does but it may be vulnerable to injection too depending on its implementation. — CodingYoshi, Jan 03 '20 at 18:20
Anothing thing you should keep in mind, not related to injection: in a database configured with a *case-senstive collation*, `[NVARCHAR]` is invalid because the type name is lowercase. If you quote it, it must be lowercase: `[nvarchar]`. Without the quotes (brackets), it's parsed as a keyword that maps to the type name, which is actually `[sys].[nvarchar]`. Same with all built-in types. Query the `sys.types` catalog view and use the exact case in the `name` column when quoting type names. — madreflection, Jan 03 '20 at 18:27
To avoid injection use a parameterized query and pass both `tablename` and each item from `columns` as parameters. — CodingYoshi, Jan 03 '20 at 18:38
@CodingYoshi: A `CREATE TABLE` statement can't be parameterized. The statement has to be constructed, whether it's in C# or T-SQL, so you still end up having to mitigate injection using techniques that involve string manipulation. That's why I suggested SMO: it will generate and execute proper SQL from an object-graph representation of the table structure. It knows all the nuances and pitfalls and how to avoid them. — madreflection, Jan 03 '20 at 18:45
Thanks @madreflection, the fact this is dynamically constructed has made it difficult to see a solution, I will read into SMO and see what I can find. — LeahKnowsnoBounds, Jan 05 '20 at 20:52

score 0 · Accepted Answer · answered Jan 03 '20 at 20:16

0

If you're using Visual Studio, then you can run source code analysis on the solution and it will raise a CA3001 warning when it detects troublespots.

CA3001: Review code for SQL injection vulnerabilities

https://docs.microsoft.com/en-us/visualstudio/code-quality/ca3001

https://docs.microsoft.com/en-us/visualstudio/code-quality/code-analysis-for-managed-code-overview

answered Jan 03 '20 at 20:16

Jeff Barnard

94
5

Thanks for this, I put double the closing quotes and install the NuGet package and it now doesn't bring up CA3001 warning, many others but not this. A bit concerning because of im not convinced I've done enough to ensure that SQL injection no longer exist but I'm not one to argue with a Microsoft recommended package such as FxCop. – LeahKnowsnoBounds Jan 07 '20 at 16:18

How to identify where SQL injection is?

1 Answers1