1384558 - DOM Cross-Site Scripting ( XSS ) - reviewboard.mozilla.org

Status: RESOLVED FIXED

(MozReview Graveyard :: Review Board: Upstream, defect)

Description

[marius]

Attachment: Screen Shot 2017-07-26 at 16.03.49.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Hi 

I would like to report some Cross-Site Scripting (XSS) issues on reviewboard.mozilla.org application.


POC: ( tested from MacBook and Windows 7 using Chrome and Opera browsers ) 


https://reviewboard.mozilla.org/r/160856/diff/#<img/src/onerror=alert('XSS')> 

https://reviewboard.mozilla.org/r/161328/diff/#<img/src/onerror=alert('XSS')> 

https://reviewboard.mozilla.org/r/160856/diff/1-2/?expand=1#<img/src/onerror=alert('XSS')> 

 
See: 

var hash=window.location.hash,commentName,targetBox;
commentName=hash.toString().substring(1);
targetBox=$("a[name="+commentName+"]").closest(".box");  

Regards

Comment 1

[Frederik Braun [:freddy]]

Marius, please note that we pointed you to the FAQ of our web bounty program in a previous bug, yesterday.
The FAQ is at <https://www.mozilla.org/en-US/security/web-bug-bounty/>. It seems you are still working on websites that are not on our list of bounty eligible websites <https://www.mozilla.org/en-US/security/web-bug-bounty/#exclusions>.

We still appreciate you helping us, of course.


NB: This bug doesn't affect Firefox, as location.hash is always encoded, unlike in Chrome or other browsers.

Comment 2

[Simon Bennetts [:psiinon]]

I've confirmed that the POCs work in Chrome 59 on Linux and Windows 10, but as :freddyb has indicated they dont work in Firefox 54 on Linux or Windows 10.

Comment 3

[:glob ✱]

Attachment: 1384558_1.patch

Comment 4

[:glob ✱]

Attachment: 1384558_2.patch

Comment 5

[:glob ✱]

i've reported this issue to upstream review board.

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8890434 [details] [diff] [review]
1384558_2.patch

Review of attachment 8890434 [details] [diff] [review]:
-----------------------------------------------------------------

::: reviewboard/reviewboard/static/rb/js/pages/views/diffViewerPageView_mozreview.js
@@ +381,5 @@
> +            /* The anchor name needs to be escaped to protect against XSS.
> +             * The catch is some browsers already escape hashes, so this
> +             * needs to be conditional.
> +             */
> +            var anchorName = this._startAtAnchorName.match(/[<>]/)

Are you sure this is adequate to identify problematic strings? It doesn't fully identify encoded vs not and I'm not confident it would fully prevent all XSS. From what I've seen I think the only intended hashes used throughout review board are alphanumeric, or alphanumeric with commas. Maybe we should just whitelist?

So we'd always decode, than make sure it only has ^[a-zA-Z0-9,]*$

@@ +394,5 @@
>               * comment locations), but not all are. If the anchor isn't found,
>               * but the URL hash is indicating that we want to start at a
>               * location within this file, add the anchor.
>               * */
>              urlSplit = this._startAtAnchorName.split(',');

If some browsers are urlencoding the hash, won't this be broken in those (as ',' would be encoded)?

::: reviewboard/reviewboard/static/rb/js/views/issueSummaryTableView.js
@@ +329,5 @@
> +            /* The comment name needs to be escaped to protect against XSS.
> +             * The catch is some browsers already escape hashes, so this
> +             * needs to be conditional.
> +             */
> +            if (commentName.match(/[<>]/)) {

These comment names can only be r"comment[1-9]+[0-9]*$", I think I'd feel better if we bailed out if it didn't match this exactly.

Comment 7

[:glob ✱]

Attachment: 1384558_3.patch

i'm concerned taking a whitelist approach may exclude legit values, and i'm keen to not have to touch this code again if that happens.

given the crux of the issue is how jquery is handling the selector we should just be able to lean on document.querySelector to perform the lookup without a security issue, and without impacting any other logic.

i've discussed and shown this approach to smacleod over irc and he's given it an ok.

Comment 8

[:glob ✱]

as per approval over irc i've deployed 'patch 3' to production.
https://hg.mozilla.org/webtools/reviewboard/rev/f8309a35ae8e6e99844c03a11df91d5bdb66a7c3
upstream review board are planning to release a security release for 2.0.x and 2.5.x today.

marius - thanks for the report!

Comment 9

[April King [:April]]

Reporter - how would you like to be credited in our bug bounty program? If there's a page you'd like us to link to, we can do that too. Thanks!

Comment 10

[marius]

Avram Marius Gabriel - https://twitter.com/securityshell 

Thank you