0

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.

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

  • The thing that gets me is that the getParameters() and the DML() parts of the flow are separated by the post back of the apex:commandButton. I'll check through the report to see if there are other flows where a GET request could result in a DML operation. I think there was another path for it that could be justified. I.e. a field is being updated with a default value from a custom setting that was unrelated to the URL parameter. – Daniel Ballinger Jan 22 '15 at 8:35
  • RIght, but an attacker can load the page directly. They do not need to load the page as a result of clicking the button. – Robert Sussland Jan 29 '15 at 23:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.