1

I was given a task to rewrite an old web API.

This API reads SQL queries from the database.

There's literally a view with "Queries" in the name which contains "SqlText" column.

SELECT SqlText FROM Queries WHERE QueryID = 123

The "SqlText" contains only simple SQL queries in the format SELECT [columns] FROM [table] by convention.

The query is altered depending on the URL parameters in the request. The result of this query is then shown as result.

string parsedColumns = ParseColumns(queryRow); //contains "Column1, Column2";
string parsedTable = ParseTable(queryRow); //contains "SomeTable"

string requestColumns = HttpContext.Request["columns"];
string sqlColumns = requestColumns ?? parsedColumns;

string col1Condition = HttpContext.Request["Column1"]
string col2Condition = HttpContext.Request["Column2"]

string sqlQuery = "SELECT " + sqlColumns 
                  + " FROM " + parsedTable 
                  + " WHERE Column1 = " + col1Condition 
                  + " AND Column2 = " + col2Condition;

This is obvious SQL injection issue so I started rewritting it.

Now there are three other problems.

  • I cannot change the structure of the database or the convention
  • The database is either Oracle or SQL Server
  • I don't know how to correctly work with the "columns" URL parameter to avoid SQL injection.

It's easy to convert the URL parameters in the WHERE clause to the SQL parameters for both SQL Server and Oracle.

SQL Server

var sqlCommand = new SqlCommand("SELECT * FROM SomeTable WHERE Condition1 = @con1 AND Condition2 = @con2");

Oracle

var oracleCommand = new OracleCommand("SELECT * FROM SomeTable WHERE Condition1 = :con1 AND Condition2 = :con2");

Column identifiers

The problem is with the HttpContext.Request["columns"]. I still need to somehow alter the SQL query string with URL parameters which I don't like at all.

To simplify the issue, let's consider a single column from URL request.

string column = HttpContext.Request["column"];
var cmd = new SqlCommand($"SELECT {column} FROM ...");

I know that in SQL Server the identifier can be surrounded by braces. So my line of thinking is that I'm safe if I strip all braces from the column.

string column = HttpContext.Request["column"];
column = column.Replace("[", "").Replace("]", "");
column = $"[{column}]";
var cmd = new SqlCommand($"SELECT {column} FROM ...");

Oracle uses quotation marks.

string column = HttpContext.Request["column"];
column = column.Replace("\"", "");
column = $"\"{column}\"";
var cmd = new OracleCommand($"SELECT {column} FROM ...");

The question

  • Is this sql-injection safe enough?
  • Or is this use case inherently sql-injection unsafe?
Mirek
  • 4,013
  • 2
  • 32
  • 47
  • The quote characters in SQL Server are brackets (`[]`) not braces (`{}`). Even so, simply wrapping the value of a dynamic value with a brackets doesn't make it safe; as the person passing the injection string would simply pass a `]` as part of their string (ending the quote). Personally, i prefer to handle dynamic SQL the SQL Server side of things, and pass a parametrised query to the SQL Instance. – Thom A Nov 21 '18 at 15:25
  • 2
    This still feels vulnerable to injection, to me at least. I guess you could check the existance of the column in `INFORMATION_SCHEMA.COLUMNS` (using a **properly parameterised query!**) before you allow it to be used in a query, but it still feels risky. – Diado Nov 21 '18 at 15:32
  • Actually Larnu, he is stripping out the "]" character so even if they pass in a "]" it won't make it to the query. And the braces he is using to add the value of column into the string as per c#. I agree with Diado though that it still feels vulnerable. Can you guarantee that column names will not have spaces? You could strip out all characters other than alphanumeric and underscore if that is the case. May make it secure. Still not positive though. – MikeS Nov 21 '18 at 15:34
  • Yes I meant brackets not braces. Also yeah, I completely forgot dynamic SQL calls exist, thank you! – Mirek Nov 21 '18 at 15:34
  • I would handle the data validation all in c#... pass NOTHING to SQL before its checked out. parse the columns; strip the [ ] and split it into an array/list, then one by one - check that list against information_schema.tables/information_schema.columns, and chuck out any that don't exist/match (avoids errors too), THEN build the command variable and build the string to execute. – Dave C Nov 21 '18 at 15:37
  • To be clear, I would pull the information_schema.columns data into c# as an array/list, then compare that to the "columns" array you created from the url param... do not ask SQL to verify existence. – Dave C Nov 21 '18 at 15:42

1 Answers1

1

Since you are working with a basic program design that you cannot change what about just trying to add edits to the input to look for injection elements. For example if the input is a column name it will need to have a maximum length of 30 (before 12.x) characters and should not contain a semicolon or the strings " OR" or " AND" in them. While not a perfect solution this should be practical solution.

Mark D Powell
  • 151
  • 2