Closed Bug 1198957 Opened 5 years ago Closed 4 years ago

Cross-site scripting vulnerability via extension GUIDs

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2015-09-03

People

(Reporter: ecfbugzilla, Assigned: nolski)

Details

(Keywords: sec-high, wsec-xss)

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).
Will, could you pick this up? That's a legit XSS.
Flags: needinfo?(wclouser)
Assignee: nobody → mathieu
Flags: needinfo?(wclouser)
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.
Flags: needinfo?(mathieu)
https://github.com/mozilla/olympia-security/pull/16
Assignee: mathieu → kmaglione+bmo
Status: NEW → ASSIGNED
Target Milestone: --- → 2015-09-03
(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&#x22;xss&#x22;%29%27%3E%22:%20{%22type%22:%20%22extension%22%7d%7d
(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.
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.
Flags: needinfo?(mathieu)
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).
Assignee: kmaglione+bmo → me
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
I don't know of anything that we expect to fail, no ;)
Then I'd say our best bet so solve this is to upgrade to jQuery 1.9 using the migration library for now.
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.
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.
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.
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?
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.
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.
Flags: needinfo?(trev.moz)
Fix is on staging.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Keywords: sec-high, wsec-xss
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".
Status: RESOLVED → REOPENED
Flags: needinfo?(trev.moz)
Resolution: FIXED → ---
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.
See Also: 1228959
Fixed by https://github.com/mozilla/olympia/pull/1260

Tested on -dev, xss isn't there anymore.
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
This should be public I think.
Flags: needinfo?(amuntner)
Group: client-services-security
Flags: needinfo?(amuntner)
You need to log in before you can comment on or make changes to this bug.