1198957 - Cross-site scripting vulnerability via extension GUIDs

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Discovery Pane, defect)

Description

[Wladimir Palant]

Open the following URL in your browser:

  https://services.addons.mozilla.org/en-US/firefox/discovery/pane/pane/40.0/strict#{%22%3Cimg%20src=%27foo%27%20onerror=%27alert%28"xss"%29%27%3E%22:%20{%22type%22:%20%22extension%22}}

This will currently result in an alert message saying "XSS". I think that the problematic code is https://github.com/mozilla/olympia/blob/7581411223af1e14b85771db50e70a11bbb7539a/static/js/zamboni/discovery_pane.js#L116 - passing unchecked strings to jQuery is a bad idea. AMO normally gets away with it because jQuery will consider anything starting with # a selector rather than HTML code (see http://bugs.jquery.com/ticket/9521).

Comment 1

[Wladimir Palant]

Bugzilla didn't linkify the end of my test URL, this one should work better:

  https://services.addons.mozilla.org/en-US/firefox/discovery/pane/pane/40.0/strict#{%22%3Cimg%20src=%27foo%27%20onerror=%27alert%28"xss"%29%27%3E%22:%20{%22type%22:%20%22extension%22%7d%7d

Comment 2

[Julien Vehent]

Will, could you pick this up? That's a legit XSS.

Comment 3

[Kris Maglione [:kmag]]

I think the real problem is that we're still using an ancient version of jQuery. In remotely recent versions, strings passed to the jQuery constructor are only treated as HTML if they actually start with `<` (ignoring whitespace).

Magopian, how do you feel about finally upgrading jQuery, so we don't have to worry about this happening elsewhere too?

I'm not sure how much damage a script can do running on the services.* sub-domain. We don't actually provide very much functionality there, and instead redirect to addons.mozilla.org for most links, but it seems likely that a script running there could steal the user's session cookie and do whatever they want with it on addons.mozilla.org. In that case, I'd call this sec-critical.

Comment 4

[Kris Maglione [:kmag]]

https://github.com/mozilla/olympia-security/pull/16

Comment 5

[Wladimir Palant]

(In reply to Kris Maglione [:kmag] from comment #3)
> I'm not sure how much damage a script can do running on the services.*
> sub-domain. We don't actually provide very much functionality there, and
> instead redirect to addons.mozilla.org for most links, but it seems likely
> that a script running there could steal the user's session cookie and do
> whatever they want with it on addons.mozilla.org.

Not just that - the same page is available via addons.mozilla.org as well:

> https://addons.mozilla.org/en-US/firefox/discovery/pane/pane/40.0/strict#{%22%3Cimg%20src=%27foo%27%20onerror=%27alert%28"xss"%29%27%3E%22:%20{%22type%22:%20%22extension%22%7d%7d

Comment 6

[Wladimir Palant]

(In reply to Kris Maglione [:kmag] from comment #3)
> I think the real problem is that we're still using an ancient version of
> jQuery. In remotely recent versions, strings passed to the jQuery
> constructor are only treated as HTML if they actually start with `<`
> (ignoring whitespace).

I wouldn't call the jQuery version the "real problem" - even with the recent jQuery versions there is always the possibility that something you meant as a selector will be interpreted as HTML code. I'd rather generally consider jQuery a security hazard as long as jQuery.parseHTML exists.

However, newer jQuery versions would indeed make issues like here or in bug 1200007 (assuming that I correctly identified the vulnerable code there) not exploitable.

Comment 7

[Mathieu Agopian [:magopian]]

As discussed on IRC, I'm not against updating jquery per se, but I'm worried that not having frontend tests, we don't have any way to know what will break, where, how, when.

Comment 8

[Mathieu Agopian [:magopian]]

Maybe we could use something like https://github.com/jojax/django-js-error-hook? (It boils down to posting "window.onerror" to a django view that then logs that).

Comment 9

[Mike Nolan [:nolski]]

Is there anything that we expect to fail if we upgrade from 1.6 -> 1.9? jQuery even provides a migration library that, when used fixes this issue. https://github.com/jquery/jquery-migrate

Comment 10

[Mathieu Agopian [:magopian]]

I don't know of anything that we expect to fail, no ;)

Comment 11

[Mike Nolan [:nolski]]

Then I'd say our best bet so solve this is to upgrade to jQuery 1.9 using the migration library for now.

Comment 12

[Wladimir Palant]

There aren't all too many features removed from jQuery (http://jquery.com/upgrade-guide/1.9/#changes-of-note-in-jquery-1-9), maybe just grepping for them would be an option as well? Chances are that AMO doesn't use any of this anywhere.

Comment 13

[Mike Nolan [:nolski]]

Well we'd have to update any API calls that have been deprecated between 1.6 and 1.9 which means that list isn't everything that'd we have to update. The migration library does have a feature where if you use the developer version, it will console.log warnings when you call a deprecated API in jQuery. Not entirely sure how we could collect all of those warnings ourselves though.

Comment 14

[Wladimir Palant]

I did the grepping and upgrading isn't all that simple apparently:

* .live() is used in quite a few places. jQuery-migrate could deal with that, fixing these should be easy however.
* .andSelf() is used by jQuery UI and jQuery miniColors libraries. This method will still work however, it's merely depreacted.
* "hover" pseudo-event is used in static/js/impala/suggestions.js - also supported by jQuery-migrate but trivial to fix.
* .selector property is used by static/js/impala/search.js, static/js/lib/jquery.pjax.js and static/js/submit/flow-pjax.js. jQuery-migrate won't help with this. search.js and flow-pjax.js are easily fixed, |container = $container.selector| can be replaced by |container = this| from what I can tell. Adapting the jQuery pjax library isn't quite as trivial, and it is no longer being developed (latest version still has that issue).
* .attrFn is used in static/js/lib/jqmobile.js and won't be caught by jQuery-migrate either but should be easy to fix.

Somebody else should probably do the grepping as well, just to verify that I didn't miss anything.

Comment 15

[Mike Nolan [:nolski]]

This looks to be right for 1.9 issues but I still think jquery-migrate should allow us to keep using these end points? Why wont .attrFn now be caught by the migration library?

Comment 16

[Wladimir Palant]

My understanding is that the migration library isn't meant for undocumented features - and .attrFn is internal and undocumented functionality. And .selector was probably just too hard to fake with modern jQuery versions.

Comment 17

[Mike Nolan [:nolski]]

Can you find a page where functionality is broken because of our use of this? I currently have a test branch using 1.9.1 and the migration library here: https://github.com/Nolski/olympia/tree/jquery-upgrade.

Comment 18

[Mike Nolan [:nolski]]

Created PR https://github.com/mozilla/olympia/pull/775

Comment 19

[Mike Nolan [:nolski]]

Fix is on staging.

Comment 20

[Wladimir Palant]

This issue still exists in production. Not sure why but $("foo<img>bar") will still create an HTML tag even though $.fn.jquery returns "1.9.1".

Comment 21

[Mathieu Agopian [:magopian]]

I believe it's because we're using jquery-migrate, which re-introduces holes that were fixed by the latest versions of jquery. We're discussing ways to get rid of jquery-migrate.

Comment 22

[Mathieu Agopian [:magopian]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1228959

Comment 23

[Mathieu Agopian [:magopian]]

Fixed by https://github.com/mozilla/olympia/pull/1260

Tested on -dev, xss isn't there anymore.

Comment 24

[Wladimir Palant]

This should be public I think.