CodeQL XSS False Positives and XSS AutoFix incorrect location for defensive encoding

[davewichers]

Select Topic Area

Product Feedback

Body

I'd like to report an XSS false positive for CodeQL AND the AutoFix for it.

If you look at the CodeQL XSS finding in my test project here: https://github.com/aspectsecurity/TestGitHubAdvSec/security/code-scanning/8

It is saying there is an XSS when writing the filename parameter out to the response, and YET that parameter value is wrapped in a call to: org.owasp.esapi.ESAPI.encoder().encodeForHTML(), which makes is safe.

I also have the same code in a private repo, where CodeQL is reporting this same XSS, and the AutoFix suggestion is to change this:

+ org.owasp.esapi.ESAPI.encoder().encodeForHTML(fileName));

To this:

+ org.owasp.esapi.ESAPI.encoder().encodeForHTML(org.owasp.esapi.ESAPI.encoder().encodeForHTML(fileName)));

Which is clearly redundant/duplicative.

A similar XSS false positive is also reported here:
https://github.com/aspectsecurity/TestGitHubAdvSec/security/code-scanning/9, where it is encoding the value of 'param' on line 72.

But the autofix for this same file in my private repo, suggests this change:

60: param = java.net.URLDecoder.decode(theCookie.getValue(), "UTF-8"); (Original code)

Suggested fix:

60: param = org.owasp.benchmark.helpers.Utils.encodeForHTML(java.net.URLDecoder.decode(theCookie.getValue(), "UTF-8"));

The problem with FIXING XSS like this way earlier than the point where the parameter value is echoed in the web response is that encoding it like this can BREAK the use of that parameter between this line and where it echoed in the web response.

In this test case 4, the value is put into session and could be used anywhere, so encoding it BEFORE it is put into session can have serious adverse effects.

In another similar test case, the same autofix is suggested to be applied to a filename parameter before the filename is ever used. HTML Encoding a filename parameter can definitely screw up the use of that filename.

The RIGHT way to do XSS defenses is ONLY encode the value just BEFORE it is included in the web response. Encoding it too early can easily break stuff.

Here's a Stack Exchange discussion on why you should encode as late as possible: https://security.stackexchange.com/questions/261081/is-there-a-consensus-on-whether-html-encoding-should-happen-upon-upload-or-retri

"Encoding/escaping should happen as late as possible." The reason for this is you have to know the HTML/JavaScript context to know what the right type of encoding to use is, and encoding too early corrupts the data for other uses that have nothing to do with using those variable values outside of a browser context.

[AlonaHlobina]

Hi @davewichers, Thank you very much for the feedback! We very appreciate it.
cc @turbo @tiferet