1025267 - (CVE-2016-2832) -moz-handler css pseudo-classes leak plugin state to content

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[John Schoenick [:johns]]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Mats Palmgren (inactive)]

Attachment: 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?

Comment 3

[Mats Palmgren (inactive)]

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).

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 5

[:Gijs (out for now; he/him)]

(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)

Comment 6

[Boris Zbarsky [: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.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

[:Gijs (out for now; he/him)]

(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... ) ?

Comment 10

[Boris Zbarsky [: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.

Comment 11

[Mats Palmgren (inactive)]

Attachment: 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

Comment 12

[Mats Palmgren (inactive)]

Thank you both for your help!

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

[:Gijs (out for now; he/him)]

(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. :-\

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 18

[Mats Palmgren (inactive)]

> 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.

Comment 19

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb55f73763cd

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb55f73763cd

Comment 21

[Mats Palmgren (inactive)]

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.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Need to uplift bug 1268749, Beta47+

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f84225671f1f4564545ea913bac4f435aab05d6c