Bug 1025267 (CVE-2016-2832)

-moz-handler css pseudo-classes leak plugin state to content

RESOLVED FIXED in Firefox 47

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: johns, Assigned: mats)

Tracking

({sec-moderate})

Trunk
mozilla48
sec-moderate
Points:
---

Firefox Tracking Flags

(firefox33 wontfix, firefox46 wontfix, firefox47 fixed, firefox48 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main47+])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
So we have a list of magic pseudo-classes[1] that nodes apply to themselves to cause chrome CSS to attach a binding[2], but these are not apparently restricted from content, allowing:
> myElement.mozMatchesSelector(":-moz-handler-disabled")

This leaks information about if the user has a installed-but-disabled plugin, allowing fingerprinting of all plugins present on the system, ignoring enabled state.

Other information that can be inferred, though less important:
- If we're using 'play preview', allowing detection of pdf.js handlers vs native plugins
- Reason a plugin didn't load (click-to-play vs blocklist vs blocklist click to play...)
- Infer a user's blocklist state if they have vulnerable plugins

There's also a few non-plugin magic selectors that might be leaking things content shouldn't see such as :-moz-window-inactive, but I haven't investigated how those are used.

[1] http://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPseudoClassList.h#154
[2] http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/content/pluginProblemBinding.css
status-firefox33: --- → affected
We have a way to restrict parsing of pseudo-elements to chrome only (in practice, system stylesheets only).  We could add something like that for pseudo-classes.
Keywords: sec-moderate
Group: core-security → core-security-release
Component: General → Layout
(Assignee)

Comment 2

a year ago
Created attachment 8735148 [details] [diff] [review]
wip

This seems to be working, except for one remaining issue.
In a Content Process we use "doc.nodePrincipal" here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js?rev=ad73190dcc49#168
which I assume is unprivileged, so the 'matches' here fails:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js?rev=1bd2197b4d48#777
with "SyntaxError: An invalid or illegal string was specified"
which causes these browser-chrome tests to fail:
browser/base/content/test/general/browser_contextmenu.js
browser/base/content/test/plugins/browser_CTP_context_menu.js
https://treeherder.mozilla.org/#/jobs?repo=try&revision=030a7752eb9e&selectedJob=18638708

Do you have any ideas on how to get around that?
Am I using the right check to assign 'mIsChrome' in CSSParserImpl::ParseSelectorString?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 3

a year ago
BTW, in CSSParserImpl::ParseSheet we use IsChromeURI():
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp?rev=a734b88639f2#1679
but the aURI in ParseSelectorString is only "for error reporting"
(it's the content document).
(Assignee)

Updated

a year ago
Component: Layout → CSS Parsing and Computation
> Do you have any ideas on how to get around that?

Executing nsContextMenu.js code in a content-principal sandbox (is that what we're doing?) seems ... quite weird.  In fact, it seems completely wrong.  Gijs, do you know what's going on there, and why, or who would know?
Flags: needinfo?(bzbarsky) → needinfo?(gijskruitbosch+bugs)

Comment 5

a year ago
(In reply to Boris Zbarsky [:bz] from comment #4)
> > Do you have any ideas on how to get around that?
> 
> Executing nsContextMenu.js code in a content-principal sandbox (is that what
> we're doing?) seems ... quite weird.  In fact, it seems completely wrong. 
> Gijs, do you know what's going on there, and why, or who would know?

Not off-hand. AFAICT we're forwarding the context menu information between processes, including the principal of the node/document - but that's just part of a whole bunch of info that needs to be forwarded to the other process. I don't think anything is being *run* in a content-principal'd sandbox, but perhaps I didn't look closely enough?

The error would be weird, I guess, if we're running in chrome-privileged code... well, unless... what principal does the string have that we pass to .matches() ? Won't that be the null principal, because it isn't associated with any kind of URI? That seems fairly easy to test for someone who knows their way around our layout / CSS code.

In fact, re-reading comment #3, it sounds like because the node on which we're calling element.matches is in a content document, the string passed to .matches gets associated with the URI of the document owning the node on which we're calling .matches, which will be some content-privileged thing (http://whatever or something). It sounds like .matches() should possibly be running with the principal of its caller (rather than the derived principal from the document in which the node resides - ie it should be possible for privileged content to call .matches() with chrome-only selectors on a content-privileged node and get the Right result), but I don't know exactly how we'd forward/determine that information in CSS land. Does that make sense?

(CC'ing Jared in case he has something to add - I know he looked at some of our content menu code recently)
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bzbarsky)
Ah, I see.  Yes, that makes sense.  matches() does indeed parse the selector with the principals of the node's owner document.

We do NOT want to start adding GetSubjectPrincipal() calls to matches(), imo: it's expensive and it would significantly complicate the parsed selector caching.  I would rather we just exposed whatever state we need here as a [ChromeOnly] accessor on MozObjectLoadingContent... which we do.  I believe replacing this:

  this.target.matches(":-moz-handler-clicktoplay")

with this:

  this.target.displayedType == HTMLObjectElement.TYPE_NULL &&
  this.target.pluginFallbackType == HTMLObjectElement.PLUGIN_CLICK_TO_PLAY

should work.  That's because nsObjectLoadingContent::ObjectState returns NS_EVENT_STATE_TYPE_CLICK_TO_PLAY only when mType == eType_Null and mFallbackType == eFallbackClickToPlay.  The displayedType attr returns mType and the pluginFallbackType attr returns mFallbackType.
Flags: needinfo?(bzbarsky)
Would that only fix it for JS though? We still use that selector in CSS, and if it is leaking to content a site could use that selector along with getComputedStyle() to infer its value.
The idea is to restrict CSS uses to chrome sheets.  But yes, if chrome styles things differently based on this selector, sites can detect the resulting styling....

Comment 9

a year ago
(In reply to Boris Zbarsky [:bz] from comment #8)
> The idea is to restrict CSS uses to chrome sheets.  But yes, if chrome
> styles things differently based on this selector, sites can detect the
> resulting styling....

Well, we style things here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/pluginproblem/content/pluginProblemBinding.css#7-29

so we style it the same for a whole host of these pseudo-classes, which would still not allow fingerprinting per se as identified in comment #0. Of course, fingerprinting whether we're doing any plugin blocking or not is always possible - if the plugin runs it can send info to JS or a server in some way...

So it seems worth making this pseudoselector chrome-only, to me - we can fix the context menu as suggested in comment #6, assuming that works for embed and applet elements as well (which we seem to all support in the current JS... ) ?
Flags: needinfo?(bzbarsky)
> assuming that works for embed and applet elements as well

It does, yes.  The displayedType and pluginFallbackType props are on the MozObjectLoadingContent, which is implemented by all three elements.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 11

a year ago
Created attachment 8736481 [details] [diff] [review]
fix+test

With your suggested changes in comment 6.  It looks like it works now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c07ddc6a6c49
https://treeherder.mozilla.org/#/jobs?repo=try&revision=620833c442c3
Assignee: nobody → mats
Attachment #8735148 - Attachment is obsolete: true
Attachment #8736481 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

a year ago
Thank you both for your help!
Comment on attachment 8736481 [details] [diff] [review]
fix+test

>+++ b/browser/base/content/nsContextMenu.js

The indent here is off, because tabs snuck in.  Please nix the tabs.

We may want a test that checks that these pseudos are in fact parseable in a chrome sheet; followup is ok for that test.

r=me
Attachment #8736481 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #13)
> We may want a test that checks that these pseudos are in fact parseable in a
> chrome sheet; followup is ok for that test.

This may already be covered by http://hg.mozilla.org/mozilla-central/file/60b269fed8cf/browser/base/content/test/general/browser_parsable_css.js since the styles exist in a chrome stylesheet and that test confirms that none of our front-end CSS contains parsing errors.

But maybe you are requesting an isolated testcase?

Matter of fact, when this lands it might break that test since I see a similar comment at http://hg.mozilla.org/mozilla-central/file/60b269fed8cf/browser/base/content/test/general/browser_parsable_css.js#l25 because a stylesheet is already using a chrome-only pseudo-class. So you may need to update this test to add an exclusion AND write a test to confirm that it does parse correctly in a chrome sheet.
Um... that test should really parse things as chrome, which is how they would actually get parsed.  :(  Ah, well.

If it did that, it would be fine for testing the stuff from this bug.

Comment 16

a year ago
(In reply to Boris Zbarsky [:bz] from comment #15)
> Um... that test should really parse things as chrome, which is how they
> would actually get parsed.  :(  Ah, well.

I thought Xidorn fixed that? It should be using chrome URIs, IIRC :-\

> If it did that, it would be fine for testing the stuff from this bug.

Yes, though I've had my doubts about its reliability, but haven't had time to investigate in more detail. There are known cases of it not catching things (cf. bug 1255975) and/or bugs on file about some of that (bug 1221383) as well.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Matter of fact, when this lands it might break that test since I see a
> similar comment at
> http://hg.mozilla.org/mozilla-central/file/60b269fed8cf/browser/base/content/
> test/general/browser_parsable_css.js#l25 because a stylesheet is already
> using a chrome-only pseudo-class. So you may need to update this test to add
> an exclusion AND write a test to confirm that it does parse correctly in a
> chrome sheet.

This revision of the file is really old. How did you get there?

The chrome URI changes were in http://hg.mozilla.org/mozilla-central/rev/8db7294a94a3 (bug 1207443) which post-dates the highlighter.css thing. I don't know off-hand if that was simply missed in review (ie should have been removed), or if "UA-only" means something else in this context, or if there's just more work to be done on that front. :-\
(In reply to :Gijs Kruitbosch from comment #16)
> This revision of the file is really old. How did you get there?

Sorry, my awesome bar jumped me there.
(Assignee)

Comment 18

a year ago
> The indent here is off, because tabs snuck in.  Please nix the tabs.

The file was missing an Emacs mode-line which is how that happened I guess.
I've added a mode-line.

(In reply to Boris Zbarsky [:bz] from comment #13)
> We may want a test that checks that these pseudos are in fact parseable in a
> chrome sheet; followup is ok for that test.

Filed bug 1261239.

(In reply to Boris Zbarsky [:bz] from comment #15)
> Um... that test should really parse things as chrome, which is how they
> would actually get parsed.  :(  Ah, well.

Filed bug 1261237.
(Assignee)

Comment 19

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb55f73763cd
https://hg.mozilla.org/mozilla-central/rev/cb55f73763cd
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 21

a year ago
Comment on attachment 8736481 [details] [diff] [review]
fix+test

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: minor info leakage about installed plugins
[Describe test coverage new/current, TreeHerder]: have baked on trunk and Aurora since 2016-04-01, have regression tests
[Risks and why]: low
[String/UUID change made/needed]: none

Uplifting to beta isn't strictly needed but Xidorn says it makes uplifting
bug 1268749 less risky.
Attachment #8736481 - Flags: approval-mozilla-beta?
Comment on attachment 8736481 [details] [diff] [review]
fix+test

Need to uplift bug 1268749, Beta47+
Attachment #8736481 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

a year ago
status-firefox47: --- → affected
https://hg.mozilla.org/releases/mozilla-beta/rev/f84225671f1f4564545ea913bac4f435aab05d6c
status-firefox47: affected → fixed
Blocks: 1268749
Whiteboard: [post-critsmash-triage]
status-firefox33: affected → wontfix
status-firefox46: --- → wontfix
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main47+]
Alias: CVE-2016-2832
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.