March 18, 2009 at 10:38 am
Hi,
I've got an SP that looks like this:
CREATE PROCEDURE USPDeleteAnswers
@Id varchar(50),
@QuestionId varchar(100)
AS
BEGIN
DECLARE @SQL nvarchar(255)
SET @SQL = 'DELETE FROM tblUserAnswer where Id = @Id$ and QuestionId in (@QuestionId$)'
EXEC sp_executesql
@SQL,
N'@Id$ varchar(50), @QuestionId$ varchar(100)',
@Id$ = @Id,
@QuestionId$ = @QuestionId
END
GO
Now my problem is that I need to send several comma separated numbers to @QuestionId, ie. "12, 13, 14" so I can delete them all this in one go. But this is throwing an error "Error converting data type varchar to real"
Any ideas on how I could do this please?
Thanks
March 18, 2009 at 11:01 am
el_wise (3/18/2009)
Hi,I've got an SP that looks like this:
CREATE PROCEDURE USPDeleteAnswers
@Id varchar(50),
@QuestionId varchar(100)
AS
BEGIN
DECLARE @SQL nvarchar(255)
SET @SQL = 'DELETE FROM tblUserAnswer where Id = @Id$ and QuestionId in (@QuestionId$)'
EXEC sp_executesql
@SQL,
N'@Id$ varchar(50), @QuestionId$ varchar(100)',
@Id$ = @Id,
@QuestionId$ = @QuestionId
END
GO
Now my problem is that I need to send several comma separated numbers to @QuestionId, ie. "12, 13, 14" so I can delete them all this in one go. But this is throwing an error "Error converting data type varchar to real"
Any ideas on how I could do this please?
Thanks
this is what i use:
here is the function:
REATE FUNCTION [dbo].[fnConvertToTableInt] (@ItemList as varchar(5000))
RETURNS @ParsedList table(UID int, Item int)
AS
BEGIN
DECLARE @Item varchar(50)
DECLARE @Pos int
DECLARE @UID int
SET @UID = 0
SET @ItemList = LTRIM(RTRIM(@ItemList))+ ','
SET @Pos = CHARINDEX(',', @ItemList, 1)
IF REPLACE(@ItemList, ',', '') <> ''
BEGIN
WHILE @Pos > 0
BEGIN
SET @Item = LTRIM(RTRIM(LEFT(@ItemList, @Pos - 1)))
IF (Select isNumeric(@Item)) = 1
BEGIN
INSERT INTO @ParsedList (Item,UID)
VALUES (CAST(@Item AS int) ,@UID)
END
SET @ItemList = RIGHT(@ItemList, LEN(@ItemList) - @Pos)
SET @Pos = CHARINDEX(',', @ItemList, 1)
SET @UID = @UID + 1
END
END
RETURN
END
you then call the function passing in your IDs as a string
select * from fnconverttotableint('101,201,301')
this will return the following table
UID ITEM
0......101
1.......201
2.......301
in your case you would then do something like
SET @SQL = 'DELETE FROM tblUserAnswer where Id = @Id$ and QuestionId in (Select Item from fnconverttotableint(@QuestionId))'
March 18, 2009 at 11:52 am
March 18, 2009 at 12:20 pm
Thanks,
bitbucket, that is the way I had it coded before, but that seems to be an easy target for SQL injection?
what do you reckon?
Thanks again
March 18, 2009 at 12:29 pm
bitbucket (3/18/2009)
Might I suggest:
CREATE PROCEDURE USPDeleteAnswers@Id varchar(50),
@QId varchar(100)
AS
DECLARE @SQL nvarchar(255)
SET @SQL = 'DELETE FROM tblUserAnswer where Id = ''' + @Id + ''' and QuestionId in (' + @QId + ' )'
PRINT @SQL -- delete after testing
EXEC sp_executesql @SQL
Test run as:
Dbo.USPDeleteAnswers 'abcdeid', '1,5,20'
Then alter the SP to delete the print statement
Note that each passed parameter is passed in character / varchar format, that is do not forget the ' ' surrounding each passed value.
Since the title of this thread is “Avoiding injection on stored procedure”, have you considered what would happen if they passed the following as parameters:
Execute USPDeleteAnswers @Id = 'abc', @QuestionId = ' 1,3 ) or 1 = 1 -- '
March 18, 2009 at 1:22 pm
March 18, 2009 at 2:07 pm
bitbucket (3/18/2009)
Michael Valentine Jones
Since the title of this thread is “Avoiding injection on stored procedure”, have you considered what would happen if they passed the following as parameters:
[Execute USPDeleteAnswers @Id = 'abc', @QuestionId = ' 1,3 ) or 1 = 1 --
dbo.USPDeleteAnswers 'abc', '1=1' OR dbo.USPDeleteAnswers 'abc', 1=1
Will produce:
Msg 102, Level 15, State 1, Line 1
Incorrect syntax near '='.
Trying: DELETE FROM tblUserAnswer where Id = '*' and QuestionId in (1,3 )
Gives: (0 row(s) affected)
To further strengthen the SP set the length value for @Id to its length as defined in the database table, and of course set it in the tables definition to a realistic length and not the default value when creating a table using SSMS.
Also set the length for the parameter @QuestionId to a realistic value, remembering that when the passed value is longer than the defined length of the parameter in the procedure it is truncated.
The stored proc call example I posted before would result in the following dynamic SQL being executed, and would delete all the rows in the table.
DELETE FROM tblUserAnswer where Id = 'abc' and QuestionId in (1,3 ) or 1 = 1 -- )
el_wise has very good reason to be skeptical of that code. Much worse could be done, depending on the access level of the account running this command.
March 18, 2009 at 9:48 pm
March 18, 2009 at 9:59 pm
March 18, 2009 at 10:41 pm
You really seem to be missing the point, and your posts didn't address the specific example I posted.
I just ran this script creating the table, loading some rows into it, creating your stored proc, followed by a two executes of proc USPDeleteAnswers with my input parameters.
print '--- Create table tblUserAnswer'
create table tblUserAnswer (Id varchar(20), QuestionId int not null)
go
print '--- Load 3 rows into the table tblUserAnswer'
insert into tblUserAnswer select '1', 2 union all select '3',4 union all select '5',6
go
CREATE PROCEDURE USPDeleteAnswers
@Id varchar(50),
@QId varchar(100)
AS
DECLARE @SQL nvarchar(255)
SET @SQL = 'DELETE FROM tblUserAnswer where Id = ''' + @Id + ''' and QuestionId in (' + @QId + ' )'
PRINT @SQL ---- delete after testing
EXEC sp_executesql @SQL
go
print '--- Delete all rows from tblUserAnswer'
Execute USPDeleteAnswers @Id = 'abc', @QId = ' 1,3 ) or 1 = 1 -- '
go
print '--- Get count of remaining rows in table tblUserAnswer'
select count(*) from tblUserAnswer
go
print '--- Delete all rows from tblUserAnswer and then drop table tblUserAnswer '
Execute USPDeleteAnswers @Id = 'abc', @QId = ' 1,3 ) or 1 = 1; drop table tblUserAnswer -- '
go
print '--- Verify table tblUserAnswer has been deleted'
exec sp_help 'tblUserAnswer'
go
drop PROCEDURE USPDeleteAnswers
go
-- drop table if it still exists
if object_id('tblUserAnswer','U') is not null drop table tblUserAnswer
Notice the following results it produced:
--- Create table tblUserAnswer
--- Load 3 rows into the table tblUserAnswer
(3 row(s) affected)
--- Delete all rows from tblUserAnswer
DELETE FROM tblUserAnswer where Id = 'abc' and QuestionId in ( 1,3 ) or 1 = 1 -- )
(3 row(s) affected)
--- Get count of remaining rows in table tblUserAnswer
-----------
0
(1 row(s) affected)
--- Delete all rows from tblUserAnswer and then drop table tblUserAnswer
DELETE FROM tblUserAnswer where Id = 'abc' and QuestionId in ( 1,3 ) or 1 = 1; drop table tblUserAnswer -- )
(0 row(s) affected)
--- Verify table tblUserAnswer has been deleted
Msg 15009, Level 16, State 1, Procedure sp_help, Line 66
The object 'tblUserAnswer' does not exist in database 'tempdb' or is invalid for this operation.
The DELETE statement produced by the first call was a valid statement that resulted in deleting all rows in table tblUserAnswer because the 1 = 1 condition is always true.
The DELETE statement produced by the second call was actually able to drop the table.
As you can see, this was a really routine SQL Injection 101 attack.
March 19, 2009 at 2:54 am
When I worked on SQL Server 2K, what we'd do for this sort of thing is pass in the items to be deleted as a string - in xml format.
Then we'd inflate that xml into a table variable. Finally we'd delete the items in the real table where the ID was in the table variable.
No dynamic SQL. No SQL Injection. Job Done.
March 19, 2009 at 7:37 am
Michael Valentine Jones
Many thanks - I have learned from your examples --- I am glad you stuck to your guns and had the patience to teach this dummy (me) to write better T-SQL.
Again thanks
March 19, 2009 at 7:42 am
Don't use dynamic SQL for this kind of thing. Use a string parser to break up the list of values into a table.
That's kind of already been said here, but I just want to reinforce the message. It's very important.
- Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
Property of The Thread
"Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon
April 6, 2009 at 2:35 pm
This may not be the sexy answer, but the best way I know to prevent an injection attack for a dynamic sql procedure is to remove the dynamic SQL! Change your procedure to handle 1 "Answer" row at a time and then have your Front End application loop over the answer keys to change the parameter values for your stored procedure in your data layer. At this point, you are creating a discrete delete statement for each row (much like LINQ would do).
Alternatively, in the data access layer, you could validate each comma separated value to make sure you are only accepting integers. That would prevent the injection of:
(23,24,25,26) and 1=1; delete from etc --
since any additional text users might input would cause an exception in your application or be thrown out, depending on your exception logic.
April 7, 2009 at 2:28 pm
Michael Valentine Jones (3/18/2009)
bitbucket (3/18/2009)
Might I suggest:
CREATE PROCEDURE USPDeleteAnswers@Id varchar(50),
@QId varchar(100)
AS
DECLARE @SQL nvarchar(255)
SET @SQL = 'DELETE FROM tblUserAnswer where Id = ''' + @Id + ''' and QuestionId in (' + @QId + ' )'
PRINT @SQL -- delete after testing
EXEC sp_executesql @SQL
Test run as:
Dbo.USPDeleteAnswers 'abcdeid', '1,5,20'
Then alter the SP to delete the print statement
Note that each passed parameter is passed in character / varchar format, that is do not forget the ' ' surrounding each passed value.
Since the title of this thread is “Avoiding injection on stored procedure”, have you considered what would happen if they passed the following as parameters:
Execute USPDeleteAnswers @Id = 'abc', @QuestionId = ' 1,3 ) or 1 = 1 -- '
Michael, I am now thinking I may have offered a poorly thought out solution to a recent post. Do you see any way of doing damage through my solution at http://www.sqlservercentral.com/Forums/Topic689208-8-1.aspx, which also uses dynamic SQL? If so, could you post it to that post?
Greg
_________________________________________________________________________________________________
The glass is at one half capacity: nothing more, nothing less.
Viewing 15 posts - 1 through 14 (of 14 total)
You must be logged in to reply to this topic. Login to reply