web: base64 encode user controlled strings (#1525) #1663
Conversation
|
Thanks @ArsenArsen . The CI failure can be fixed by adding Also that package seems to have quite a few deps, including ghc-byteorder which requires cabal-version (cabal file format) 2.2, which I believe means hledger would be no longer buildable with cabal-install <2.2 (adding a new potential roadblock for installers). Debian oldstable has cabal 2.2 though, so I guess that would be ok. There's also the base64-string package. Although older and probably unmaintained, it has no dependencies. |
|
Could you briefly summarise the limitations of this fix versus the "real fix" you mention ? Your commit message passed bin/commitlint, but not quite all of our guidelines - would you mind making it Also for the changelog, could you remind me briefly of how the vulnerability looked to users ? Eg, here's a draft:
|
|
PS if you can say without too much work which hledger version introduced the vulnerability, I'll mention that too. |
|
Would other GHC versions not also require the extra dep specified? To quote the base64 readme: "There are no dependencies aside from those bundled with GHC, text-short, and the ghc-byteorder re-export". The second approach has the advantage of preventing further XSS, to the This vulnerability appears to have been first introduced when An appropriate changelog entry for this vulnerability would be:
It's not fair to call this obfuscated JavaScript, it just uses some HTML laxness to go around an insufficient prevention previously added to the add form. And this works in any field that can be autocompleted via Bloodhound. |
ArsenArsen
force-pushed
the
master
branch
2 times, most recently
from
3f94269
to
00c3ef4
Compare
This fixes a reported Stored XSS vulnerability in toBloodhoundJson by encoding the user-controlled values in this payload into base64 and parsing them with atob. In my exploration of the vulnerability with various payloads I and others crafted, it would appear that this is the only available XSS in hledger-web in relation to stored accounts and transaction details. If there is other parts of the UI which may contain user-controlled data, they should be examined for similar things. In this instance, protections provided by yesod and other libraries worked fine, but in a bit of code that hledger-web was generating, the user could insert a </Script> tag (which is valid HTML and equivalent to </script> but not caught by the T.Replace that existed in toBloodhoundJson) in order to switch out of a script context, allowing the parser to be reset, and for arbitrary JavaScript to run. The real fix is a bit more involved, but produces much better results: Content-Security-Policy headers should be introduced, and using sha256-<hash of script> or a different algorithm, they should be marked as trusted in the header. This way, if the (in-browser) parser and hledger-web generator disagree on the source code of the script, the script won't run. Note that this would still be susceptible to attacks that involve changing the script by escaping from the string inside it or something similar to that, which can be avoided additionally by using either the method used in this commit, or a proper JSON encoder. The second approach has the advantage of preventing further XSS, to the extent specified above, in practice, a combination of both should be used, b64 for embedded data and the CSP sha256-hash script-src over everything else, which will eliminate all injected or malformed script blocks (via CSP), in combination with eliminating any HTML closing tags which might occur in stored data (via b64). This vulnerability appears to have been first introduced when autocompletion was added in hledger-web, git tag hledger-0.24, commit hash: ec51d28 Test payload: </Script><svg onload=alert(1)//> Closes simonmichael#1525
This has the advantage of there being no extra unpacking/packing of Data.Text to/from strings where it isn't necessary.
|
Great work, thanks for the vulnerability fix! |
|
Thank you @ArsenArsen and @simonmichael for the acknowledgement. I am wondering just for the record, can we file a CVE for the reported bug? |
|
Sure, is it needed ? The bug was fixed by 1.23. The install page may show which distros ship an older version. Is it something you can do ?
|
|
Yes, just to record version <1.23 was vulnerable to stored XSS. I will test the recent versions soon, when I get some free time. |
|
We disclose vulnerabilities only when it is fixed. So now that it is fixed, we can file for CVE. |
This fixes a reported Stored XSS vulnerability in toBloodhoundJson by
encoding the user-controlled values in this payload into base64 and
parsing them with atob.
In my exploration of the vulnerability with various payloads I and
others crafted, it would appear that this is the only available XSS in
hledger-web in relation to stored accounts and transaction details. If
there is other parts of the UI which may contain user-controlled data,
they should be examined for similar things. In this instance,
protections provided by yesod and other libraries worked fine, but in a
bit of code that hledger-web was generating, the user could insert a
</Script> tag (which is valid HTML and equivalent to </script> but not
caught by the T.Replace that existed in toBloodhoundJson) in order to
switch out of a script context, allowing the parser to be reset, and for
arbitrary JavaScript to run.
The real fix is a bit more involved, but produces much better results:
Content-Security-Policy headers should be introduced, and using
sha256- or a different algorithm, they should be marked
as trusted in the header. This way, if the (in-browser) parser and
hledger-web generator disagree on the source code of the script, the
script won't run. Note that this would still be susceptible to attacks
that involve changing the script by escaping from the string inside it
or something similar to that, which can be avoided additionally by using
either the method used in this commit, or a proper JSON encoder.
Test payload:
</Script><svg onload=alert(1)//>Closes #1525