SQL Clone
SQLServerCentral is supported by Redgate
 
Log in  ::  Register  ::  Not logged in
 
 
 


Need assistance with making a sproc SQL Injection proof


Need assistance with making a sproc SQL Injection proof

Author
Message
dndaughtery
dndaughtery
SSCrazy
SSCrazy (2.7K reputation)SSCrazy (2.7K reputation)SSCrazy (2.7K reputation)SSCrazy (2.7K reputation)SSCrazy (2.7K reputation)SSCrazy (2.7K reputation)SSCrazy (2.7K reputation)SSCrazy (2.7K reputation)

Group: General Forum Members
Points: 2669 Visits: 1093
Hey guys, its beena while since I've been on a team where there is a web app so I'm having to knock the dust off of some things I haven't had to worry about in the last few years. Here's my question:

Say I have the following sproc:


USE [MyDatabase]
GO
/****** Object: StoredProcedure [dbo].[SearchMember] Script Date: 04/21/2014 11:20:38 ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO

ALTER procedure [dbo].[SearchMember](
@memberID varchar(16) = null,
@LastName varchar(60) = null,
@Firstname varchar(35) = null,
@SSN varchar(11) = null,
@DOB char(10) = null,
@MedicaidID varchar(60) = null,
@IsCoverageActive char(1) = null,
@IsPDPOnly char(1) = null,
@IsInpatientAllowed char(1) = null,
@maxcnt int = 1000,
@returnCnt int OUTPUT,
@debug bit = 0
)
as
/***********************************************************************************
Object: dbo.SearchMember
created: 2013-01-28 Linda Parrish
Purpose: search for member in MemberSearch table
Uses dynamic SQL to build query

Notes: Assumes input values for MemberID, last and firstname parms
are the BEGINNING characters of stored values. This will then leverage
indexes on those fields.
2013-02-05 add Hic/Medicaid search parameter = full value
2013-02-08 MemberID = full value search
2013-02-18 Search MemberID and QNXTMedid for the @MemberID value - causes 2 index seeks
2013-0225 Added input params for IsCoverageActive and IsPDPOnly
2013-03-06 changed datatypes for 2 new parms to char(1)
2013-07-09 add input parm for IsInpatientAllowed and add to output

Example:
exec SearchMember @memberID=null, @LastName='smith', @FirstName='jac', @SSN=null, @DOB=null, @MedicaidID=null, @IsCoverageActive=0, @IsPDPOnly=0, @maxcnt=100, @returnCnt=100 , @debug=1

***********************************************************************************/
set nocount on;


declare @sql NVarchar(2000);
declare @sqlcnt NVarchar(2000);
declare @where nvarchar(2000);
declare @recCnt int;
declare @first bit;
set @first = 1;


set @sqlcnt = N'Select @recCnt = count(*) from MemberSearch where ';
declare @outparm nvarchar(30);
set @outparm = N'@recCnt int OUTPUT';

set @sql = N'Select MemberID
,QNXTMEMid
,LastName
,FirstName
,DOB
,SSN
,MedicaidID
,MedicareID
,IsPDPOnly
,IsReferralAllowed
,IsPreCertAllowed
,IsDual
,IsSinglePlanDual
,IsPPO
,IsCoverageActive
,IsInpatientAllowed
,s.SourceDescription as ApplicationSource from MyMembers m join MyMembers_System s
on m.sourcesystemid = s.sourceid where '

if nullif(@memberid, '') is not null
begin
if @first = 1
begin
set @where = N' (MemberID = ''' + convert(nvarchar,@memberid) + '''';
set @where = @where + N' or QNXTMemid = ''' + convert(nvarchar,@memberid) + ''')';
set @first = 0
end
end

if nullif(@lastname, '') is not null
begin
if @first = 1
begin
set @where = N' lastname like ''' + convert(nvarchar,@lastname) + '%'''
set @first = 0
end
else
set @where = @where + N' and lastname like ''' + convert(nvarchar,@lastname) + '%'''
end
if nullif(@firstname,'') is not null
begin
if @first = 0
set @where = @where + N' and firstname like ''' + convert(nvarchar,@firstname) + '%'''
else
begin
set @where = N' firstname like ''' + convert(nvarchar,@firstname) + '%'''
set @first = 0
end
end
if isdate(@dob) = 1
begin
if @first = 0
set @where = @where + N' and DOB = ''' + convert(nvarchar,@dob) + ''''
else
begin
set @where = N' DOB = ''' + convert(nvarchar,@dob) + ''''
set @first = 0
end
end
if nullif(@ssn, '') is not null
begin
if @first = 0
set @where = @where + N' and SSN = ''' + convert(nvarchar,@ssn) + ''''
else
begin
set @where = N' SSN = ''' + convert(nvarchar,@ssn) + ''''
set @first = 0
end
end
if nullif(@MedicaidID, '') is not null
begin
if @first = 0
set @where = @where + N' and MedicaidID = ''' + convert(nvarchar,@MedicaidID) + ''''
else
begin
set @where = N' MedicaidID = ''' + convert(nvarchar,@MedicaidID) + ''''
set @first = 0
end
end

if nullif(@IsCoverageActive, '') is not null
begin
if @first = 0
set @where = @where + N' and IsCoverageActive = ''' + convert(nvarchar,@IsCoverageActive) + ''''
else
begin
set @where = N' IsCoverageActive = ''' + convert(nvarchar,@IsCoverageActive) + ''''
set @first = 0
end
end

if nullif(@IsPDPOnly, '') is not null
begin
if @first = 0
set @where = @where + N' and IsPDPOnly = ''' + convert(nvarchar,@IsPDPOnly) + ''''
else
begin
set @where = N' IsPDPOnly = ''' + convert(nvarchar,@IsPDPOnly) + ''''
set @first = 0
end
end

if nullif(@IsInpatientAllowed, '') is not null
begin
if @first = 0
set @where = @where + N' and IsInpatientAllowed = ''' + convert(nvarchar,@IsInpatientAllowed) + ''''
else
begin
set @where = N' IsInpatientAllowed = ''' + convert(nvarchar,@IsInpatientAllowed) + ''''
set @first = 0
end
end

set @sql = @sql + @where;
set @sqlCnt = @sqlcnt + @where;
execute sp_executesql @sqlcnt, @outparm, @recCnt = @recCnt OUTPUT;
--select @recCnt;
set @returnCnt = @recCnt;

if @debug = 1
begin
print @sql
print @sqlcnt
print 'IsCoverageActive=' + cast(@IsCoverageActive as varchar(1))
print 'IsPDPOnly=' + cast(@IsPDPOnly as varchar(1))
print 'IsInpatientAllowed=' + cast(@IsInpatientAllowed as varchar(1))
print 'First = ' + cast(@first as char(10))
return
end
else
begin
if @recCnt <= @maxcnt
execute sp_executesql @sql
else
begin
--declare @inParm nvarchar(10);
--set @inParm = convert(nvarchar(10), @maxcnt);
--set @sql = N'Select top ' + @inParm + N' MemberID,QNXTMemID, LastName,FirstName,DOB,SSN,MedicaidID,MedicareID
-- ,IsPDPOnly,IsReferralAllowed,IsPreCertAllowed,IsDual,IsSinglePlanDual,IsPPO,IsCoverageActive,s.sourcedescription as ApplicationSource
-- from membersearch m join MemberSearch_SourceSystem s on m.sourcesystemid = s.sourceid where ' + @where
--execute sp_executesql @sql
declare @inParm nvarchar(25);
set @inParm = 'Select top ' + convert(nvarchar(10), @maxcnt + ' ');
set @sql = replace(@sql,'Select ',@inParm)
execute sp_executesql @sql
end
end

return;






How could I change the sproc to defend against SQL Injections? If I use Parameterized queries and the user doesn't submit a value for one of the filters in the where clause I dont want that filter to be considered.
LutzM
LutzM
SSC-Insane
SSC-Insane (23K reputation)SSC-Insane (23K reputation)SSC-Insane (23K reputation)SSC-Insane (23K reputation)SSC-Insane (23K reputation)SSC-Insane (23K reputation)SSC-Insane (23K reputation)SSC-Insane (23K reputation)

Group: General Forum Members
Points: 23515 Visits: 13559
All I can recommend is Gails great article for catch-all-queries



Lutz
A pessimist is an optimist with experience.

How to get fast answers to your question
How to post performance related questions
Links for Tally Table , Cross Tabs and Dynamic Cross Tabs , Delimited Split Function
TheSQLGuru
TheSQLGuru
SSC-Dedicated
SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)

Group: General Forum Members
Points: 32021 Visits: 8671
See Books Online for details about sp_executesql. You need to be using the fully parameterized capabilities:

EXEC sp_executesql [ @statement = ] statement
[
{ , [ @params = ] N'@parameter_name data_type [ OUT | OUTPUT ][ ,...n ]' }
{ , [ @param1 = ] 'value1' [ ,...n ] }
]



The good thing is that you don't have to do conditionals to build the parameter string. You can declare them all and just simply not pass in values for the one's that aren't used in your built-up @statement.

To my knowledge, fully parameterized TSQL is the ONLY WAY to guarantee protection from SQL Injection (at least without discarding valid input from the user such as CAST, EXEC, DECLARE, etc).

Best,
Kevin G. Boles
SQL Server Consultant
SQL MVP 2007-2012
TheSQLGuru on googles mail service
Eirikur Eiriksson
Eirikur Eiriksson
SSC-Forever
SSC-Forever (40K reputation)SSC-Forever (40K reputation)SSC-Forever (40K reputation)SSC-Forever (40K reputation)SSC-Forever (40K reputation)SSC-Forever (40K reputation)SSC-Forever (40K reputation)SSC-Forever (40K reputation)

Group: General Forum Members
Points: 40314 Visits: 19452
Quite some room for improvements, start by moving the parameters to the sp_executesql params and validating the inputs.
Cool
Jeff Moden
Jeff Moden
SSC Guru
SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)

Group: General Forum Members
Points: 213607 Visits: 41977
LutzM (4/21/2014)
All I can recommend is Gails great article for catch-all-queries


+1000 to that!

--Jeff Moden

RBAR is pronounced ree-bar and is a Modenism for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
Stop thinking about what you want to do to a row... think, instead, of what you want to do to a column.
If you think its expensive to hire a professional to do the job, wait until you hire an amateur. -- Red Adair

Helpful Links:
How to post code problems
How to post performance problems
Forum FAQs
MarbryHardin
MarbryHardin
Valued Member
Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)

Group: General Forum Members
Points: 67 Visits: 153
This has always been a sticking point for me, it always seemed to lead to "dirty" solutions. This is an approach I came up with to accommodate a flexible user interface. There are about 8 different fields in the original that can be selected from beyond the date range, and the user can select 0 - many items from each field.

This passes all of the selection into the proc in a table type parameter as name value pairs. If you needed to select two different departments for instance you would pass in two different records with the same parameter name, but different parameter values.

There haven't been any performance issues, but the primary table is only about 60,000 rows at this point. I do like that it allows for flexibility with a minimum of fuss, and no dynamic SQL.




CREATE PROCEDURE [dbo].[usp_GetUserSelection]
@paramVal dbo.tdt_ParameterData READONLY
AS
BEGIN
SET NOCOUNT ON;

-- Create internal parameter table so that we can modify values.
DECLARE @paramValues TABLE
(
ParameterName varchar(50),
ParameterValue varchar(200) NULL
);

-- Copy the incoming parameters over.
INSERT INTO @paramValues
(
ParameterName,
ParameterValue
)
SELECT
LTRIM(RTRIM(ParameterName)),
LTRIM(RTRIM(ParameterValue))
FROM @paramVal;

-- Get start and end date, these are the only required params.
DECLARE @startDate datetime = (SELECT CAST(ParameterValue as datetime) FROM @paramValues WHERE ParameterName = 'StartDate');
DECLARE @endDate datetime = (SELECT CAST(ParameterValue as datetime) FROM @paramValues WHERE ParameterName = 'EndDate');

-- Add a null value placeholder for any unused parameters.
IF NOT EXISTS (SELECT ParameterValue FROM @paramValues WHERE ParameterName = 'DeptNumber')
INSERT INTO @paramValues (ParameterName, ParameterValue) VALUES('DeptNumber', NULL);
IF NOT EXISTS (SELECT ParameterValue FROM @paramValues WHERE ParameterName = 'LocationCode')
INSERT INTO @paramValues (ParameterName, ParameterValue) VALUES('LocationCode', NULL);


SELECT
t.ItemID,
ISNULL(lm.LocationCode, '') LocationCode,
ISNULL(lm.[Description], '') LocationDesc,
ISNULL(lm.DeptNumber, '') DeptNumber,
ISNULL(dd.DepartmentDescription, '') DepartmentDescription
FROM dbo.Item t
LEFT OUTER JOIN dbo.Location lm ON t.LocationID = lm.LocationID
LEFT OUTER JOIN dbo.Department dd ON lm.DeptNumber = dd.DepartmentNumber
JOIN @paramValues p0 ON p0.ParameterName = 'LocationCode'
AND lm.LocationID = ISNULL(p0.ParameterValue, lm.LocationID)
JOIN @paramValues p1 ON p1.ParameterName = 'DeptNumber'
AND lm.DeptNumber = ISNULL(p1.ParameterValue, lm.DeptNumber)
WHERE
(
t.EndDate >= @startDate
AND t.StartDate <= @endDate
)
AND
(
(
p0.ParameterValue IS NOT NULL
OR p1.ParameterValue IS NOT NULL
)
OR
(
@startDate IS NOT NULL
AND @endDate IS NOT NULL
)
);

END


TheSQLGuru
TheSQLGuru
SSC-Dedicated
SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)SSC-Dedicated (32K reputation)

Group: General Forum Members
Points: 32021 Visits: 8671
Sorry MarbryHardin, but your solution is totally awful from a query optimization standpoint and you are guaranteed to get suboptimal performance. Read Gail's blog posts. Also, scale your table up to 6M or 600M rows and look at the query plans you get when you run your code. Also, watch how the plan will get reused with parameters that really need a different plan.

Oh, and your table variable isn't going to do you any favors either. I can give you a one column, one row table variable example that will give you a bad plan where the same in a temp table gives you the correct plan.

Best,
Kevin G. Boles
SQL Server Consultant
SQL MVP 2007-2012
TheSQLGuru on googles mail service
MarbryHardin
MarbryHardin
Valued Member
Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)

Group: General Forum Members
Points: 67 Visits: 153
The bulk of the execution plan cost comes out to a clustered index scan on the primary table. It's more than sufficient for our needs.

If I get the chance it would be nice to play with optimizing it against a large number of records.
Jeff Moden
Jeff Moden
SSC Guru
SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)SSC Guru (213K reputation)

Group: General Forum Members
Points: 213607 Visits: 41977
MarbryHardin (4/24/2014)
The bulk of the execution plan cost comes out to a clustered index scan on the primary table. It's more than sufficient for our needs.

If I get the chance it would be nice to play with optimizing it against a large number of records.


To be honest, those are famous last words. :-) No one ever goes back to plan for the inevitable scalability problems and, when such problems occur, it creates a drop-everything panic.

My recommendation would be that what is "sufficient" for your "needs" really isn't. Do it the right way now. It's not "pre-optimization"... it's just writing good code.

--Jeff Moden

RBAR is pronounced ree-bar and is a Modenism for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
Stop thinking about what you want to do to a row... think, instead, of what you want to do to a column.
If you think its expensive to hire a professional to do the job, wait until you hire an amateur. -- Red Adair

Helpful Links:
How to post code problems
How to post performance problems
Forum FAQs
MarbryHardin
MarbryHardin
Valued Member
Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)Valued Member (67 reputation)

Group: General Forum Members
Points: 67 Visits: 153
I know what you're saying, but I did a quick test loading 1.2 million rows to a test table which is about 10x what the production table should ever be based on usage and maintenance. It went from sub second to under 2 seconds, and that with unfavorably structured data. Not ideal, but there are some other optimizations that could be made.

And I will say that Gail's approach would be fine. If it fit. It does not allow for matching on an unknown number of values per field.

And you'll have to come saw my foot off to get me to write dynamic SQL. ;-)
Go


Permissions

You can't post new topics.
You can't post topic replies.
You can't post new polls.
You can't post replies to polls.
You can't edit your own topics.
You can't delete your own topics.
You can't edit other topics.
You can't delete other topics.
You can't edit your own posts.
You can't edit other posts.
You can't delete your own posts.
You can't delete other posts.
You can't post events.
You can't edit your own events.
You can't edit other events.
You can't delete your own events.
You can't delete other events.
You can't send private messages.
You can't send emails.
You can read topics.
You can't vote in polls.
You can't upload attachments.
You can download attachments.
You can't post HTML code.
You can't edit HTML code.
You can't post IFCode.
You can't post JavaScript.
You can post emoticons.
You can't post or upload images.

Select a forum

































































































































































SQLServerCentral


Search