1253013 - Persistent XSS using JavaScript URLs

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Daniel Tomescu]

Attachment: conversation settings.PNG

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36

Steps to reproduce:

Hi,

I discovered a persistent Corss-Site Scripting (XSS) vulnerability in Firefox Hello.

On every conversation, we can add a conversation title, a conversation URL and conversation comments. The URL can be anything, including javascript: or data: URIs.

Steps to reproduce the vulnerability:

1. Create a new conversation.
2. Edit the conversation details; set the URL value to "javascript:alert(document.domain)" and the conversation title & comments to something that would make a victim click on it.
3. Save the conversation details.
4. Share the conversation URL with the victim.
5. Wait until the victim clicks on the URL. 


A few mentions:
1. The script is stored on the server and is automatically embedded in the page corresponding to our conversation.
2. The script runs in the security context of hello.firefox.org.
3. I successfully tested the vulnerability in Google Chrome and Firefox (latest versions for Windows 10). It probably works in Safari too. It cannot be reproduced in IE or mobile browsers because Firefox Hello does not support those browsers.
4. Being a persistent XSS, the XSS filter from Google Chrome does not protect against the vulnerability.
5. For successfully exploiting the vulnerability, some user interaction is required: visiting a link provided by the attacker and clicking on a button. However, this is perfectly plausible user interaction - that's what people do on the internet, they visit links and they click on buttons.

Example of page that contains embedded JavaScript code:
https://hello.firefox.com/Esid9OqPjOQ#fbWSfplg34-hTQJdzHI_7Q

Also, please see attached files.

Quick fix: do not allow javascript and data URLs. I would go a step further and I would allow only http:// and https:// URLs.

If I can be of help with anything more on this subject, please let me know.

Thank you,
Daniel Tomescu

Comment 1

[Daniel Tomescu]

Attachment: XSS onclick conversation comments.png

Comment 2

[Daniel Tomescu]

Attachment: XSS onclick conversation title.png

Comment 3

[Daniel Tomescu]

A small correction:

2. The script runs in the security context of hello.firefox.com

(there is no hello.firefox.org, the right domain is hello.firefox.com)

Comment 4

[Daniel Veditz [:dveditz]]

I'm going to call this sec-moderate wrt firefox because there doesn't seem to be much of value on hello.firefox.com that the context can reach. Not filtering javascript and data URLs (or better, whitelisting) is a basic web development error and we should fix this ASAP.

Comment 5

[Mark Banner (:standard8)]

I'll fix this in the morning.

Comment 6

[Daniel Tomescu]

Hi,

I would argue that this is not a sec-moderate vulnerability since:
- it is a stored XSS;
- it executes in the context of a domain that the users trust, but they have a shaky understanding of it; 

An attacker could easily prompt the victim to "install additional software in order to make Firefox Hello work properly on his browser", leading to more serious trouble; or to alter the webpage and to insert a fake login to persona or mozilla accounts; or to ask for a $5 donation, getting access to CC or paypal data...

The way I see it, those kind of vulnerabilities are not limited by the domain in which the script is executed, but by the trust the users have in that domain. However, in the end, it's your call.

I like that you guys are really committed in fixing this asap and that, in general, you have really fast responses to security issues. Keep it going!

Cheers

Comment 7

[Mark Banner (:standard8)]

Attachment: [checked in] Display the context tile only for valid urls.

This fixes the issues - we simply won't display the context if the url is something other than http(s) or ftp.

dmose and I pair reviewed this over vidyo.

Comment 8

[Mark Banner (:standard8)]

Attachment: [checked in] Add tests for checking the context tile is only shown for valid urls.

Here's the test. I'm going to commit it separately after we've deployed the test, as its very obvious what we're protecting against here.

Again, r=dmose for this from pair reviewing.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8726347 [details] [diff] [review]
[checked in] Display the context tile only for valid urls.

https://github.com/mozilla/loop/commit/c391cb5c0ad857c0f1cbcad9eb91f1b52007b9e7

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8726350 [details] [diff] [review]
[checked in] Add tests for checking the context tile is only shown for valid urls.

The fix is now live everywhere including production.

I've just landed the unit tests:

https://github.com/mozilla/loop/commit/d534a80266e62d3bdeae261e3621fe5425e0c633

Comment 11

[Mark Banner (:standard8)]

Thank you for the report Daniel.

Comment 12

[Daniel Tomescu]

Hi Mark,

No problem. Should the fix be fully deployed into production now?

After a sort overview, I can tell that the vulnerability is only partially fixed. After joining the conversation, a javascript URL can still be added under the conversation title.

Attachement "XSS onclick conversation title.png"
https://bug1253013.bmoattachments.org/attachment.cgi?id=8725865

Comment 13

[Mark Banner (:standard8)]

Hrm, ok, I missed the title part of this. I hadn't actually realised that was clickable.

Comment 14

[Daniel Tomescu]

Hi,

Any update on this matter? Is a fix hard to implement for the second parameter? Although I may be wrong, I think having a similar solution for the conversation title as for the conversation comments is applicable.

Thanks

Comment 15

[Mark Banner (:standard8)]

Yes, we're dealing with other release activities at the moment, but we'll fix it as soon as we can.

Comment 16

[David Critchley (:dcritch)]

Attachment: Link to Github pull-request: https://github.com/mozilla/loop/pull/299

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

It strikes me that both the first and second patches are reasonable band-aids, but what we really want is to prevent more bugs like this from creeping in in the future.  I suggest a belt-and-suspenders approach which should actually only be a bit more work:

a) Factor out a tiny function, called, say, loop.shared.utils.formatSanitizedContextURL that does exactly what the common pattern in both of these patches is: calls format URL, and rejects any non-whitelisted protocols.

b) Factor out a tiny, tiny PureRenderingMixin-using class from inside ContextURLView called ContextURLLink which uses formatSanitizedContextURL to enforce that restriction, and every <a href="contextURL"> in the code base with that component.

c) Ensure that we use that function (or something similar, like, say, a getValidatedContextURL function) in each place that we read the context URL from the network or accept it as input from the user.

This should actually make it pretty hard to introduce bugs like this in the future.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

'and every <a href="contextURL"> in the code base' should read 'and replace every <a href="contextURL"> in the code base'

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8732428 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/299

Dave and I discussed, the plan is to try and get something that does at least a) and b) fairly quickly, and then reflag for review.

Comment 20

[David Critchley (:dcritch)]

Attachment: 1253013_simpleFix.patch - Interim patch

Comment 21

[David Critchley (:dcritch)]

Attachment: [checked in] 1253013_simpleFix.patch - Interim patch

Comment 22

[David Critchley (:dcritch)]

Attachment: 1253013_simpleFix_tests.patch - Tests for Interim patch

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8734173 [details] [diff] [review]
[checked in] 1253013_simpleFix.patch - Interim patch

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

r=dmose with the two suggested tweaks.

::: standalone/content/js/standaloneRoomViews.jsx
@@ +533,3 @@
>  
>      renderContext: function() {
>        var urlData = (this.props.room.roomContextUrls || [])[0] || {};

Please add an XXX comment to make this code more readable.

@@ +543,5 @@
>          urlData.description || urlData.location ||
>          mozL10n.get("room_name_untitled_page");
> +      // Only allow specific types of URLs.
> +
> +      // Only allow specific types of URLs.

you can probably drop line 545.  :-)

Comment 24

[David Critchley (:dcritch)]

Attachment: Full solution patch

Comment 25

[David Critchley (:dcritch)]

Attachment: Full solution tests patch

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8735600 [details] [diff] [review]
Full solution tests patch

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

Interim tests patch looks good; please update with the wording tweaks suggested, and then wait to land until our next production deployment.

::: standalone/test/standaloneRoomViews_test.js
@@ +517,5 @@
> +        expect(view.getDOMNode().querySelector(".standalone-info-bar-spacer .context-info a").getAttribute("href"))
> +          .eql(null);
> +      });
> +
> +      it("should not allow context link if context location is not whitelisted", function() {

Maybe "if the context location protocol is not whitelisted" would be a better description of what's being tested.

@@ +861,4 @@
>                      .not.eql(null);
>                  });
>  
> +        it("should enable clicking of context link when url checks against whitelist",

"URL protocol whitelist " or "URL scheme whitelist" would be clearer.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

By which I actually meant to say "Full solution tests patch looks good"

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8735599 [details] [diff] [review]
Full solution patch

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

Almost there!  feedback+, I'd like to have a quick look at it once you've addressed the comments.

Please add an XXX (or ask manu) about the use of .context-info around the TOSView display in StandaloneInfoView.render.  This seems likely to be a mistake, and if it's not, we want to probably rename .context-info.  Or something.

::: shared/js/views.jsx
@@ +537,5 @@
>    /**
> +   * Renders the a href link for context.
> +   *
> +   * @property {Boolean} allowClick         Set to true to allow the url to be clicked. If this
> +   *                                        is specified, then 'dispatcher' is also required.

Please remove this and the other reference to "dispatcher".

@@ +544,5 @@
> +   * @property {String}  thumbnail          The thumbnail url (expected to be a data url) to
> +   *                                        display. If not specified, a fallback url will be
> +   *                                        shown.
> +   * @property {String}  url                The url to be displayed. If not present or invalid,
> +   *                                        then this view won't be displayed.

Please correct the comment to match the behavior.

@@ +559,5 @@
> +    },
> +
> +    render: function() {
> +      var sanitizedURL = loop.shared.utils.formatSanitizedContextURL(this.props.url);
> +      var handleClick = this.props.handleClick || null;

We should have a test that verifies that not passing handleClick doesn't explode, and instead does nothing.

@@ +568,5 @@
> +      });
> +
> +      return (
> +        <a className={wrapperClasses}
> +           href={this.props.allowClick && sanitizedURL ? sanitizedURL.location : null}

I'd suggest using a temporary before the return statement to improve readability.

@@ +633,5 @@
> +          <ContextUrlLink allowClick={this.props.allowClick}
> +                          description={description}
> +                          handleClick={this.handleLinkClick}
> +                          url={url}>
> +            {thumbnail ? <img className="context-preview" src={thumbnail} /> : null}

I wouldn't mind if this become its own very simple pure component.

::: standalone/content/css/webapp.css
@@ +262,5 @@
> +  border: none;
> +  border-radius: 0;
> +  background: inherit;
> +  padding: 0;
> +  flex-flow: row nowrap;

Instead of cancelling out other CSS classes, how about splitting up the classes to make this more maintainable.

::: standalone/content/js/standaloneRoomViews.jsx
@@ +509,5 @@
>  
>      renderContext: function() {
> +      var urlData = {};
> +      if (this.props.room.roomContextUrls && this.props.room.roomContextUrls.length > 0) {
> +        urlData = this.props.room.roomContextUrls[0];

Maybe use a temp to avoid all the redundancy for easier reading?

Comment 29

[David Critchley (:dcritch)]

Was looking at the code with Dan and we encountered this: (around line 579 in standaloneRoomView.jsx)

<div className="context-info">
  <ToSView dispatcher={this.props.dispatcher} />
</div>

It appears that the use of context-info as a class name here was by mistake? Just want to confirm before renaming it.

Comment 30

[Daniel Tomescu]

Hi guys,

Sorry to disturb, it's clear that I don't understand programming as well as you do, but I think we're losing focus here. What is the end goal of this topic - to write code so beautiful that it could win Miss Universe 2017? To write code so beautiful that kids will study it as poetry and mothers will name their children after the code variables? Or to fix the vulnerability and eliminate a risk? I'm joking a little, but you might consider a quick & dirty fix, so that the vulnerability is fixed, and spend time afterwards to transform the solution into something that respects all your quality standards.

Just my two cents. Keep up the good work!

Comment 31

[Daniel Tomescu]

Another thing. Could this report get a "bug-bounty ?" flag?

Thanks!

Comment 32

[Mark Banner (:standard8)]

@Daniel: The quick & dirty flag has landed. We currently have ops & QA working on deploying it.

That's different teams, so in the meantime we're working in parallel on the better longer term fix.

Comment 33

[Manuel Casas Barrado [:mancas]]

(In reply to David Critchley (:dcritch) from comment #29)
> Was looking at the code with Dan and we encountered this: (around line 579
> in standaloneRoomView.jsx)
> 
> <div className="context-info">
>   <ToSView dispatcher={this.props.dispatcher} />
> </div>
> 
> It appears that the use of context-info as a class name here was by mistake?
> Just want to confirm before renaming it.

Dave, the |context-info| class is used to apply the same css rules, that are applied to the room-context, to the terms and condition (colors, font-size...). I agree that is a bit confusing so I've prepared a small patch[1] for you in case you want to remove that <div> tag.

[1] https://github.com/mancas/loop/commit/a3247a89c3478fb60b5a7712a382d06fd908e843

Comment 34

[David Critchley (:dcritch)]

Attachment: Full solution patch

Comment 35

[David Critchley (:dcritch)]

Attachment: Full solution tests patch

Comment 36

[Daniel Veditz [:dveditz]]

(In reply to Daniel Tomescu from comment #31)
> Could this report get a "bug-bounty ?" flag?

Flag added, but in the future please mail security at mozilla.org for bug bounty requests as mentioned in our guidelines. Bounty discussions are an off-topic distraction for the developers working on fixing the bug itself.

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8736801 [details] [diff] [review]
Full solution tests patch

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

r=dmose

Comment 38

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8736797 [details] [diff] [review]
Full solution patch

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

r=dmose, with the tweaks suggested here.

::: shared/css/common.css
@@ +557,5 @@
> +  border-radius: 4px;
> +  background: #fafafa;
> +  padding: 1.1rem .8rem;
> +  font-size: 13px;
> +  line-height: 14px;

Let's specify font-size and line-height in rems.

::: shared/js/views.jsx
@@ +541,5 @@
> +   * @property {String}  description        The description for the context url.
> +   * @property {String}  thumbnail          The thumbnail url (expected to be a data url) to
> +   *                                        display. If not specified, a fallback url will be
> +   *                                        shown.
> +   * @property {String}  url                The url to be displayed. If not present or invalid,

handleClick is not documented here.

@@ +561,5 @@
> +      var wrapperClasses = classNames({
> +        "context-wrapper": true,
> +        "clicks-allowed": this.props.allowClick
> +      });
> +      var opts = {};

I propose adding 1-line of vertical whitespace above the opts decl for readability.

@@ +577,5 @@
> +        <a {...opts}
> +           className={wrapperClasses}
> +           rel="noreferrer"
> +           target="_blank">
> +          {this.props.children}

Write a unit it test that null is handled properly and make sure it passes.

Comment 39

[David Critchley (:dcritch)]

Attachment: Full solution patch

Comment 40

[David Critchley (:dcritch)]

Attachment: Full solution tests patch

Comment 41

[David Critchley (:dcritch)]

Full solution patch complete, waiting until the temp fix has been deployed. Likely next week.

Comment 42

[Mark Banner (:standard8)]

The quick fix is now on production. Dave, please can you land the remaining patches.

Comment 43

[David Critchley (:dcritch)]

commit for fix:   https://github.com/mozilla/loop/commit/2ea05d1704287bd2e3a1fafd812bda4d94aa85ff
commit for tests: https://github.com/mozilla/loop/commit/c2f7cb093d8d217f7e868f1212a340105834cef5

Comment 44

[Daniel Tomescu]

Hi guys,

A javascript URL can still be added under the conversation title.

Attachement "XSS onclick conversation title.png"
https://bug1253013.bmoattachments.org/attachment.cgi?id=8725865

Am I retesting too soon? Will the fix take effect in the near future?

Thank you,
Daniel Tomescu

Comment 45

[Mark Banner (:standard8)]

(In reply to Daniel Tomescu from comment #44)
> A javascript URL can still be added under the conversation title.
...
> Am I retesting too soon? Will the fix take effect in the near future?

You might need to shift-reload a few times to ensure you've picked up the new code. The cases shown in the url of comment 0 are not clickable for me.

Comment 46

[Daniel Tomescu]

Hi Mark,

You are right, the title is not clickable anymore, probably it was cached in my browser. Sorry for that, my bad.

Thank you,
Daniel Tomescu

Comment 47

[Mark Banner (:standard8)]

(In reply to Daniel Tomescu from comment #46)
> You are right, the title is not clickable anymore, probably it was cached in
> my browser. Sorry for that, my bad.

No problem, thanks for the verification, its always good to be sure.