Take the 2-minute tour ×
Salesforce Stack Exchange is a question and answer site for Salesforce administrators, implementation experts, developers and anybody in-between. It's 100% free, no registration required.

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.

share|improve this question

2 Answers 2

up vote 5 down vote accepted

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.

share|improve this answer
    
I still don't buy it as being an actual vulnerability, though, since the emitted code would already be pre-escaped correctly. But definitely I'd parse it in the browser side if only because I'd expect the browser to do the right thing and convert values to native types. –  sfdcfox Aug 9 at 1:19
    
You're right, it's not in this case as long as nothing else ever manipulates the same string. –  ca_peterson Aug 9 at 1:35
    
Many thanks for the tips :) I still don't buy the "eval is evil" in this context (stackoverflow.com/questions/1843343/json-parse-vs-eval) but if it'll shut the scanner up... As for support - caniuse.com/json I guess? We're stuck with IE8 support till Summer '15 and my S1-friendly pages are added as Chatter actions to the "desktop" version too so looks like I have some more testing to do... –  eyescream Aug 10 at 12:18

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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