Best Practices for Secure and Efficient Parameter Handling in SQL

best practicesparametersql-injection

Our senior programmer has been having me write stored procedures in the following format to protect against injection attacks. He says that the best practice is to take the parameter in, then declare a new variable in the body and assign that var the parameter, that this step is extra protection against injection attacks as it forces any injection attempts to be considered data and not taken literally. Is that true? I would think this would slow down the query and use extra memory and not add any extra protection, but I could be wrong. Example below. 

    CREATE PROCEDURE [dbo].[sp_foo_GET] 
        @parAcct            NVARCHAR(20)
AS
BEGIN
    SET NOCOUNT ON;
    DECLARE 
        @varAcct            NVARCHAR(20) = @parAcct;

    SELECT [userid] 
    FROM [tblAccounts] 
    WHERE [accountID] = @varAcct;   
END

EDIT After talking with the programmer he seems to think that passing parameters to one variable and assigning them to a declared variable helps with security AND it will cause queries to run faster.

Best Answer

He seems confused. Using the same value from a local variable should be no different to using it from an input parameter. Is your colleague getting the wrong end of the stick with regard to using parametrised queries (sometimes called prepared statements) instead of full ad-hoc SQL?

For example:

resultSet = dbconnector.getRS('EXEC sp_foo_GET \''+inputVariable+'\');

could be an injection route and moving the value between variables inside the procedure will not affect it at all as the injected code will not be seen by the procedure. If inputVariable were to have the value xxx'; WAITFOR DELAY '00:10:00'-- the SQL sent to the server would be 

EXEC sp_foo_GET 'xxx'; WAITFOR DELAY '00:10:00'--'

Your procedure would run with he input parameter set to xxx and would not know of the extra code at all, it is run after the procedure call has completed.

With a prepared statement, assuming your DB access library properly supports them and isn't doing ad-hoc silliness under the hood, this would not happen:

resultSet = dbconnector.getRSWithParams('EXEC sp_foo_GET ?', [inputVariable]);

Your procedure would be called with the value xxx'; WAITFOR DELAY '00:10:00'-- rather than that extra code being seen as something to execute.

Of course the technique might be part of a larger pattern he wants you to follow the purpose of which will become clear as you do more advanced things yet which actually does nothing useful (other than getting you into the habit of following the pattern) at this stage, but I suspect he is simply misunderstanding and therefore adding unnecessary work. Could you ask him to explain in more detail? Perhaps he could provide you with an example call that he thinks the pattern will protect against?

After Comment Re. Performance

As per your comment, while I doubt there is any security reason for this pattern the new explanation that it could make a performance difference is more valid though you might find in most cases it makes no difference and in some it reduces rather than increases performance so benchmarking rather than blindly using the pattern is recommended.

The issue comes from query plan caching and how input parameters & literals are handled differently to local variables. I'll not go into it into massive detail here as we are straying away from the original question (search in the MS documentation and the web generally around the phrases "parameter sniffing", "query plan cache", and "OPTIMIZE FOR, then ask a new question if you need more clarity after reading a few things) but as a starter-for-ten: in the example you give above what you are doing with the local variable is effectively the same as declaring the procedure with OPTION (OPTIMIZE FOR (@parAcct UNKNOWN)). The affect of this, as with any optimisation hint, can vary a lot depending on the queries being run in the procedure and the balance of the data touched by them.

The difference between using input parameters, literals and local variables with respect to cached plans can be significant when debugging performance issues (it can mean the performance profile you see when debugging/testing is quite different to that seen in production if you are not aware of the issue). http://www.brentozar.com/archive/2014/06/tuning-stored-procedures-local-variables-problems/ covers the issue from this particular context.

Related Question