CODEWITHSUNDEEP
coldfusioncoldfusion-2016
Steve
Steve

Reputation: 3095

Coldfusion alternative to evaluate() due to possible code injection?

Currently just finishing up a code review and the auditor doesn't like this use of evaluate() and assigns it a high risk due to possible code injection.

The user is presented a form of products associated with his or her account. There's a hidden input with a valuelist of the product IDs. There are then radio inputs to change the status of products. There could be anywhere from 1 to several products listed. Those inputs are all named r_#productid#:

<form>
   <input type="hidden" name="prodIdList" value="#valueList(prodIds)#"/>
   <input type="radio" id="dn_#pr_prid#" name="r_#pr_prid#" value="X" checked="checked"/>
   <input type="radio" id="dn_#pr_prid#" name="r_#pr_prid#" value="P"/>
   <input type="radio" id="dn_#pr_prid#" name="r_#pr_prid#" value="L"/>
</form>

When submitted the code loops over the form.prodIdList and evalutes those IDs to get the submitted value (X, P or L).

<cfif StructKeyExists(FORM,"doProcessChanges")>
  <cfloop list="#FORM.assetIdList#" index="i">
    <cfswitch expression="#Evaluate('FORM.r_' & i)#">
      <cfcase value="P">
        --- do something i=productId ---
      </cfcase>
      <cfcase value="L">
        --- do something else i=productId  ---  
      </cfcase>
    </cfswitch> 
  </cfloop>
</cfif>

Is there an alternate way of accomplishing this that doesn't use Evaluate and will satisfy this code reviewer?

[edit] One change I did was to evaluate and then check the values vs. an expected list or regex. I hadn't thought of array notation and will give that a try as well. For now here's the first update:

gender = evaluate('form.gender_' & i);
    if( gender == 'M' || gender == 'F' || gender == 'O' || gender == 'X' ) {
    -- do stuff
    } else {
    -- error 
    };

Upvotes: 0

Views: 900

Answers (2)

rrk
rrk

Reputation: 15846

You can use array notation like this FORM['r_' & i] instead of Evaluate('FORM.r_' & i). I think this is a duplicate question. I'll flag if I can find the original.

Upvotes: 4

James A Mohler
James A Mohler

Reputation: 11120

One of the things that can be done to make this code safer is to use encodeForHTMLAttribute()

Furthermore, it should be scoped to whatever generated it. I image a query created this, so a query's name should be used

<form>
   <input type="hidden" name="prodIdList" value="#EncodeForHTMLAttribute(valueList(qry.prodIds))#"/>
   <input type="radio" id="dn_#EncodeForHTMLAttribute(qry.pr_prid)#" name="r_#EncodeForHTMLAttribute(qry.pr_prid)#" value="X" checked="checked"/>
   <input type="radio" id="dn_#EncodeForHTMLAttribute(qry.pr_prid)#" name="r_#EncodeForHTMLAttribute(qry.pr_prid)#" value="P"/>
   <input type="radio" id="dn_#EncodeForHTMLAttribute(qry.pr_prid)#" name="r_#EncodeForHTMLAttribute(qry.pr_prid)#" value="L"/>
</form>

Upvotes: 2

Related Questions