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: 246960 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
The absence of consumable DDL, sample data and desired results is, however, evidence of the absence of my response
- Phil Parkin

ratbak Ten Centuries Points: 1358 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: 246960 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
The absence of consumable DDL, sample data and desired results is, however, evidence of the absence of my response
- Phil Parkin

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: 2157 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: 16154 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.