How do I fix this Stored XSS?

Question

The CheckMarx security scanner says that this line is a Stored XSS vulnerability

<apex:column style="background-color:#{!trade.bgcolor};color:{!trade.fgcolor};">

is this HTMLENCODE the correct fix or should it be JSENCODE?

<apex:column style="background-color:#{!HTMLENCODE(trade.bgcolor)};color:{!HTMLENCODE(trade.fgcolor)};">

Thanks!

Answer 1

You want HTMLENCODE, that's the correct operator for use in CSS.

Answer 2

This is a great question. An HTML Style attribute passes the value to the CSS parser, much like eventhandler attributes ('onclick') pass the value to the JS Parser.

In the case of a merge-field in the value of an onclick attribute, you would use JSENCODE, but in the case of a 'style' attribute -- what do you use?

Unfortunately, nothing! VisualForce does not have a CSS encode function, as the encoding would only prevent data from breaking out of a quoted string, but many CSS property values are not quoted.

So unlike JSENCODE, CSS encoding doesn't help a whole lot, and you are left with aggressive white-listing:

If you want an RGB value, make sure the field is an RGB value, etc. That's all you need to do, and if the Checkmarx scanner complains, then don't worry about the false positive. Btw, the scanner should not be flagging this -- you may want to report it by sending an email to [email protected]

Now some people here have mentioned that HTMLENCODE is the way to go. I think the reason for this belief is that the HTML parser is going to process the html tag first, and only once everything is tokenized will the result be sent to the CSS parser. So if you could inject a double quote into the merge-field, you could escape the quoted attribute value and enter a Javascript eventhandler to execute code:

attack-vector: " onmouseover="my_payload"

So yes, the merge-field does need to be HTMLENCODED, but the platform does that for you automatically! Everything outside of a <script> or <style> tag is automatically HTMLENCODED (and a style attribute is not the same as a style tag). If you apply the VF HTMLENCODE function, you will end up double encoding.

More importantly, as you need to be whitelisting anyways, you should only be passing, say, RGB hex values to the color field, so HTMLENCODE operations are null-ops, right?

In short, aggressively whitelist the values that go into style attributes of apex tags, but do not HTMLENCODE them

I hope the above makes sense. If not, please book an office hour with the security team. CSS encoding can be tricky.