Checkmarx and JSON.serialize

Question

My Salesforce1 code has been recently run through Force.com Security Scanner (a.k.a. Checkmarx). A stored XSS vulnerability has been found and marked as critical security risk.

It boils down to something like this:

// controller
public String getRecent(){
    return JSON.serialize([SELECT Id, Type, Name
        FROM RecentlyViewed
        WHERE Type IN ('Account', 'Contact')
        ORDER BY LastViewedDate DESC]);
}

// JS on VF page
var recent = {!recent};

Checkmarx recommends using JSENCODE, HTMLENCODE, URLENCODE etc to fix the vulnerability and shut the scanner up.

My question: really? I think it's a false positive. In what situation JSON.serialize would fail to properly escape quotes and / or html tags?

And if it's legit - how can I protect this case without screwing up the data to a point where parsing it back / unescaping becomes a pain?

I could fudge it by URLENCODING and decoding back I guess but I'd like to know if there's a real life scenario I should be worried about. After all 1 parse error and the relevant bit of my JS won't run.

Answer 1

Your page's javascript is effectively using the eval style of deserializing JSON which has been frowned upon in general ever since browsers started having pretty widespread support for JSON.parse.

If you don't need to support old versions of IE (and if you do there's polyfils) it's really, really a better idea to do something like

var recent = JSON.parse('{!JSENCODE(recent)}');

Is it really that much better from a security perspective? Not really dramatically in your case, but in more complex situations it can possibly have a tangible impact.

Also, since JSON is a subset of javascript in modern browsers it's not uncommon to see a performance boost using JSON methods over eval-style. JSON methods also handle escape characters that might be invalid in other places.

Answer 2

Regardless of the possible effort you may have made already in APEX, in my experience checkmarx is expecting always any {!...} output in visualforce to be escaped using JSENCODE, HTMLENCODE, URLENCODE - that ruleset feels quite simple.

So there may be false positives and you should be able to discuss it with the security review guys.

Usually I just clamp my teeth and go on double-escaping the string and unescaping it back with JS. As said by @ca_peterson it could add a slight future looking security improvement in case you might ever add parts or further processing to your string (after JSON.serialize and before {!...}).

In my use cases and with Firefox 30.0 the performance didn't change at all using the additional escaping/unescaping or not. This may vary by the amount of data and browser, but I wouldn't expect much.