Source Scanner Cross Site Request Forgery (XSRF) spanning apex:commandButton

Question

I'm getting results back from the source scanner for XSRF issues where the result path spans an apex:commandButton.

A number of the XSRF paths in my report follow a pattern:

1. In the controllers constructor a query string request parameter is read and stored in a member (non-volatile, so it will go via the view state).
2. An apex:commandButton calls a controller Apex method
3. The apex method subsequently performs a DML operation, such as updating an Opportunity. The member from step 1 may influence the DML operation.

Example XSRF involving POST request:

XSRF result path 47:   Path Id: -1811245813  
Object: get in file: unpackaged\classes\XyzController.cls  
L 2059:    addNewRecord = Integer.valueOf(System.currentPageReference().getParameters().get('addNewRecord'));  
Object: valueof in file: unpackaged\classes\XyzController.cls  
L 2059:    addNewRecord = Integer.valueOf(System.currentPageReference().getParameters().get('addNewRecord'));  
Object: addNewRecord in file: unpackaged\classes\XyzController.cls  
L 2059:    addNewRecord = Integer.valueOf(System.currentPageReference().getParameters().get('addNewRecord'));  
Object: loadSaveRecord63 in file: unpackaged\pages\SaveRecord.page  
L 166:    <apex:commandButton value="Save Selected Records" action="{!saveMultipleOli}" rendered="{! multipleRecordsSelected && !enableMultiLineRecordEditing}"/>  
Object: savemultipleoli62 in file: unpackaged\pages\SaveRecord.page  
L 166:    <apex:commandButton value="Save Selected Records" action="{!saveMultipleOli}" rendered="{! multipleRecordsSelected && !enableMultiLineRecordEditing}"/>  
Object: savemultipleoli in file: unpackaged\classes\XyzController.cls  
L 3271:    PageReference pr = saveMultipleOli(RecordMapper);  
Object: opp in file: unpackaged\classes\XyzController.cls  
L 3659:    update opp;  
Object: update in file: unpackaged\classes\XyzController.cls  
L 3659:    update opp;    

My understanding was that Salesforce provides XSRF protection for POST requests, such as those that originate from a apex:commandButton.

Is the problem that the query string parameter is ultimately returned to the method via view state and could be used in updating the record?

I’m confident enough to flag these as false positives, especially due to the ultimate usage of the query string parameter. It seems to me this would be an extremely common outcome after reading a query string parameter and that the ViewStateCSRF form value in the post back should be sufficient.

Answer 1

The issue is a bit complex. Yes, the path cited is a false positive, however in general Checkmarx will flag these only when there is a real issue. The error is that it is reporting too many paths. Here is what I mean:

CSRF generally happens when you have a page action that performs a DML operation. That means you create a page, e.g. saveAccount.page, so that when this page is loaded the account is saved. Now, on another page, fooAccount.page, you may have an apex form or button so that, when clicked, a page reference to saveAccount.page is returned.

The above is vulnerable to CSRF, because an attacker can load the saveAccount.page directly without using your form/button, and trigger the account update. You have put a big lock on your front door, but left the windows unlocked, so no one needs to use the form/button on fooAccount.page to load saveAccount.page

Your intended flow is:

apex button --> pageReference --> getParameters() --> DML()

But the vulnerable flow is:

getParameters() --> DML()

And often times Checkmarx will report both the intended flow and the vulnerable flow as two different paths.

Therefore the short answer is that yes, this flow is a false positive, but you are still vulnerable because of another flow.

Hope that makes sense. If you have more questions, please book an office hour with the security team.

http://security.force.com/security/contact/ohours