4

Given the following stored procedure, can I protect against SQL injection in the supplied additional WHERE condition?

CREATE PROCEDURE [dbo].[ProtectDynamicWhereClause]
   (@TableName varchar(50),
    @OldestRecordDate varchar(15),
    @WhereCondition varchar(250) = NULL)
AS
BEGIN
    -- Protect the table name from SQL Injection.
    DECLARE @TableNameClean varchar(150) = QUOTENAME(@TableName)

    DECLARE @sql nvarchar(4000)
    SET @sql = 
            N'
            SELECT * FROM ' + 'dbo.' + @TableNameClean
            + ' WHERE EntryAge > DATEDIFF(year, @OldestRecordDate, GETDATE()) '

    IF (@WhereCondition IS NOT NULL)
    BEGIN
        SET @sql = @sql + ' AND ' + @WhereCondition
    END               

    EXEC  sp_executesql @sql, N'@TableNameClean varchar(50), @OldestRecordDate varchar(15)', 
                                @TableNameClean = @TableNameClean, @OldestRecordDate = @OldestRecordDate
END

EXEC ProtectDynamicWhereClause @TableName='CustomerTbl', @OldestRecordDate = '2012-01-01', @WhereCondition = 'CustomerName LIKE ''%Smith%'''
EXEC ProtectDynamicWhereClause @TableName='ProductTbl', @OldestRecordDate = '2010-01-01', @WhereCondition = 'ProductId IN (123, 345, 567)'

Could you please tell me how to protect against SQL injection in this statement when the @WhereCondition is actually passed-into the stored procedure as a parameter as shown?

SET @sql = @sql + ' AND ' + @WhereCondition

Thank you.

6

You simply don't write it in such a way that a user can pass in a structured WHERE clause. This is a recipe for disaster, and I bet at least half of the companies who have been exploited by SQL injection thought they were protected by checking the input for keywords, stripping out semi-colons and comments, etc. They weren't, and there will always be ways around whatever protection you try to manually write.

Don't be lazy. Write it so that the user picks the possible columns and operations from drop-downs, and only enters the parameter values into free text. Then you construct the dynamic SQL with the columns you know exist and with strongly-typed parameters that can only be treated as such rather than just appending your query with whatever someone typed into a form field on a web page and blindly executing it.

Surely there aren't 8,000 columns in this table, and the number of permutations of clauses that a user can add are not exhaustive. You could always limit it logically to 5 or 10 clauses so that you don't have GB-long WHERE clauses.

Some constructive ideas for dealing with dynamic SQL and flexible WHERE clauses:

Other answers where I talk about the kitchen sink procedure:

  • Thank you very much for your suggestions. Sadly I inherited the stored proc so I'm restricted in my available options regarding the design of the application that calls the stored proc. – Nana A Sep 23 '15 at 14:02
  • 1
    +1. Don't do that is the best way if you have any control to dictate how the procedure works. – David Spillett Sep 23 '15 at 14:18
0

The only way to protect against injection here would be to write a parser, in or called from this stored procedure, for the SQL language parts that you wish to allow - that code could validate the strings to make sure they do nothing nasty. This almost certainly isn't practical nor would it be remotely efficient written in TSQL - so technically the answer is yes there is a way, but practically the answer is no.

If you only want to allow a very small subset of the SQL language - i.e. all the clauses will be of the form <fieldname> LIKE '%<string>%' or <fieldname> IN (number, number, number) parsing the string to make sure it matches those would be more practical, but this would be better done in your layer above in your application where it'll be more efficient. You might have to include a fixed list of possible column names as the user that the application is running as may not be able to select from sys.columns to verify the provided names that way.

  • Thank you very much for your response. I think it is likely to be a white list parser solution based on how the SP is currently used in the live environment. The monitoring should help me work out what to allow in the parser. The downside is that new uses of the Where condition will probably fail until I update the parser or its inputs. – Nana A Sep 23 '15 at 14:07
  • That is the only safe way to be: if you are not very strict in what you allow (explicitly enumerating the good instead of trying to detect the bad) you risk allowing injection routes. Even then you need to be very very careful, and need to be away it'll be a wall of potentially inefficient (and probably hard to read/debug) string manipulation code. – David Spillett Sep 23 '15 at 14:17
0

I have the same problem with a read mostly database. It contains the metadata from a machine learning algorithm that classifieds images. The main search form grew too large to use the GET method, which people liked because they could copy the URI and include it in an email or a slack post.

The easiest way to give them such a link was too include the where clause. Since I'm working with Java servlets and I control the generation of the where clause it was fairly straightforward to produce a validator that is too simple to call a parser. Here is the method that I call. It will take some work to adapt to your problem but I think it's easy enough to understand how it works;

  /**
 * Verify a where clause is valid
 * example:  GPStime >= 1164554240 AND GPStime <= 1188081664  AND  ifo = 'L1' 
 *          AND  pipeline = 'GravitySpy' AND  label = 'Blip' AND  
 *          confidence  >= 0.960 AND confidence <= 1.00 
 *          ORDER BY confidence DESC, snr DESC
 * Note all tokens must be separated by whitespace, I'm lazy
 * @param where - clause to be validated
 * @return - True if we like the clause, Full survey it fails
 */
public boolean checkWhere(String where)
{
    boolean ret = true;
    Pattern[] pats = 
    {
        Pattern.compile("^'.*'$"),  // quoted string
        Pattern.compile("^[\\<\\>\\=]{1,2}$"),  // Number comparator
        // acceptable keyword
        Pattern.compile("(?i)^(and|or|like|order|by|ASC|DESC),{0,1}$"),
        // floating-point number
        Pattern.compile("^\\s*[+-]?\\s*((\\d+\\.?\\d*)|(\\.\\d+))\\s*["
                + "Ee]?\\s*([+-]?\\d+)?\\s*$")
    };
    Pattern identifier = Pattern.compile("[\\w_]+"); // column name

    StringTokenizer toks = new StringTokenizer(where);

    // loop verifies that each token is acceptable ignoring the order
    while (toks.hasMoreTokens())
    {
        String token = toks.nextToken();
        Matcher m;
        boolean ok = false;
        for (Pattern pat : pats)
        {
            m = pat.matcher(token);
            if (m.find())
            {
                ok = true;
                break;
            }
        }
        if (!ok)
        {
            m = identifier.matcher(token);
            if (m.find())
            {
                if (!fieldNames.containsKey(token))
                {
                    ret = false;
                    break;
                }
            }
            else
            {
                ret = false;
                break;
            }
        }
    }

    return ret;
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.