How to fix Inline C# query that is prone to SQL injection

Question

Following is my query that I wrote in order to get a simple max value for a given where clause.

The only problem is it has to go through a veracode scan which determines if the query is prone to any SQL injection.

This is my query

string strconnectionString = @Data Source = xxx\server; Initial Catalog =DBname;

These are user inputs I am just hardcoding for now

private DateTime? GetFirmsLastDate()
{
    // My user inputs hardcoded for now 
    string tableName = "abc";
    string columnName = "CalculationTable";
    string filterColumn = "CalcValue";
    string firmName = "Bank1";

    using(SqlConnection connection = new SqlConnection(strconnectionString)
    {
        using(Sqlcommand cmd = new SqlCommand())
        {
            cmd.Connection = connection;
            cmd.CommandType = System.Data.CommandType.Text;

            cmd.CommandText = String.Format(@"Select MAX(K.{3}) FROM {1} K WHERE K.{2} ={0}" ,
                                             Sanitizer.GetSafeHtmlFragment(firmName), 
                                             Sanitizer.GetSafeHtmlFragment(tableName),
                                             Sanitizer.GetSafeHtmlFragment(filterColumn),
                                             Sanitizer.GetSafeHtmlFragment(column)

            connection.Open();
            object dateVal = cmd.ExecuteScalar();

            return (dataVal != DBNull.Value) : DateTime.Parse(dateVal.ToString()) : null;
        }
    }
}

Why I had to use the inline query is TableName. It is a textbox where the user can enter the table name. I cannot specify the tableName as a parameter for that purpose i have to prepare my statement when passing a SqlCommand.

Thank you for your time.

There is no way to parameterize table AND column names. You should provide your user with a whitelist of tablenames to choose from using some kind of readonly UI like a ListBox, ComboBox or DropDownList. Of course once the name of the table has been choosen the same rule applies to the names of table's columns — Steve
– Steve, Commented Mar 15, 2019 at 20:20

Thom A · Accepted Answer · 2019-03-15 20:39:45Z

1

The only way I can think of doing this "safely" is by changing your inline query to a parametrised call to a stored procedure, and then gracefully handling the object names there. This, however, has the requirement that the column being checked always has the same data type (in the case, I've assumed int):

CREATE PROC TableMaxCount @Table sysname, @Column sysname, @Value int AS
BEGIN

      DECLARE @SQL nvarchar(MAX);
      SET @SQL = N'SELECT MAX(' + QUOTENAME(@Column) + NCHAR(13) + NCHAR(10) +
                 N'FROM ' + QUOTENAME(@Table) + NCHAR(13) + NCHAR(10) +
                 N'WHERE ' + QUOTENAME(@Column) + N' = @Value;';
      EXEC sp_executesql @SQL, N'@Value int', @Value = @Value;

END

This, however, will generate an error if the user enters a value that isn't a read table, or column. If that isn't desired, and you want to validate the object names, you could do something more like this:

CREATE PROC TableMaxCount @Table sysname, @Column sysname, @Value int AS
BEGIN

      DECLARE @SQL nvarchar(MAX);
      SELECT @SQL = N'SELECT MAX(' + QUOTENAME(c.[name]) + NCHAR(13) + NCHAR(10) +
                    N'FROM ' + QUOTENAME(t.[name]) + NCHAR(13) + NCHAR(10) +
                    N'WHERE ' + QUOTENAME(c.[name]) + N' = @Value;'
      FROM sys.tables t
           JOIN sys.columns c ON t.object_id = c.object_id
           JOIN sys.schemas s ON t.schema_id = s.schema_id
      WHERE t.[name] = @Table
        AND c.[name] = @Column
        AND s.[name] = N'dbo'; --Assumes always dbo schema

      EXEC sp_executesql @SQL, N'@Value int', @Value = @Value;

END

I'm afraid my C# is awful at best, however, hopefully you already know how to use parametrised code to call SQL. If not, I know the documentation does cover it. (SqlCommand.Parameters Property)

edited Mar 15, 2019 at 20:39

answered Mar 15, 2019 at 20:34

Thom A♦

97.7k12 gold badges67 silver badges102 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

user3920526 Over a year ago

I was not looking for a Sql query , i am just looking for a C# code.

user3920526 Over a year ago

The above query is absolutely wrong their are 4 parameter and all are string I think you just copy pasted the query from somewhere but Also I see a big mistake i.e in sql never declare a nvarchar max variable. Please delete this answer . Thank you

Thom A Over a year ago

@user3920526 because it is there to guide you; and you can easily change this to use 4 parameters if you want. And what reason do you have to never declare an nvarchar(MAX)? You're going to have a problem when you need to store more than 4,000 characters in a unicode string. As Steve as mentioned in his comment, you can't parametrise an object, not the way you want to, and hence why you need to use dynamic SQL to achieve this. Just because you don't like that answer doesn't mean it won't help future readers.

user3920526 Over a year ago

Please read the question . I was looking for a way to pass the tableName in an in-line C# query . You are answering my question in sql . Plus you want to use nvarchar max for a select max(colCount) from tableName where filtercolumn=Colname count how many characters . Please delete this sql answer .

user3920526 Over a year ago

If you think it’s right does not mean it’s the right answer . I am not looking at this approach .

|

Collectives™ on Stack Overflow

How to fix Inline C# query that is prone to SQL injection

1 Answer 1

7 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

7 Comments

Your Answer

Sign up or log in

Post as a guest

Related