Need assistance with making a sproc SQL Injection proof

  • 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-28Linda 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-05add Hic/Medicaid search parameter = full value

    2013-02-08MemberID = full value search

    2013-02-18Search MemberID and QNXTMedid for the @MemberID value - causes 2 index seeks

    2013-0225Added input params for IsCoverageActive and IsPDPOnly

    2013-03-06changed datatypes for 2 new parms to char(1)

    2013-07-09add 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.

  • All I can recommend is Gails great article for catch-all-queries[/url]



    Lutz
    A pessimist is an optimist with experience.

    How to get fast answers to your question[/url]
    How to post performance related questions[/url]
    Links for Tally Table [/url] , Cross Tabs [/url] and Dynamic Cross Tabs [/url], Delimited Split Function[/url]

  • 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

  • Quite some room for improvements, start by moving the parameters to the sp_executesql params and validating the inputs.

    😎

  • LutzM (4/21/2014)


    All I can recommend is Gails great article for catch-all-queries[/url]

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

    Change is inevitable... Change for the better is not.


    Helpful Links:
    How to post code problems
    How to Post Performance Problems
    Create a Tally Function (fnTally)

  • 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

  • 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

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

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

    Change is inevitable... Change for the better is not.


    Helpful Links:
    How to post code problems
    How to Post Performance Problems
    Create a Tally Function (fnTally)

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

  • MarbryHardin (4/24/2014)


    ... And you'll have to come saw my foot off to get me to write dynamic SQL. 😉

    Then you are discounting one of the best tools for dealing with a number of data processing problems (of which the open-ended-search is a prime example). I have used it any number of times to achieve 4-5 ORDER OF MAGNITUDE performance gains while simultaneously bringing concurrency up from a molasses-in-February pace.

    Best,
    Kevin G. Boles
    SQL Server Consultant
    SQL MVP 2007-2012
    TheSQLGuru on googles mail service

  • There are other considerations beyond performance alone that discount the use of dynamic SQL. Speaking of writing it "right" in the first place.

    Never say never, but it is something to be avoided.

Viewing 12 posts - 1 through 11 (of 11 total)

You must be logged in to reply to this topic. Login to reply