What are the dangers of dynamic SQL, and can they be avoided?

Question

We've just been given the following code as a solution for a complicated search query in a new application provided by offshore developers. I'm skeptical of the use of dynamic SQL because I could close the SQL statement using '; and then excute a nasty that will be performed on the database!

Any ideas on how to fix the injection attack?

ALTER procedure [dbo].[SearchVenues] --'','',10,1,1,''
@selectedFeature as varchar(MAX),
@searchStr as varchar(100),
@pageCount as int,
@startIndex as int,
@searchId as int,
@venueName as varchar(100),
@range int,
@latitude varchar(100),
@longitude varchar(100),
@showAll int,
@OrderBy varchar(50),
@SearchOrder varchar(10)

AS
DECLARE @sqlRowNum as varchar(max)
DECLARE @sqlRowNumWhere as varchar(max) 
DECLARE @withFunction as varchar(max)
DECLARE @withFunction1 as varchar(max)
DECLARE @endIndex as int
SET  @endIndex = @startIndex + @pageCount -1

SET @sqlRowNum = ' SELECT Row_Number() OVER (ORDER BY '

IF @OrderBy = 'Distance'
    SET @sqlRowNum =  @sqlRowNum  + 'dbo.GeocodeDistanceMiles(Latitude,Longitude,' + @latitude + ',' + @longitude + ') ' +@SearchOrder
ELSE
    SET @sqlRowNum =  @sqlRowNum + @OrderBy + ' '+ @SearchOrder

SET @sqlRowNum = @sqlRowNum + ' ) AS RowNumber,ID,RecordId,EliteStatus,Name,Description,
Address,TotalReviews,AverageFacilityRating,AverageServiceRating,Address1,Address2,Address3,Address4,Address5,Address6,PhoneNumber,
visitCount,referalCount,requestCount,imgUrl,Latitude,Longitude,
Convert(decimal(10,2),dbo.GeocodeDistanceMiles(Latitude,Longitude,' + @latitude + ',' + @longitude + ')) as distance
FROM VenueAllData '

SET @sqlRowNumWhere = 'where Enabled=1 and EliteStatus <> 3 ' 

--PRINT('@sqlRowNum ='+@sqlRowNum)
IF  @searchStr <> ''
BEGIN

    IF (@searchId = 1)    -- county search
    BEGIN
       SET @sqlRowNumWhere  = @sqlRowNumWhere +  ' and Address5 like ''' + @searchStr + '%'''
    END
    ELSE IF(@searchId = 2  ) -- Town search
    BEGIN
       SET @sqlRowNumWhere  = @sqlRowNumWhere +  ' and Address4 like ''' + @searchStr + '%'''
    END  
    ELSE IF(@searchId = 3  ) -- postcode search
    BEGIN
       SET @sqlRowNumWhere  = @sqlRowNumWhere +  ' and Address6 like ''' + @searchStr + '%'''
    END    

    IF (@searchId = 4)   -- Search By Name
    BEGIN
        IF @venueName <> ''
            SET @sqlRowNumWhere  = @sqlRowNumWhere +  ' and ( Name like ''%' + @venueName + '%'' OR Address like ''%'+ @venueName+'%'' ) '
        ELSE
            SET @sqlRowNumWhere  = @sqlRowNumWhere +  ' and  ( Name like ''%' + @searchStr + '%'' OR Address like ''%'+ @searchStr+'%'' ) '
    END
END


IF @venueName <> '' AND @searchId <> 4
    SET @sqlRowNumWhere  = @sqlRowNumWhere +  ' and ( Name like ''%' + @venueName + '%'' OR  Address like ''%'+ @venueName+'%'' ) '


set @sqlRowNum = @sqlRowNum +  ' '   + @sqlRowNumWhere 


--PRINT(@sqlRowNum)

IF @selectedFeature <> ''
    BEGIN
        DECLARE @val1 varchar (255)
        Declare @SQLAttributes varchar(max)
        Set @SQLAttributes = ''
        Declare @tempAttribute varchar(max)
        Declare @AttrId int
        while (@selectedFeature <> '')
            BEGIN
                SET @AttrId = CAST(SUBSTRING(@selectedFeature,1,CHARINDEX(',',@selectedFeature)-1) AS Int)
                Select @tempAttribute = ColumnName from Attribute where id = @AttrId
                SET @selectedFeature = SUBSTRING(@selectedFeature,len(@AttrId)+2,len(@selectedFeature))
                SET @SQLAttributes = @SQLAttributes + ' ' + @tempAttribute + ' = 1 And '
            END
        Set @SQLAttributes = SUBSTRING(@SQLAttributes,0,LEN(@SQLAttributes)-3)
        set @sqlRowNum = @sqlRowNum +  ' and ID in  (Select VenueId from '
        set @sqlRowNum = @sqlRowNum +  ' CachedVenueAttributes WHERE ' + @SQLAttributes + ')  '

    END

IF @showAll <> 1
    set @sqlRowNum = @sqlRowNum +  ' and  dbo.GeocodeDistanceMiles(Latitude,Longitude,' + @latitude + ',' + @longitude + ')   <=  ' +  convert(varchar,@range )


set @withFunction = 'WITH LogEntries AS (' + @sqlRowNum +  ')

SELECT * FROM  LogEntries WHERE RowNumber between '+ Convert(varchar,@startIndex) + 
' and ' + Convert(varchar,@endIndex) + ' ORDER BY ' + @OrderBy + ' ' + @SearchOrder


print(@withFunction)
exec(@withFunction)

Answer 1

As an aside, I would not use EXEC; rather I would use sp_executesql. See this superb article, The Curse and Blessings of Dynamic SQL, for the reason and other info on using dynamic sql.

Answer 2

See this answer.

Also, these:

Am I immune to SQL injections if I use stored procedures?

Avoiding SQL Injection in SQL query with Like Operator using parameters?

Can I protect against SQL Injection by escaping single-quote and surrounding user input with single-quotes?

Answer 3

Here's an optimized version of the query above that doesn't use dynamic SQL...

Declare @selectedFeature as varchar(MAX),
@searchStr as varchar(100),
@pageCount as int,
@startIndex as int,
@searchId as int,
@venueName as varchar(100),
@range int,
@latitude varchar(100),
@longitude varchar(100),
@showAll int,
@OrderBy varchar(50),
@SearchOrder varchar(10)

Set @startIndex = 1
Set @pageCount = 50



Set @searchStr = 'e'
Set @searchId = 4
Set @OrderBy = 'Address1'
Set @showAll = 1
--Select dbo.GeocodeDistanceMiles(Latitude,Longitude,@latitude,@longitude)


DECLARE @endIndex int
SET  @endIndex = @startIndex + @pageCount -1
;

WITH LogEntries as (
SELECT 
    Row_Number() 
        OVER (ORDER BY 
            CASE @OrderBy
               WHEN 'Distance' THEN Cast(dbo.GeocodeDistanceMiles(Latitude,Longitude,@latitude,@longitude) as varchar(10))
               WHEN 'Name' THEN Name
               WHEN 'Address1' THEN Address1
               WHEN 'RecordId' THEN Cast(RecordId as varchar(10))
               WHEN 'EliteStatus' THEN Cast(EliteStatus as varchar(10))
            END) AS RowNumber,
RecordId,EliteStatus,Name,Description,
Address,TotalReviews,AverageFacilityRating,AverageServiceRating,Address1,Address2,Address3,Address4,Address5,Address6,PhoneNumber,
visitCount,referalCount,requestCount,imgUrl,Latitude,Longitude,
Convert(decimal(10,2),dbo.GeocodeDistanceMiles(Latitude,Longitude,@latitude,@longitude)) as distance
FROM VenueAllData 
where Enabled=1 and EliteStatus <> 3
And 
    (
        (Address5 like @searchStr + '%' And @searchId = 1) OR
        (Address4 like @searchStr + '%' And @searchId = 2) OR
        (Address6 like @searchStr + '%' And @searchId = 3) OR
        (
            (
                @searchId = 4 And 
                    (Name like '%' + @venueName + '%' OR Address like '%'+ @searchStr+'%')
            )
        )
    )
And
    ID in (
        Select VenueID 
        From CachedVenueAttributes 
        --Extra Where Clause for the processing of VenueAttributes using @selectedFeature
    )
And
    (   
        (@showAll = 1) Or
        (@showAll <> 1 and dbo.GeocodeDistanceMiles(Latitude,Longitude,@latitude,@longitude) <= convert(varchar,@range )) 
    )
)

SELECT * FROM  LogEntries 
WHERE RowNumber between @startIndex and @endIndex 
ORDER BY CASE @OrderBy
               WHEN 'Distance' THEN Cast(Distance as varchar(10))
               WHEN 'Name' THEN Name
               WHEN 'Address1' THEN Address1
               WHEN 'RecordId' THEN Cast(RecordId as varchar(10))
               WHEN 'EliteStatus' THEN Cast(EliteStatus as varchar(10))
            END

The only thing I haven't fixed is the selection from CachedVenueAttributes that seems to build up a where statement in a loop. I think I might put this in a table valued function, and refactor it in isolation to the rest of the procedure.

Answer 4

I like dynamic SQL for search.

Where I have used it in the past I have used .Net prepared statements with any user generated string being passed in as a parameter NOT included as text in the SQL.

To run with the existing solution you can do a number of thing to mitigate risk.

1. White list input, validate input so that it can only contain a-zA-Z0-9\w (alpha numerics and white space) (bad if you need to support unicode chars)
2. Execute any dynamic sql as a restricted user. Set owner of stored proc to a user which has only read access to the tables concerned. deny write to all tables ect. Also when calling this stored proc you may need to do it with a user with similar restrictions on what they can do, as it appares MS-SQL executes dynamic sql within a storedproc as the calling user not the owner of the storedproc.

Answer 5

I've realized that this is a really old post, but when doing things like:

    AND
    (   
        (@showAll = 1) 
        OR (@showAll <> 1 
            AND dbo.GeocodeDistanceMiles(Latitude,Longitude,@latitude,@longitude) <= convert(varchar,@range)) 
    )

... an OPTION(RECOMPILE) will usually help pick a more concise plan, as long as it's not going to be executed a thousand times per second or anything.