[SalesForce] Checkmarx Reflected XSS with getParameter

I have a visualforce page, which can have a parameter caseId.

For example :
https://myInstance/apex/myApp?caseId=500c000000DFyxW

The VF page creates a lightning component, but needs to pass case Id as parameter. Following is code for my page, and my server side controller –

Page –

<apex:page tabStyle="Case" controller="myController"
           title="Some Title" sidebar="false" showHeader="false" wizard="false"  cache="false" 
           applyHtmlTag="false" applyBodyTag="false" docType="html-5.0">

    <apex:includeLightning />
    <head>
        <meta name="viewport" content="width=device-width, initial-scale=1.0"/>
        <title>Some Title</title>
    </head>
    <body>

        <div id="lightning"></div>
        <script>

        $Lightning.use("c:someApp", function() {
            $Lightning.createComponent("c:someComponent",{{!lightningParameters}},"lightning",function(cmp) {

            });
        });
        </script>
    </body>

</apex:page>

Code for my server side controller –

public class myController {

    public String lightningParameters{
        get{
            if(lightningParameters == null){
                String unsanitizedCaseId = ApexPages.currentPage().getParameters().get('caseId');
                if(unsanitizedCaseId!=null){
                    //String sanitizedCaseId = String.escapeSingleQuotes(unsanitizedCaseId);
                    String sanitizedCaseId = unsanitizedCaseId.escapeHtml4();
                    lightningParameters = '"caseId" : "'+ sanitizedCaseId + '"';
                }else{
                    lightningParameters = '';
                }
            }
            return lightningParameters;
        }
        private set;
    }

}

It was an existing code with no sanitizing. Initial code just read the parameter, and constructed a string to be returned. From my understanding of the code, if the caseId parameter is present, it returns following string

"caseId" : "500c000000DFyxW"

and when there is no parameter, it returns empty string.

In my code, as you can see, i have tried two approaches to sanitize it –
String sanitizedCaseId = String.escapeSingleQuotes(unsanitizedCaseId);
String sanitizedCaseId = unsanitizedCaseId.escapeHtml4();

But no matter what, checkmarx gives the exact same report –

Method get{ at line 4 of src/classes/myController.cls gets a client-side controlled data for the getparameters element. This element’s value is used in client-side code without being properly sanitized or validated and is eventually integrated into the HTML code in $Lightning.createComponent at line 30 of src/pages/someVFPage.page.

I am not sure what i need to do to address the vulnerability. Just for kicks, i tried modifying the page, enclosing the lightningparameter inside JSENCODE, which does make vulnerability go away in checkmarx scan, but breaks the functionality of my app

Best Answer

You need to use JSENCODE by itself, without trying to make your own JSON string, or you need to serialize the entire JSON object and use that instead.

The reason why stripHtml4 and escapeSingleQuotes won't work is because they are the wrong solution to the problem at hand. If you don't encode the string, you can inject any sort of script you want from the URL, like this:

https://myinstance/apex/myApp?caseId=%22%2Cparam2%3A%20alert(%22Hello%20World%22)%2C%20param3%3A%20%22

Note: This may require fiddling with escaping some characters, but the principle is correct. This is a security vulnerability.

Try it out, you'll see why this can be a Bad Thing.

As an alternative:

lightningParameters = JSON.serialize(
  new Map<String, String> {
    'caseId' => unsanitizedCaseId
  }
);

From here, you need to not use the object notation:

    $Lightning.use("c:someApp", function() {
      $Lightning.createComponent(
        "c:someComponent",
        {!lightningParameters},
        "lightning",
        function(cmp) {

        });
    });

There's still a possibility that you might get a security alert from the scanner this way, too, but it should be impossible to actually inject code this way, since everything will be safely escaped inside a valid JSON structure.

Related Topic