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

See this answer.

Also, these:

http://stackoverflow.com/questions/239905/am-i-immune-to-sql-injections-if-i-use-stored-procedures

http://stackoverflow.com/questions/228476/avoiding-sql-injection-in-sql-query-with-like-operator-using-parameters

http://stackoverflow.com/questions/139199/can-i-protect-against-sql-injection-by-escaping-single-quote-and-surrounding-us

Answer 2

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