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

Question

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 
    };

Possible duplicate of [Dynamic Variable Naming and Reference (ColdFusion)](https://stackoverflow.com/questions/18410726/dynamic-variable-naming-and-reference-coldfusion) — rrk, Nov 01 '18 at 12:46
Also duplicate of https://stackoverflow.com/questions/44075509/trying-to-replace-all-evaluate-functions-with-dynamic-notation-in-coldfusion-9 — Cory Fail, Nov 01 '18 at 13:36
Btw, most all system scopes like FORM, URL, etc.. are structures, so you can always use associative array notation to access keys dynamically, ie — SOS, Nov 01 '18 at 13:54
Possible duplicate of [Is ColdFusion evaluate() really dangerous?](https://stackoverflow.com/questions/20179728/is-coldfusion-evaluate-really-dangerous) — James A Mohler, Nov 01 '18 at 14:40
@JamesAMohler - CF2016. Whether it's really dangerous or not isn't really up to me. The company doing the code audit doesn't like it as it is so I have to change it. See edits above for changes that I implemented. — Steve, Nov 02 '18 at 12:38
One suggestion about the "edit" code, when comparing many values, it is shorter to use listFind() or listFindNoCase() rather than many OR conditions. — SOS, Nov 06 '18 at 00:09

rrk · Accepted Answer · 2018-11-02T13:09:59.263

4

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.

edited Nov 02 '18 at 13:09

answered Nov 01 '18 at 12:43

rrk

15,677
4
29
45

1

You have an extra ' up there. I ended up going with this solution: gender = FORM['gender_' & i]; – Steve Nov 02 '18 at 12:54

score 2 · Answer 2 · answered Nov 01 '18 at 14:45

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>

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

2 Answers2