Click here to monitor SSC
SQLServerCentral is supported by Red Gate Software Ltd.
 
Log in  ::  Register  ::  Not logged in
 
 
 
        
Home       Members    Calendar    Who's On


Add to briefcase 12»»

Need assistance with making a sproc SQL Injection proof Expand / Collapse
Author
Message
Posted Monday, April 21, 2014 11:05 AM
Old Hand

Old HandOld HandOld HandOld HandOld HandOld HandOld HandOld Hand

Group: General Forum Members
Last Login: Friday, July 18, 2014 1:57 PM
Points: 313, Visits: 715
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.
Post #1563562
Posted Monday, April 21, 2014 11:12 AM


SSCertifiable

SSCertifiableSSCertifiableSSCertifiableSSCertifiableSSCertifiableSSCertifiableSSCertifiableSSCertifiableSSCertifiable

Group: General Forum Members
Last Login: Today @ 11:03 AM
Points: 7,019, Visits: 12,912
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
Post #1563566
Posted Monday, April 21, 2014 12:20 PM


SSCarpal Tunnel

SSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal Tunnel

Group: General Forum Members
Last Login: 2 days ago @ 8:36 AM
Points: 4,319, Visits: 6,112
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 at GMail
Post #1563578
Posted Monday, April 21, 2014 12:28 PM
Ten Centuries

Ten CenturiesTen CenturiesTen CenturiesTen CenturiesTen CenturiesTen CenturiesTen CenturiesTen Centuries

Group: General Forum Members
Last Login: Today @ 2:24 PM
Points: 1,243, Visits: 3,608
Quite some room for improvements, start by moving the parameters to the sp_executesql params and validating the inputs.
Post #1563585
Posted Monday, April 21, 2014 5:20 PM


SSC-Dedicated

SSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-Dedicated

Group: General Forum Members
Last Login: Today @ 11:31 AM
Points: 36,728, Visits: 31,180
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."

(play on words) "Just because you CAN do something in T-SQL, doesn't mean you SHOULDN'T." --22 Aug 2013

Helpful Links:
How to post code problems
How to post performance problems
Post #1563648
Posted Wednesday, April 23, 2014 3:49 PM
Grasshopper

GrasshopperGrasshopperGrasshopperGrasshopperGrasshopperGrasshopperGrasshopperGrasshopper

Group: General Forum Members
Last Login: Wednesday, July 16, 2014 2:32 PM
Points: 19, Visits: 96
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

Post #1564468
Posted Wednesday, April 23, 2014 7:34 PM


SSCarpal Tunnel

SSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal TunnelSSCarpal Tunnel

Group: General Forum Members
Last Login: 2 days ago @ 8:36 AM
Points: 4,319, Visits: 6,112
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 at GMail
Post #1564491
Posted Thursday, April 24, 2014 6:49 AM
Grasshopper

GrasshopperGrasshopperGrasshopperGrasshopperGrasshopperGrasshopperGrasshopperGrasshopper

Group: General Forum Members
Last Login: Wednesday, July 16, 2014 2:32 PM
Points: 19, Visits: 96
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.
Post #1564628
Posted Thursday, April 24, 2014 8:14 AM


SSC-Dedicated

SSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-DedicatedSSC-Dedicated

Group: General Forum Members
Last Login: Today @ 11:31 AM
Points: 36,728, Visits: 31,180
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."

(play on words) "Just because you CAN do something in T-SQL, doesn't mean you SHOULDN'T." --22 Aug 2013

Helpful Links:
How to post code problems
How to post performance problems
Post #1564666
Posted Thursday, April 24, 2014 10:19 AM
Grasshopper

GrasshopperGrasshopperGrasshopperGrasshopperGrasshopperGrasshopperGrasshopperGrasshopper

Group: General Forum Members
Last Login: Wednesday, July 16, 2014 2:32 PM
Points: 19, Visits: 96
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.
Post #1564752
« Prev Topic | Next Topic »

Add to briefcase 12»»

Permissions Expand / Collapse