Evitando injeção de SQL em SQL dinâmico

Em seu código de exemplo, você está passando três categorias de "coisas" para seu SQL dinâmico.

1. Você passa @OrderDir, que é uma palavra-chave para significar ASCou DESC.
2. Você passa @OrderBy, que é um nome de coluna (ou potencialmente um conjunto de nomes de coluna, mas com base na maneira como #1 é implementado, suponho que você espera um único nome de coluna.
3. Você passa @PageNumbere @PageSize, que se tornam literais na string gerada.

Palavras-chave

Isso é realmente simples - você só quer validar sua entrada. Você está certo de que esta é a coisa certa para esta opção. Nesse caso, você está esperando ASCou DESC, portanto, você pode verificar se o usuário passa um desses valores ou alternar para uma semântica de parâmetro diferente, onde você tem um parâmetro que é uma opção de alternância. Declare seu parâmetro como @SortAscending bit = 0, em seguida, dentro de seu procedimento armazenado, traduza o bit para ASCou DESC.

Nomes de coluna

Aqui, você deve usar a QUOTENAMEfunção. Quotename garantirá que os objetos sejam corretamente [quoted], garantindo que, se alguém tentar passar uma "coluna" de "; TRUNCATE TABLE USERS", ela será tratada como um nome de coluna e não como um pedaço arbitrário de código injetado. Isso falhará, em vez de truncar a USERStabela:

SELECT [; TRUNCATE TABLE USERS]...
FROM...

Literais e parâmetros

Para @PageNumbere @PageSize, você deve usar sp_executesqlpara passar parâmetros corretamente. Parametrizar corretamente seu SQL dinâmico permite não apenas passar valores, mas também obter valores de volta .

Neste exemplo, @xe @yseriam variáveis ​​com escopo para seus procedimentos armazenados. Eles não estão disponíveis em seu SQL dinâmico, portanto, você os passa para @ae @b, que têm como escopo o SQL dinâmico. Isso permite que você tenha valores digitados corretamente dentro e fora do SQL dinâmico.

DECLARE @i int,
 @x int,
 @y int,
 @sql nvarchar(1000),
 @params nvarchar(1000);


SET @x = 10;
SET @y = 5;
SET @params = N'@i_out int OUT, @a int, @b int';
SET @sql    = N'SELECT @i_out = @a + @b';


EXEC sp_executesql @sql, @params, @i_out = @i OUT, @a = @x, @b = @y; 
SELECT @i;

Mesmo com valores varchar, manter o valor como uma variável impede que alguém passe arbitrariamente o código que é executado. Este exemplo garante que a entrada do usuário seja SELECTed e não executada arbitrariamente:

DECLARE @UserInput varchar(100),
         @params nvarchar(1000) = N'@value varchar(100)',
         @sql nvarchar(1000)    = N'SELECT Value = @value';

SET @UserInput = '; TRUNCATE TABLE USERS;'
EXEC sp_executesql @sql, @params, @value = @UserInput;  

Meu código

Aqui está minha versão do seu procedimento armazenado, com definição de tabela e algumas linhas de amostra:

CREATE TABLE dbo.Persons
       (
        id INT,
        firstName VARCHAR(50),
        lastName VARCHAR(50)
       );
GO

INSERT INTO dbo.Persons(id, firstName,lastName)
VALUES (1,'George','Washington'),
       (2,'John','Adams'),
       (3,'Thomas','Jefferson'),
       (4,'James','Madison'),
       (5,'James','Monroe')


ALTER PROCEDURE dbo.GetPersons
       @pageNumber INT = 1,
       @pageSize INT = 20,
       @orderBy VARCHAR(50) = 'id',
       @orderDir VARCHAR(4) = 'desc'
AS
       SET NOCOUNT ON;

--validate inputs
IF NOT EXISTS ( SELECT   1 FROM     sys.columns
                WHERE    object_id = OBJECT_ID('dbo.Persons')
                AND name = @orderBy )
BEGIN
        RAISERROR('Order by column does not exist.', 16,1);
        RETURN;
END;

IF (@orderDir NOT IN ('ASC', 'DESC'))
BEGIN
        RAISERROR('Order direction is invalid. Must be ASC or DESC.', 16,1);
        RETURN;
END;

--Now do stuff
--sp_executesql takes in nvarchar as a datatype
DECLARE @sql NVARCHAR(MAX);

SET @sql = N'SELECT id, firstName, lastName
FROM (
    SELECT id, firstName, LastName, ROW_NUMBER() OVER(ORDER BY '
           + QUOTENAME(@orderBy) + N' ' + @orderDir + N') AS rn
    FROM dbo.Persons
    ) t
WHERE rn > ( @pageNumber-1) * @pageSize
        AND rn <= @pageNumber * @pageSize 
ORDER BY ' + QUOTENAME(@orderBy) + N' ' + @orderDir;

EXEC sys.sp_executesql @sql, N'@pageNumber int, @pageSize int',
                   @pageNumber = @pageNumber, @pageSize = @pageSize;

GO

Você pode ver aqui, que o código é funcional e fornece a ordenação e paginação adequadas:

EXEC dbo.GetPersons @OrderBy = 'id', @orderDir = 'DESC';
EXEC dbo.GetPersons @OrderBy = 'id', @orderDir = 'ASC';
EXEC dbo.GetPersons @OrderBy = 'firstName';
EXEC dbo.GetPersons @OrderBy = 'lastName';
EXEC dbo.GetPersons @PageNumber = 2, @PageSize = 1, @OrderBy = 'lastName', @orderDir = 'ASC';

E veja também como a manipulação de entrada protege contra alguém tentando fazer coisas estranhas:

EXEC dbo.GetPersons @OrderBy = 'lastName', @orderDir = 'UP';
EXEC dbo.GetPersons @OrderBy = ';TRUNCATE TABLE Persons;';

Leitura Adicional

exemplo sp_executesql

Maus Hábitos de Aaron Bertrand para Chutar: Usando EXEC() em vez de sp_executesql

Procedimento da pia da cozinha de Aaron Bertrand

Um método comum para mitigar a injeção de SQL é usar QUOTENAMEem torno de variáveis ​​que são passadas para o procedimento armazenado.

Então, no seu exemplo, o código poderia ser modificado assim:

declare @sql varchar(max)
set @sql = 'select id, firstName, lastName
from (
    select id, firstName, LastName, row_number() over(order by '+@orderBy+' '+@orderDir+') as rn
    from Persons
    ) t
where rn > ('+cast(@pageNumber as varchar)+'-1) * '+cast(@pageSize as varchar)+'
        and rn <= '+cast(@pageNumber as varchar)+' * '+cast(@pageSize as varchar)+' 
order by '+ Quotename(@orderBy)+' '+@orderDir

Se alguém tentasse passar o comando extra 'delete', a execução apresentaria um erro porque o SQL dinâmico resultante se parece com isso:

select id, firstName, lastName
from (
    select id, firstName, LastName, row_number() over(order by id)a from Persons)t;delete from Persons;print' ) as rn
    from Persons
    ) t
where rn > (1-1) * 20
        and rn <= 1 * 20 
order by [id)a from Persons)t;delete from Persons;print'] 

resultando neste erro:

Msg 102, Level 15, State 1, Line 27
Sintaxe incorreta perto de ']'.

Além disso, Aaron Bertrand tem um ótimo blog sobre Bad Habits to Kick : Using EXEC() em vez de sp_executesql

Uma solução óbvia é não usar SQL dinâmico. Acho que sua tarefa pode ser realizada com código T-SQL regular e não dinâmico, que também oferece outras vantagens em termos de segurança (como encadeamento de propriedade).

Então, em vez de:

declare @sql varchar(max)
set @sql = 'select id, firstName, lastName
from (
    select id, firstName, LastName, row_number() over(order by '+@orderByName+') as rn
    from Persons
    ) t
where rn > ('+cast(@pageNumber as varchar)+'-1) * '+cast(@pageSize as varchar)+'
        and rn <= '+cast(@pageNumber as varchar)+' * '+cast(@pageSize as varchar)+' 
order by '+@orderByName

exec(@sql)

Você poderia, por exemplo..

SELECT id, firstName, lastName
FROM (
    SELECT id, firstName, lastName, ROW_NUMBER() OVER (
        ORDER BY (CASE @OrderByName
                  WHEN 1 THEN id END),
                 --- different datatypes, I'm assuming
                 (CASE
                  WHEN 2 THEN firstName
                  WHEN 3 THEN lastName END)) AS rn
    FROM persons
    ) AS t
WHERE rn > (@pageNumber-1) * @pageSize
  AND rn <= @pageNumber * @pageSize
ORDER BY rn;

Leia-se,

• Procedimento da pia da cozinha de Aaron Bertrand
• Encadeamento de propriedade
• OFFSET FETCH

A resposta de @Scott Hodgin toca nisso, mas basicamente a melhor abordagem ao gerar sequências SQL dinâmicas voltadas para o cliente/aplicativo é utilizar sp_executesql .

Embora não seja totalmente infalível para eliminar ataques de SQL Injection, sp_executesql é provavelmente o melhor que você obterá. O artigo que Scott linka por Aaron Bertrand é bastante direto, mas para resumir rapidamente os benefícios do sp_executesql sobre outras abordagens é que:

1. Utiliza variáveis ​​fortemente tipadas dentro da string
2. Tem melhor chance de reutilização do plano de consulta

O primeiro ponto é o que eu sinto que é o mais importante relacionado à sua pergunta, porque você pode limitar o tamanho, o tipo, etc. dos seus parâmetros. Isso torna incrivelmente mais difícil injetar código desagradável.

Para fornecer uma resposta ainda mais completa, atualizei seu sp de acordo. Interessante o suficiente, no seu caso, porque você está tentando parametrizar literais de coluna, precisará aninhar instruções sp_executesql, para que a primeira instrução aninhada defina nomes de coluna como literais e a segunda execução passe os valores de paginação, da seguinte maneira:

create procedure GetPersons @pageNumber int = 1, @pageSize int = 20, @orderBy varchar(50) = 'id', @orderDir varchar(4) = 'desc'
as

if @orderDir not in ('asc', 'desc') or @orderBy not in ('id', 'firstName', 'lastName')
begin
    raiserror('Cheater!', 16,1)
    return
end

declare @sql nvarchar(max), @sql_out nvarchar(max)
set @sql = 'SELECT @sql_out = ''select id, firstName, lastName
from (
    select id, firstName, LastName, row_number() over(order by '' + @oB + '' '' + @oD + '') as rn
    from Persons
    ) t
where rn > (@pN -1) * @pS
        and rn <= @pN * @pS
order by '' + @oB + '' '' + @oD + '''''

--PRINT(@sql)
EXEC sp_executesql @sql, N'@oB varchar(50), @oD varchar(4), @sql_out nvarchar(max) OUTPUT', @oB=@orderBy, @oD=@orderDir, @sql_out=@sql_out OUTPUT
EXEC sp_executesql @sql_out, N'@pN int, @pS int', @pN=@pageNumber, @pS=@pageSize

Opção simples - junte-se a sys.columnspara certificar-se de que é um nome de coluna válido e use CASEcomo padrão ASCse algo diferente DESCfor passado.

(oh, e use nvarchar(max)para @sqle sp_executesql)

declare @sql nvarchar(max)
select @sql = 'select id, firstName, lastName
  from (
    select id, firstName, LastName, row_number() over(order by '+QUOTENAME(c.name)+' '+ CASE WHEN @orderDir = 'DESC' THEN 'DESC' ELSE 'ASC' END +') as rn
    from Persons
    ) t
  where rn > ('+cast(@pageNumber as varchar)+'-1) * '+cast(@pageSize as varchar)+'
    and rn <= '+cast(@pageNumber as varchar)+' * '+cast(@pageSize as varchar)+' 
order by '+QUOTENAME(c.name)+' '+ CASE WHEN @orderDir = 'DESC' THEN 'DESC' ELSE 'ASC' END
FROM sys.columns c
WHERE c.name = @orderby
AND c.object_id = OBJECT_ID('dbo.Persons');

EXEC sp_executesql @sql;