Safe dynamic SQL for generic search

Question

Prompted by discussion about SQL injection, I wanted to put a proof of concept forward to get feedback about whether this is in fact safe and protected against SQL injection or other malicious use. For good reference on the subject of constructing a dynamic search with dynamic SQL, I'd probably look there.

This is meant to be a proof of concept, not a complete working solution to illustrate how we can accept text input from users but handle it as if it were parameterized properly.

The assumptions are as follows:

1. We don't want to run code client-side -- in theory, this could have been done in a middle tier as some kind of API. However, even if there were a middle tier API endpoint, it does no good if it does not properly parameterize the query it makes on users' behalf. Furthermore, having it in SQL means that it is now generic to any clients who may need the functionality but at expense of poor portability. This will likely work only on Microsoft SQL Server, not on other database vendors, at least not without significant modifications.

   NOTE: Though the code below has been tested with SQL Server 2012, in theory, it should be compatible for 2008 R2 and higher. We can assume that any answer that will work on 2008 R2 is acceptable; if an answer relies on some features introduced in the later version, it will be also considered, especially if it improves the security.

2. Under no circumstances should the users be allowed to write the dynamic SQL, whether directly or indirectly. The only thing that should write the dynamic SQL is our code, without any user's inputs. That means taking additional indirection to ensure that users' input cannot become a part of the dynamic SQL being assembled.

3. We assume that the users only need to search a single table, wants all columns, but may want to filter on any columns. This is only to simplify the proof of the concept - there is no technical reason why it can't do more, provided that the practices outlined in the proof of concept is rigorously followed.

4. Because dynamic SQL is ultimately executed, we have to require that the users has at least the SELECT permission on the table they want to search.

Helper Function for data types

We first need a function to help us with building a formatted data type because the sys.types doesn't present the information in most friendly manner for writing a parameter. While this could be more sophisticated, this suffices for most common cases:

CREATE OR ALTER FUNCTION dbo.ufnGetFormattedDataType (
    @DataTypeName sysname,
    @Precision int,
    @Scale int,
    @MaxLength int
) RETURNS nvarchar(255) 
WITH SCHEMABINDING AS
BEGIN
    DECLARE @Suffix nvarchar(15);

    SET @Suffix = CASE 
        WHEN @DataTypeName IN (N'nvarchar', N'nchar', N'varchar', N'char', N'varbinary', N'binary')
        THEN CONCAT(N'(', IIF(@MaxLength = -1, N'MAX', CAST(@MaxLength AS nvarchar(12))), ')')

        WHEN @DataTypeName IN (N'decimal', N'numeric')
        THEN CONCAT(N'(', @Precision, N', ', @Scale, N')')

        WHEN @DataTypeName IN (N'datetime2', N'datetimeoffset', N'time')
        THEN CONCAT(N'(', @Scale, N')')

        ELSE N''
    END;

    RETURN CONCAT(@DataTypeName, @Suffix);
END;

Main dynamic search procedure

With the function, we can then build our main procedure for creating the dynamic SQL to support generic search:

CREATE OR ALTER PROCEDURE dbo.uspDynamicSearch (
    @SchemaName sysname,
    @TableName sysname,
    @ParameterXml xml 
) AS
BEGIN
    DECLARE @stableName sysname,
            @stableId int,
            @err nvarchar(4000)
    ;

    SELECT  
        @stableName = o.Name,
        @stableId = o.object_id
    FROM sys.objects AS o
    WHERE o.name = @TableName
      AND OBJECT_SCHEMA_NAME(o.object_id) = @SchemaName;

    IF @stableName IS NULL
    OR @stableId IS NULL
    BEGIN
        SET @err = N'Invalid schema or table name specified.';
        THROW 50000, @err, 1;
        RETURN -1;
    END;

    SELECT
        x.value(N'@Name', N'sysname') AS ParameterName,
        x.value(N'@Value', N'nvarchar(MAX)') AS ParameterValue
    INTO #RawData
    FROM @ParameterXml.nodes(N'/l/p') AS t(x);

    IF EXISTS (
        SELECT NULL
        FROM #RawData AS d
        WHERE NOT EXISTS (
            SELECT NULL
            FROM sys.columns AS c
            WHERE c.object_id = @stableId
              AND c.name = d.ParameterName
        )
    )
    BEGIN
        SET @err = N'Invalid column name(s) specified.';
        THROW 50000, @err, 1;
        RETURN -1;
    END;

    SELECT
        ROW_NUMBER() OVER (ORDER BY (SELECT NULL)) AS Id,
        c.name AS ColumnName,
        d.ParameterValue AS ParameterValue,
        c.user_type_id AS DataTypeId,
        t.name AS DataTypeName,
        c.max_length AS MaxLength,
        c.precision AS Precision,
        c.scale AS Scale,
        dbo.ufnGetFormattedDataType(t.name, c.precision, c.scale, c.max_length) AS ParameterDataType
    INTO #ParameterData
    FROM #RawData AS d
    INNER JOIN sys.columns AS c
        ON d.ParameterName = c.name
    INNER JOIN sys.types AS t
        ON c.user_type_id = t.user_type_id
    WHERE c.object_id = @stableId;

    DECLARE @Sql nvarchar(MAX) = CONCAT(N'SELECT * FROM ', QUOTENAME(OBJECT_SCHEMA_NAME(@stableId)), N'.', QUOTENAME(@stableName));

    IF EXISTS (
        SELECT NULL
        FROM #ParameterData
    )
    BEGIN
        DECLARE @And nvarchar(5) = N' AND ';

        SET @Sql += CONCAT(N' WHERE ', STUFF((
            SELECT 
                CONCAT(@And, QUOTENAME(d.ColumnName), N' = @P', d.Id)
            FROM #ParameterData AS d
            FOR XML PATH(N'')
        ), 1, LEN(@And), N''));

        DECLARE @Params nvarchar(MAX) = CONCAT(N'DECLARE ', STUFF((
            SELECT
                CONCAT(N', @P', d.Id, N' ', d.ParameterDataType, N' = ( SELECT CAST(d.ParameterValue AS ', d.ParameterDataType, N') FROM #ParameterData AS d WHERE d.Id = ', d.Id, N') ')
            FROM #ParameterData AS d
            FOR XML PATH(N'')
        ), 1, 2, N''), N';');

        SET @Sql = CONCAT(@Params, @Sql);
    END;

    SET @Sql += N';';
    EXEC sys.sp_executesql @Sql;
END;

Analysis

Let's go over the procedures in parts to elaborate the reasoning behind the design, starting with the parameters.

@SchemaName sysname,
@TableName sysname,
@ParameterXml xml

The schema and table names are self-evident but we require that the users provide their search conditions as a XML document. It doesn't have to be an XML document; JSON would work as well (provided that you're using a recent version of SQL Server). The point is that it must be a well-defined format with native support for parsing the contents. A sample XML may look something like this:

<l>
  <p Name="First Name" Value="Martin" />
  <p Name="Last Name" Value="O’Donnell" />
</l>

The XML is basically a (l)ist of the (p)arameters in name-value pairs.

We have to validate both parameters. First is easily done:

SELECT  
    @stableName = o.Name,
    @stableId = o.object_id
FROM sys.objects AS o
WHERE o.name = @TableName
  AND OBJECT_SCHEMA_NAME(o.object_id) = @SchemaName;

Because we do not want users' inputs to go directly into the dynamic SQL, we use a separate variable, @stableName which would be same value as the @TableName but only if the user isn't malicious and tried to sneak in extra characters. Since we filter it through the sys.objects, that implicitly enforces SQL Server's identifier rules and thus validate that the input is valid.

For the parameters, we need some more work, so we need to load into a temporary table. We can't trust the user inputs so we must treat it accordingly.

SELECT
    x.value(N'@Name', N'sysname') AS ParameterName,
    x.value(N'@Value', N'nvarchar(MAX)') AS ParameterValue
INTO #RawData
FROM @ParameterXml.nodes(N'/l/p') AS t(x);

Using a temporary table allow us to materialize the contents of XML in a relational table since we refer to it twice later on.

We need to validate all the column names in the same manner we did with the table name. Since it's a set, we'll use EXISTS to help us out.

IF EXISTS (
    SELECT NULL
    FROM #RawData AS d
    WHERE NOT EXISTS (
        SELECT NULL
        FROM sys.columns AS c
        WHERE c.object_id = @stableId
          AND c.name = d.ParameterName
    )
)
BEGIN
    SET @err = N'Invalid column name(s) specified.';
    THROW 50000, @err, 1;
    RETURN -1;
END;

In addition to validating the column name, we also verify it's in the same table we are going to query.

SELECT
    ROW_NUMBER() OVER (ORDER BY (SELECT NULL)) AS Id,
    c.name AS ColumnName,
    d.ParameterValue AS ParameterValue,
    c.user_type_id AS DataTypeId,
    t.name AS DataTypeName,
    c.max_length AS MaxLength,
    c.precision AS Precision,
    c.scale AS Scale,
    dbo.ufnGetFormattedDataType(t.name, c.precision, c.scale, c.max_length) AS ParameterDataType
INTO #ParameterData
FROM #RawData AS d
INNER JOIN sys.columns AS c
    ON d.ParameterName = c.name
INNER JOIN sys.types AS t
    ON c.user_type_id = t.user_type_id
WHERE c.object_id = @stableId;

In addition to validating the column names we want to use for filters, we collect metadata from the sys.columns and sys.types. Note that the XML itself can't be used to tell us what the data types the user wants to use. That would be a vector for malicious attack so we must rely on the information from the catalog views, accepting only the parameter values directly from the XML supplied by the user.

At this point, we've validated and collected all the metadata about the columns but we still can't trust the contents of the ParameterValue.

Note the ROW_NUMBER() generating the IDs of the parameters. That is important as will be seen later on.

DECLARE @Sql nvarchar(MAX) = CONCAT(N'SELECT * FROM ', QUOTENAME(OBJECT_SCHEMA_NAME(@stableId)), N'.', QUOTENAME(@stableName));

We build our first part of the dynamic SQL. We assume that it's OK to allow the users to select entire table, though that might be dickish if there's lot of records. In a complete solution, it might be more prudent to have a TOP 100 or something like that. The problem is that a TOP N usually doesn't make sense without an ORDER BY so the same complete solution should probably have some support for allowing users to specify a sort order to ensure consistent results, even if it's something lame like sorting by the identity column.

Going forward, we'll assume that we have a set of parameters that we need to filter on.

SET @Sql += CONCAT(N' WHERE ', STUFF((
    SELECT 
        CONCAT(@And, QUOTENAME(d.ColumnName), N' = @P', d.Id)
    FROM #ParameterData AS d
    FOR XML PATH(N'')
), 1, LEN(@And), N''));

Here, we abuse the FOR XML PATH to provide a concatenation of the filter predicate for the WHERE clause. Using the sample XML above, the output would have been something like WHERE [First Name] = @P1 AND [Last Name] = @P2. Note the horrid naming of columns, with spaces in it, to show the value of QUOTENAME to ensure that even in a crappy database schema, we can avoid getting an error with an iffy identifier.

Note that we also assume all filters in XML are AND'd together. In a more complex implementation, users might want the option to OR or mix AND and OR, either which could be provided via an XML attribute.

DECLARE @Params nvarchar(MAX) = CONCAT(N'DECLARE ', STUFF((
    SELECT
        CONCAT(N', @P', d.Id, N' ', d.ParameterDataType, N' = ( SELECT CAST(d.ParameterValue AS ', d.ParameterDataType, N') FROM #ParameterData AS d WHERE d.Id = ', d.Id, N') ')
    FROM #ParameterData AS d
    FOR XML PATH(N'')
), 1, 2, N''), N';');

This is the closest the user's input can get to our dynamic SQL. We would read from the same temporary table we created and assign to a parameter we create ourselves, with a CAST. Note that we could have used a TRY_CAST to avoid a runtime error but I would argue that an error needs to occur if the users put in bad input. In a complete solution, the procedure could be wrapped in a TRY/CATCH block to sanitize the error message somehow.

Again using the XML example from above, it'd come out something like this (formatted for readability):

DECLARE @P1 varchar(100) = (
  SELECT CAST(d.ParameterValue AS varchar(100)) 
  FROM #ParameterData AS d WHERE d.Id = 1
);

Note that we did not even use the name that the users provided to us; we used a numeric ID which was concatenated by our own code. Furthermore, the code reads from the temporary table and CAST it into the parameter we want it to be. That makes it easier for us to handle different data types for various parameters the users may send to us but without actually concatenating the values they provide to us to our dynamic SQL.

Once we have that, we concatenate the assignments to the @Sql and execute it:

EXEC sys.sp_executesql @Sql;

Note that we didn't use the @params parameter of the sp_executesql -- there's no parameters we can really pass in since the parameters are in a temporary table, which was why we used assignments inside the dynamic SQL to move the user's input from a XML document to a parameter within the dynamic SQL.

Sample calling code

--Returns one match
EXEC dbo.uspDynamicSearch 
    N'dbo',
    N'Customers', 
    N'<l><p Name="First Name" Value="Martin" /><p Name ="Last Name" Value="O’Donnell" /></l>';

-- Returns an empty result set
EXEC dbo.uspDynamicSearch 
    N'dbo',
    N'Customers', 
    N'<l><p Name="First Name" Value="Martin''; DROP TABLE Customers; --" /><p Name ="Last Name" Value="O’Donnell" /></l>';

--Returns an error; invalid table name
EXEC dbo.uspDynamicSearch 
    N'dbo',
    N'Customers''; DROP TABLE Customers; --', 
    N'<l><p Name="First Name" Value="Martin" /><p Name ="Last Name" Value="O’Donnell" /></l>';

--Returns an error; invalid column name
EXEC dbo.uspDynamicSearch 
    N'dbo',
    N'Customers', 
    N'<l><p Name="First Name''; DROP TABLE Customers; --" Value="Martin" /><p Name ="Last Name" Value="O’Donnell" /></l>';

Here's a sample of the complete dynamic SQL assembled by the code, formatted for readability:

DECLARE @P1 nvarchar(100) = ( 
  SELECT CAST(d.ParameterValue AS nvarchar(100)) 
  FROM #ParameterData AS d WHERE d.Id = 1
), @P2 nvarchar(100) = ( 
  SELECT CAST(d.ParameterValue AS nvarchar(100)) 
  FROM #ParameterData AS d WHERE d.Id = 2
);

SELECT * 
FROM [dbo].[Customers]
WHERE [First Name] = @P1 AND [Last Name] = @P2;

Can this be circumvented?

As mentioned, the discussion about SQL injection made me wonder if I may have missed something or made an assumption where the malicious user could still manage to circumvent the layers of indirection I've put in and inject their nasty little SQL?

Answer 1

Comments on your approach

The most obvious thing to me is that you're already inserting all of your data into the temp table; why not just join to it? You can dynamically pivot into a better, strongly-typed table instead (see this post), and then you don't need to muck around with the parameter stuff.

I also think I think you're overcomplicating this by a lot. Ultimately, the best way to write dynamic SQL is to:

1. Trust no user inputs
2. Validate all user inputs
3. Validate all inputs, even if they come out of a table/procedure you control. You never know how a different attack vector could affect you indirectly.
4. Limit the scope of what a user can input

Do you actually really need to let anyone run any filter on any column on any table? I suspect that you don't actually need that much freedom.

If I'm correct, then you can likely make this much safer by maintaining a list of valid table/column combinations that can be filtered (still verify per #3 above), and immediately throw out junk and identify someone trying to do something that isn't kosher. You could even create a stored procedure per-table that has strongly typed input variables, and then you have parameterized SQL all the way down with almost no dynamic SQL required.

---

Coding style

Here are just a few things I think could be handled with better style:

SELECT @stableName = o.name,
       @stableId = o.object_id
  FROM sys.objects o
  WHERE o.name = @TableName
    AND OBJECT_SCHEMA_NAME( o.object_id ) = @SchemaName;

This is cleaner with sys.tables and SCHEMA_ID

SELECT @sTableName = tables.name,
       @sTableId = tables.object_id
  FROM sys.tables 
  WHERE tables.name = @TableName
    AND tables.schema_id = SCHEMA_ID( @SchemaName );        

Exists checks can often be replaced with joins, which are easier to read

IF EXISTS ( SELECT NULL
              FROM #RawData d
              WHERE NOT EXISTS ( SELECT NULL
                                   FROM sys.columns c
                                   WHERE c.object_id = @stableId
                                     AND c.name = d.ParameterName ))


IF EXISTS( SELECT 1
             FROM #RawData RD
               LEFT OUTER JOIN sys.columns columns
                 ON columns.object_id = @sTableId
                    AND columns.name = RD.ParameterName 
             WHERE columns.object_id IS NULL )

I also think you would benefit from breaking this up more; for example, you could make this into a table valued function. Also, since you're using SELECT INTO you can use the IDENTITY function instead of ROW_NUMBER

SELECT ROW_NUMBER() OVER ( ORDER BY ( SELECT NULL )) Id,
       c.name ColumnName,
       d.ParameterValue ParameterValue,
       c.user_type_id DataTypeId,
       t.name DataTypeName,
       c.max_length MaxLength,
       c.precision Precision,
       c.scale Scale,
       dbo.ufnGetFormattedDataType( t.name, c.precision, c.scale, c.max_length ) ParameterDataType
  INTO #ParameterData
  FROM #RawData d
    INNER JOIN sys.columns c
      ON d.ParameterName = c.name
    INNER JOIN sys.types t
      ON c.user_type_id = t.user_type_id
  WHERE c.object_id = @stableId;

I don't really like the syntax of

SET @variable = ( SELECT <<whatever>> )

Instead I prefer

SELECT TOP( 1 )
       @variable = <<whatever>>
  ...
  ORDER BY ( SELECT NULL );

If you PIVOT here you can even do it all in one table access.