sql-serversql-injectiondynamic-sql

Is COL_LENGTH sufficient for parameter sanitation


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.


Solution

  • 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.