A buddy of mine at work noticed that our stored procedures we use at work are sql injectable. Luckily they are called from the objects and are not based on user input. I started investigating the matter and I think I found a solution that might work. I was hoping that the blogsphere community could either shoot holes in it or confirm that it is a sound theory.
We are using stored procedures, but we are also using sp_executesql calls inside those stored procedures. I have posted an offending stored procedure below.
Create Procedure Showroom_Select
@ShowroomId int = Null
, @SpeedId int = Null
, @FieldList nvarchar(2000) = '*'
, @View nvarchar(100) = ''
, @OrderBy nvarchar(600) = Null
, @Debug int = 0
As
Set CONCAT_NULL_YIELDS_NULL Off
Declare @Select nvarchar(4000)
Declare @Where nvarchar(4000)
Declare @ConCat varchar(5)
Declare @PageRowCount int
Set @ConCat=''
Set @Where=''
If @SpeedId Is Not Null
Begin
Set @Where=@Where + @ConCat + 'SpeedId = @SpeedId'
Set @concat = ' And '
End
If @Where <>'' Set @Where = ' Where ' + @Where
Set @Select = @Select + 'Select '
Set @Select = @Select + @FieldList + ' From Showroom' + @View + ' '
If @OrderBy is not Null Select @Where = @Where + ' Order By ' + @OrderBy
Set @Select=@Select+@Where
If @Debug = 1
Select @Select
Else
Execute sp_ExecuteSQL @Select,
N'@ShowroomId int, @SpeedId int'
, @ShowroomId, @SpeedId
Now I purposely made this stored procedure simple. I colored the offending lines in red. The first line allows the caller to do something like @FieldList = '*; Drop Table Showroom' or @View = '_Wide; Select * From User'; we definitely don't want to allow this.
I have created a new stored procedure and it looks something like this...
Create Procedure Showroom_Select
@ShowroomId int = Null
, @SpeedId int = Null
, @FieldList nvarchar(2000) = '*'
, @View nvarchar(100) = ''
, @OrderBy nvarchar(600) = Null
, @Debug int = 0
As
Set CONCAT_NULL_YIELDS_NULL Off
Declare @Select nvarchar(4000)
Declare @Where nvarchar(4000)
Declare @ConCat varchar(5)
Declare @PageRowCount int
Declare @Error int
Set @ConCat=''
Set @Where=''
-- check that FieldList is clean of sql injection
If (PatIndex('%;%',@FieldList) > 0)
Begin
RAISERROR('Malformed sql statement detected.',16,1)
Set @error = 50000
RETURN @error
End
-- check that View is clean of sql injection
If (PatIndex('%;%',@View) > 0)
Begin
RAISERROR('Malformed sql statement detected.',16,1)
Set @error = 50000
RETURN @error
End
-- check that OrderBy is clean of sql injection
If (PatIndex('%;%',@OrderBy) > 0)
Begin
RAISERROR('Malformed sql statement detected.',16,1)
Set @error = 50000
RETURN @error
End
If @SpeedId Is Not Null
Begin
Set @Where=@Where + @ConCat + 'SpeedId = @SpeedId'
Set @concat = ' And '
End
If @Where <>'' Set @Where = ' Where ' + @Where
If @FieldList Is Not Null Set @Select = @Select + 'Select ' + @FieldList
If @View Is Not Null Set @Select = @Select + ' From Showroom' + @View
If @OrderBy Is Not Null Set @Where = @Where + ' Order By ' + @OrderBy
Set @Select=@Select+@Where
If @Debug = 1
Select @Select
Else
Execute sp_ExecuteSQL @Select,
N'@ShowroomId int, @SpeedId int'
, @ShowroomId, @SpeedId
This time I highlighted the "better" way to handle the injection text in green. This allows me to just return a sql error if it detects the batc
The thing that I am wondering is can this be defeated also. I tried some basic tests against sql injection and I couldn't penetrate it. I realize that I am not that competent when it comes to breaking into things though also. Any ideas? I am hoping Mr. Bouma will weigh in on how he handles things like this in his LLBLGen product. I know he doesn't use stored procedures and I do see some merit to his maintenance argument about stored procedures. I just don't know if I can drink the "No Stored Procedures' flavored Kool-Aid at work just yet.
Any ideas?