Building Dynamic SQL Server WHERE Clause – Best Practices

performancesql serversql-injection

Let us review this dba.exchange Oracle question for SQL-Server.

This is SaUce's code, after a little formatting:

CREATE PROCEDURE GetCustomer 
    @FirstN nvarchar(20) = NULL, 
    @LastN nvarchar(20) = NULL, 
    @CUserName nvarchar(10) = NULL, 
    @CID nvarchar(15) = NULL 
as
begin 
    DECLARE @sql nvarchar(4000)

    SELECT @sql = 'C_FirstName, C_LastName, C_UserName, C_UserID 
FROM CUSTOMER
WHERE 1=1 '

    IF @FirstN IS NOT NULL 
        SELECT @sql = @sql + ' AND C_FirstName like @FirstN ' 

    IF @LastN IS NOT NULL 
        SELECT @sql = @sql + ' AND C_LastName like @LastN ' 

    IF @CUserName IS NOT NULL 
        SELECT @sql = @sql + ' AND C_UserName like @CUserName ' 

    IF @CID IS NOT NULL 
        SELECT @sql = @sql + ' AND C_UserID like @CID ' 

    EXEC sp_executesql @sql, N'@C_FirstName nvarchar(20), @C_LastName nvarchar(20), @CUserName nvarchar(10), @CID nvarchar(15)', @FirstN, @LastN, @CUserName, @CID
end
go

SaUce mentioned in his second note, that he had 1000 lines of code to prevent SQL-Injection. My feeling is that he needn't do this to shield his stored procedure.

My Question

is this SQL-Server Procedure immune
against SQL-Injection by itself
is it as satisfying solution with
respect to performance.

Best Answer

According to Erland's Sommarskog's article about dynamic sql, the use of parameterized queries should be the best point in fighting against sql injection. About performance, I also see no problem, because the execution plan of dynamic sql is reused since MSSQl 2005, if I remember correctly. Only in sql 2000 and older there was a problem with plan reuse.

His points in taking over sql injection are: "

* Never run with more privileges than necessary. Users that log into an application with their own login should normally only have EXEC permissions on stored procedures. If you use dynamic SQL, it should be confined to reading operations so that users only need SELECT permissions. A web site that logs into a database should not have any elevated privileges, preferably only EXEC and (maybe) SELECT permissions. Never let the web site log in as sa!

* For web applications: never expose error messages from SQL Server to the end user.

* Always used parameterised statements. That is, in a T-SQL procedure use sp_executesql, not EXEC().

The first point is mainly a safeguard, so that if there is a injection hole, the intruder will not be able to do that much harm. The second point makes the task for the attacker more difficult as he cannot get feedback from his attempts."

Related Solutions

Handling Exceptions in Stored Procedures with Insert-Exec Blocks

The error in the EXEC part of the INSERT-EXEC statement is leaving your transaction in a doomed state.

If you PRINT out XACT_STATE() in the CATCH block it is set to -1.

Not all errors will set the state to this. The following check constraint error goes through to the catch block and the INSERT succeeds.

ALTER PROCEDURE test -- or create
AS
  BEGIN try
      DECLARE @retval INT;

      DECLARE @t TABLE(x INT CHECK (x = 0))

      INSERT INTO @t
      VALUES      (1)

      SET @retval = 0;

      SELECT @retval;

      RETURN( @retval );
  END try

  BEGIN catch
      PRINT XACT_STATE()

      PRINT ERROR_MESSAGE();

      SET @retval = -1;

      SELECT @retval;

      RETURN( @retval );
  END catch;

Adding this to the CATCH block

 IF (XACT_STATE()) = -1
BEGIN
    ROLLBACK TRANSACTION;
END;

Doesn't help. It gives the error

Cannot use the ROLLBACK statement within an INSERT-EXEC statement.

I don't think there is any way of recovering from such an error once it has happened. For your specific use case you don't need INSERT ... EXEC anyway though. You can assign the return value to a scalar variable then insert that in a separate statement.

DECLARE @RC INT;

EXEC sp_executesql
  N'EXEC @RC = test',
  N'@RC INT OUTPUT',
  @RC = @RC OUTPUT;

INSERT INTO @t
VALUES      (@RC)

Or of course you could restructure the called stored procedure so that it doesn't raise that error at all.

DECLARE @RetVal INT = -1

IF OBJECT_ID('PrintMax', 'P') IS NULL
  BEGIN
      EXEC('create procedure PrintMax as begin print ''hello world'' end;')

      SET @RetVal = 0
  END

SELECT @RetVal;

RETURN( @RetVal );

SQL Server – Troubleshooting sp_executesql Issues

I think you are attempting to get a list of tables that contain rows referenced in another table. I think you want to encapsulate this into a re-usable object, such as a stored procedure.

I created a simple schema to test your code, as such:

USE TempDB;
CREATE TABLE dbo.T1
(
    ID INT NOT NULL PRIMARY KEY CLUSTERED
    , SomeData VARCHAR(255)
);
CREATE TABLE dbo.T2
(
    ID INT NOT NULL FOREIGN KEY REFERENCES dbo.T1(ID)
    , SomeOtherData VARCHAR(255)
);

INSERT INTO dbo.T1 VALUES (1, 'Test');
INSERT INTO dbo.T2 VALUES (1, 'Test2');

The following shows the table T2 as output, which I think is what you are attempting to do.

CREATE PROCEDURE dbo.GetReferencedTables
(
    @TableName SYSNAME
    , @RowID INT
)
AS
BEGIN
    DECLARE @OutputTable TABLE
    (
            TableName SYSNAME
    );
    DECLARE @Command nvarchar(max);

    SELECT @Command = isnull(@Command + ' union all ', '') + '
        SELECT ''' + object_name(parent_object_id) + ''' 
        WHERE EXISTS(SELECT * FROM ' + object_name(parent_object_id) 
        + ' WHERE ' + col.name+ ' = ' + cast(@RowId as varchar) + ')' 
    FROM sys.foreign_key_columns fkc
        JOIN sys.columns col ON
            fkc.parent_object_id = col.object_id AND fkc.parent_column_id = col.column_id
    WHERE OBJECT_NAME(referenced_object_id) = @TableName;

    --Totally unnecessary to put the results into a table variable
    --unless you want to further manipulate or use the results
    INSERT INTO @OutputTable
    EXEC sp_executesql @Command;

    SELECT * /* NEVER use SELECT * unless you're me. 
            see http://bit.ly/1nRziYq for reasons.
            */
    FROM @OutputTable
    ORDER BY TableName;
END
GO
EXEC dbo.GetReferencedTables 'T1',1;

Output:

enter image description here

Best Answer

Related Solutions

Handling Exceptions in Stored Procedures with Insert-Exec Blocks

SQL Server – Troubleshooting sp_executesql Issues

Related Question