Security: Dynamic Order By clause using sp_executeSql

Question

Post reply

Security: Dynamic Order By clause using sp_executeSql

Tom257

SSChampion

Points: 11543

August 17, 2023 at 4:38 pm

#4263777

This is a contrived example - but illustrates what I'm trying to fix

I have a code where the client can change the sort order via a parameter.

DECLARE @pageTop INT = 1
DECLARE @pageBottom INT = 10
DECLARE @OrderBy NVARCHAR(500) = 'object_id desc';

DECLARE @SQL NVARCHAR(MAX) = '
SELECT * ,ROW_NUMBER() OVER(ORDER BY ' + @OrderBy + ') AS [RowNo] INTO #orderedCols FROM sys.columns  ORDER BY ' + @OrderBy + ';SELECT * FROM #orderedCols WHERE RowNo BETWEEN @PageTop AND @PageBottom ORDER BY RowNo
'
EXEC sp_executeSql @SQL, N'@PageTop INT, @PageBottom INT', @pageTop, @pageBottom

And this is vulnerable to injection attacks as follows

DECLARE @pageTop INT = 1
DECLARE @pageBottom INT = 10
DECLARE @OrderBy NVARCHAR(500) = 'name) FROM sys.database_principals;PRINT ''you have been hacked!''--'

DECLARE @SQLUnsafe NVARCHAR(MAX) = '
SELECT * ,ROW_NUMBER() OVER(ORDER BY ' + @OrderBy + ') AS [RowNo] INTO #orderedCols FROM sys.columns  ORDER BY ' + @OrderBy + '; SELECT * FROM #orderedCols WHERE RowNo BETWEEN @PageTop AND @PageBottom ORDER BY RowNo
'

EXEC sp_executeSql @SQLUnsafe, N'@PageTop INT, @PageBottom INT', @pageTop, @pageBottom

The Order by parameter cannot be included in the sp_executeSql parameters - this does not work

DECLARE @pageTop INT = 1
DECLARE @pageBottom INT = 10
DECLARE @OrderBy NVARCHAR(500) = 'object_id desc';

DECLARE @SQL_givesError NVARCHAR(MAX) = '
SELECT * ,ROW_NUMBER() OVER(ORDER BY @OrderBy) AS [RowNo] INTO #orderedCols FROM sys.columns ORDER BY @OrderBy; SELECT * FROM #orderedCols WHERE RowNo BETWEEN @PageTop AND @PageBottom ORDER BY RowNo
'
EXEC sp_executeSql @SQL_givesError, N'@OrderBy NVARCHAR(500), @PageTop INT, @PageBottom INT', @OrderBy, @pageTop, @pageBottom

Error is:

Msg 1008, Level 16, State 1, Line 57

The SELECT item identified by the ORDER BY number 1 contains a variable as part of the expression identifying a column position. Variables are only allowed when ordering by an expression referencing a column name.

How can I parameterize @OrderBy to protect against the injection attack

August 20, 2023 at 7:42 pm

This was removed by the editor as SPAM

Viewing 8 posts - 1 through 7 (of 7 total)

You must be logged in to reply to this topic. Login to reply

Phil Parkin SSC Guru Points: 246991 More actions · Answer 1

It's a bit hacky and not as user-friendly, but if you were to create two arguments as follows

@SortColumnCode TINYINT

@SortOrder BIT = 0

and then decode the SortColumnCode number to a column name inside your proc, the injection threat goes away because the datatype is enforced. But you'd have to provide a Code/Column Name list to the client.

The absence of evidence is not evidence of absence.
Martin Rees

You can lead a horse to water, but a pencil must be lead.
Stan Laurel

ratbak Ten Centuries Points: 1383 More actions · Answer 2

Maybe a TVP w/ SortColumnName and Direction/SortOrder columns? SortOrder could be constrained to 'ASC' & 'DESC', or '+' & '-'

With validation in the procedure that SortColumnName is actually a valid column name

Phil Parkin SSC Guru Points: 246991 More actions · Answer 3

ratbak wrote:

--
With validation in the procedure that SortColumnName is actually a valid column name

I thought about this too. But this validation needs to be written carefully, also to avoid the possibility of injection.

The absence of evidence is not evidence of absence.
Martin Rees

You can lead a horse to water, but a pencil must be lead.
Stan Laurel

Tom257 SSChampion Points: 11543 More actions · Answer 4

For completeness: code for Phil's solution

public enum SortColumn (
Name = 1,
Object_Id = 2
)
public enum SortDirection (
Ascending = 0,
Descending = 1
)
public class SortSubClause (
public SortColumn SortColumn {get; set;}
public SortDirection SortDirection {get; set;} = SortDirection.Ascending
)
public class SortOrder (
public IEnumerable<SortSubClause> SortClauses {get; set;}
)
// build TVP type udt_SortOrder from SortOrder

CREATE TYPE udt_SortOrder AS TABLE (
SortColumnId TINYINT NOT NULL,
SortDirection BIT NOT NULL
)

-- in the PROC add parameter udt_SortOrder

-- example
DECLARE @SortClauses [udt_SortOrder]
INSERT INTO @SortClauses (SortColumnId, SortDirection) VALUES
(1,1) ,(2,0)

DECLARE @OrderBy NVARCHAR(MAX);
SELECT @OrderBy = STUFF ((
SELECT ', ' + CASE SortColumnId WHEN 1 THEN '[Name]'
WHEN 2 THEN '[object_id]' 
       END + ' ' +
       CASE WHEN SortDirection = 1 THEN 'DESC' ELSE '' END  
FROM @SortClauses  
FOR XML PATH ('') ),1,2,'')
SELECT @OrderBy

Advantage - deals with the injection threat - and client can only use valid sort parameters.

But the disadvantage is maintenance in two places

Steve Collins SSCrazy Points: 2176 More actions · Answer 5

Curious, why not create 2, 4, or ?? procs instead? Instead of enums in C#, a UDT, and dynamic SQL could you not get rid of the parameterization and just create a proc for each situation?

Aus dem Paradies, das Cantor uns geschaffen, soll uns niemand vertreiben können

frederico_fonseca SSCoach Points: 16167 More actions · Answer 6

unless the client is able to do their own sql, the list of possible columns, or even formulas used, would be available upfront on the UI - so allowing them to select for a list of possible column from within the output list, and specifying if ASC/DESC should be trivial.

that output number of the column (from output list) would then be the one supplied to the SP, or even inserted into a session table for use within the SP. Using the column ordinal instead of the name would make it trivial to validate data as only valid values would be either a number or ASC/DESC.