I have a stored procedure that we are scanning using some AI tools to look for vulnerabilities. I am doing a dynamic SQL statement with an order by clause and that parameter for that order by I am running through this within a Try Catch:
IF COL_LENGTH('dbo.TableName', @OrderByColumn) IS NULL
BEGIN
RAISERROR('OrderByColumn not a recognized column', 16, 1);
END
IF (UPPER(@OrderByType) != 'ASC' AND UPPER(@OrderByType) != 'DESC')
BEGIN
RAISERROR('OrderByType not a recognized direction', 16, 1);
END
Later I build dynamic SQL using concat
SET @OrderBySql = CONCAT(' Order By ', @OrderByColumn, ' ', @OrderByType) ;
The AI tool says this can be exploited and I just don't think so.
SQL injection: The stored procedure constructs dynamic SQL by concatenating user-provided parameters like @OrderByColumn and @OrderByType directly into the query string. While there is validation using COL_LENGTH, this approach is inherently risky. An attacker could potentially bypass the validation or exploit edge cases to inject malicious SQL code. The safer approach would be to use a whitelist of allowed column names and directions, or use parameterized queries with proper escaping.
Even if you check the column exists, you still need to correctly quote it, in case it contains spaces or other strange characters.
SET @OrderBySql = CONCAT(' ORDER BY ', QUOTENAME(@OrderByColumn), ' ', @OrderByType);
"a whitelist of allowed column names" could indeed be a bit safer, although I think the risk is small to non-existent with correct quoting.
Parameterization is impossible with ORDER BY, so clearly your AI tool doesn't know what it's talking about (as usual).
On a side note: you should use THROW instead of RAISERROR, as it has better error handling semantics.