How to counter this SQL Injection risk (Coldfusion)?

user_4405732 0
Viewed 138

Recently, a security risk company we hired identified a SQL injection risk for a specific parameter of a function in our Coldfusion application. The argument is correctly captured as a cfargument:

<cfargument name="status" required="false" type="string">

and also correctly paramed as a cfprocparam:

<cfif isDefined('ARGUMENTS.status') AND ARGUMENTS.status NEQ "">
    <cfprocparam  type="in" value="#ARGUMENTS.status#" dbvarname="@search_status" cfsqltype="cf_sql_varchar">
<cfelse>
    <cfprocparam  type="in" null="yes" dbvarname="@search_status" cfsqltype="cf_sql_varchar">
</cfif>

And then it's used in the stored proc this way, building a where clause for a SQL query:

if (len(@search_status) > 0)
    begin
        set @strWhere = @strWhere + ' AND e1.cps_txt LIKE '+ '''%'+@search_status+'%'''
    end

Of course, paraming to make sure the value is a varchar can't prevent a string of malicious code being entered and passed. This seems like it could easily be a larger problem, too, since it's far from the only place in our application where string arguments are passed to cfc functions and then used in SQL queries.

I'm wondering if the best approach to remedy the security risk in this one case is to compare the argument's value to all of the possible expected values (there's about twenty of them, stored in a table), and if it doesn't match one of them, then to throw an error, or just pass to the stored proc as null.

coldfusion sql-injection
2 Answers

Had to tackle this at a previous employer. The trick is to pass true or false to the null argument and to avoid writing all this conditional code that calls different versions of the procparam with or without the attribute. Just always have the attribute and put the conditional logic in the value.

<cfprocparam type="in" 
    dbvarname="@search_status"
    cfsqltype="cf_sql_varchar"
    value="#ARGUMENTS.status#"
    null="#!(structKeyExists(arguments, "status") AND (len(arguments.status) NEQ 0))#">

Depending on the complexity of the condition, you can simply pass it as a variable.

<cfset statusNull = true>
<cfif structKeyExists(arguments, "status") AND (len(arguments.status) NEQ 0)>
    <cfset statusNull = false>
</cfif>

<cfprocparam type="in" 
    dbvarname="@search_status"
    cfsqltype="cf_sql_varchar"
    value="#ARGUMENTS.status#"
    null="#statusNull#">

And I'd like to reiterate the comment from SOS that mentions dynamic SQL. We also eliminated dynamic SQL from thousands of stored procedures and replaced it with the conditional check from his linked message. This is so much easier to read and debug than dealing with acres of string concatenation code.

WHERE ( @OrderId IS NULL OR tblOrder.OrderId = @OrderId )
AND   ( @OrderCustomer IS NULL OR tblOrder.OrderCustomer = @OrderCustomer )

I would also avoid adding a query just to look up valid values for status before passing the argument to the this function/proc. That just adds overhead to you system for every call to this process.

user_110470

I think you pretty much answered your own question. Here's an approach I've used in the past to handle validations using accepted values stored in a small lookup table. Do this before calling your stored procedure. I'll use generic names since I don't know your table and column names.

<!--- Validation code prior to running stored proc --->
<cfif isDefined('ARGUMENTS.status') AND ARGUMENTS.status NEQ "">
    <!--- Query acceptable values --->
    <cfquery name="qValidateSearchStatus" datasource="myDsn">
    select
        search_status
    from
        lookup_table
    </cfquery>

    <cfif listFind(valueList(qValidateSearchStatus.search_status), ARGUMENTS.status)>
        <cfset statusValid = true>
        <cfset statusNull = false>
    <cfelse>
        <cfset statusValid = false>
        <cfset statusNull = false>
    </cfif>

<cfelse>
    <cfset statusValid = true>
    <cfset statusNull = true>
</cfif>

<cfif statusValid>
    <!--- Call stored procedure --->
    <cfstoredproc procedure="myStoredProc" datasource="myDsn"> 

        <cfif statusNull>
            <cfprocparam  type="in" null="yes" dbvarname="@search_status" cfsqltype="cf_sql_varchar">
        <cfelse>
            <cfprocparam  type="in" value="#ARGUMENTS.status#" dbvarname="@search_status" cfsqltype="cf_sql_varchar">
        </cfif>

    </cfstoredproc>

<cfelse>
    <!--- Process error handling --->
</cfif>
user_120311190
Related