Closed Bug 1332501 Opened 3 years ago Closed 3 years ago

ContentPrefServiceParent calls arbitrary (content-process-controlled) methods on nsIContentPrefService2

Categories

(Toolkit :: Preferences, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 + fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: jld, Assigned: Gijs)

Details

(Keywords: sec-low, Whiteboard: [post-critsmash-triage][adv-main52+])

Attachments

(1 file, 1 obsolete file)

The problem is here: http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/components/contentprefs/ContentPrefServiceParent.jsm#129

Exposing the entire content pref API to a (possibly compromised) content process isn't necessarily a problem, depending on exactly what the few features that use content prefs do with the data they get.

The bigger problem is that data.call can be *any* property, like "__defineGetter__" or "constructor", and there might be ways to achieve remote code execution.
(In reply to Jed Davis [:jld] {⏰UTC-7} from comment #0)
> The problem is here:
> http://searchfox.org/mozilla-central/rev/
> 30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/components/contentprefs/
> ContentPrefServiceParent.jsm#129
> 
> Exposing the entire content pref API to a (possibly compromised) content
> process isn't necessarily a problem, depending on exactly what the few
> features that use content prefs do with the data they get.

What about bug 984012?
These aren't about:config prefs. They're per-domain prefs like the zoom level.
Something like this?
Attachment #8828758 - Flags: review?(jld)
Attachment #8828758 - Flags: review?(bobbyholley)
[Tracking Requested - why for this release]:
52 is the pwn2own target this year and will also become our next ESR (Tor).
Comment on attachment 8828758 [details] [diff] [review]
don't trust the content process to call any old thing on the content pref service,

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

This still lets the content process call QueryInterface, along with the NYI functions in [1] right?

Given that we already have this explicit enumeration of the methods we expect, it seems like we should put that in a shared place in some more-structured format, and use it on both sides of the IPC.

[1] http://searchfox.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefServiceChild.jsm
Attachment #8828758 - Flags: review?(bobbyholley) → review-
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> Comment on attachment 8828758 [details] [diff] [review]
> don't trust the content process to call any old thing on the content pref
> service,
> 
> Review of attachment 8828758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This still lets the content process call QueryInterface, along with the NYI
> functions in [1] right?
> 
> Given that we already have this explicit enumeration of the methods we
> expect, it seems like we should put that in a shared place in some
> more-structured format, and use it on both sides of the IPC.
> 
> [1]
> http://searchfox.org/mozilla-central/source/toolkit/components/contentprefs/
> ContentPrefServiceChild.jsm

Would a simple white list of method names be best? Also, don't see any reason to do those checks on the child side.
(In reply to Jim Mathies [:jimm] from comment #6)
> Would a simple white list of method names be best? Also, don't see any
> reason to do those checks on the child side.

My point is that the child already has a list of what methods are expected to be forwarded. It seems like we should just use the same list.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> (In reply to Jim Mathies [:jimm] from comment #6)
> > Would a simple white list of method names be best? Also, don't see any
> > reason to do those checks on the child side.
> 
> My point is that the child already has a list of what methods are expected
> to be forwarded. It seems like we should just use the same list.

By "list" do you mean that because the child jsm only forwards within some methods, it 'knows'? I don't see a list structure that could be shared as-is, if that's what you mean - maybe I'm not looking in the right place. We can create one, of course...

(In reply to Jim Mathies [:jimm] from comment #4)
> [Tracking Requested - why for this release]:
> 52 is the pwn2own target this year and will also become our next ESR (Tor).

Are we actually going to ship anything with 52 to sandbox the child meaningfully to stop them doing "bad" things from the child? I didn't think any of the stronger sandboxing guarantees were going to be shipped with it. If not, not sure this really needs to track 52, though if the patch is simple we should uplift it.
Flags: needinfo?(bobbyholley)
(In reply to :Gijs from comment #8)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> > (In reply to Jim Mathies [:jimm] from comment #6)
> > > Would a simple white list of method names be best? Also, don't see any
> > > reason to do those checks on the child side.
> > 
> > My point is that the child already has a list of what methods are expected
> > to be forwarded. It seems like we should just use the same list.
> 
> By "list" do you mean that because the child jsm only forwards within some
> methods, it 'knows'? I don't see a list structure that could be shared
> as-is, if that's what you mean - maybe I'm not looking in the right place.
> We can create one, of course...

Yeah, I was imagining refactoring all the boilerplate in the child to some more-compact table representation that we could share with the child. If that's hard or whatever, just duplicating those in a separate whitelist could work I suppose, it just seems yucky. I don't own this code though, so I'm probably not the right person to be nit-picking about this.
Flags: needinfo?(bobbyholley)
I agree with Bobby that we should have an explicit whitelist. If that means we have to duplicate the list of methods, so be it. There aren't that many of them.
OK, fair enough. This uses a single list, though I'm not sure it's better than duplicating. Happy to do that if people think it's clearer.
Attachment #8829244 - Flags: review?(jld)
Attachment #8829244 - Flags: review?(bobbyholley)
Attachment #8828758 - Attachment is obsolete: true
Attachment #8828758 - Flags: review?(jld)
Comment on attachment 8829244 [details] [diff] [review]
fix list of content pref service methods callable by child,

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

This looks nice, thanks.
Attachment #8829244 - Flags: review?(bobbyholley) → review+
Attachment #8829244 - Flags: review?(jld) → review+
(In reply to :Gijs from comment #8)
> Are we actually going to ship anything with 52 to sandbox the child
> meaningfully to stop them doing "bad" things from the child? I didn't think
> any of the stronger sandboxing guarantees were going to be shipped with it.
> If not, not sure this really needs to track 52, though if the patch is
> simple we should uplift it.

On Windows we will, and already do as of release 50: “level 1” as described by the current version of https://wiki.mozilla.org/Security/Sandbox.  For Pwn2Own that means the attacker will have to escalate from low IL to medium IL to meet the contest requirements, but I don't know enough about Windows internals to have an idea of how hard that is given the other restrictions we do/don't impose.
Trypush:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=856aa68611b4ff754b6f49b3cb41ca56a7c4b284
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to Jed Davis [:jld] {⏰UTC-7} from comment #13)
> (In reply to :Gijs from comment #8)
> > Are we actually going to ship anything with 52 to sandbox the child
> > meaningfully to stop them doing "bad" things from the child? I didn't think
> > any of the stronger sandboxing guarantees were going to be shipped with it.
> > If not, not sure this really needs to track 52, though if the patch is
> > simple we should uplift it.
> 
> On Windows we will, and already do as of release 50: “level 1” as described
> by the current version of https://wiki.mozilla.org/Security/Sandbox.  For
> Pwn2Own that means the attacker will have to escalate from low IL to medium
> IL to meet the contest requirements, but I don't know enough about Windows
> internals to have an idea of how hard that is given the other restrictions
> we do/don't impose.

52 will have the current level 1 sandbox which includes making the content process low integrity. It also includes pretty robust file system write restrictions.

One of the requirements for a p2o win is native code execution running at medium integrity, which means they'll be looking for ways to get code executing over in the browser for the prize. Hard to predict how they'll go about this, but a js exploit that they can target from the content process over in the browser is a likely path.

Please correct me if I'm wrong but this seems like a nice avenue for getting arbitrary js running over in the browser from content? In which case it would play into this type of attack.
(In reply to Jim Mathies [:jimm] from comment #15)
> Please correct me if I'm wrong but this seems like a nice avenue for getting
> arbitrary js running over in the browser from content? In which case it
> would play into this type of attack.

Jed and I talked about this very briefly on IRC. I don't believe it would be possible to run arbitrary JS, because everything you pass needs to go over the structured clone system, so I don't think you'd be able to pass functions as arguments. So you can override existant functions with strings or objects or whatever, but nothing callable (except maybe plain regexes if we still treat those as functions and they are serialized, which I dunno). Anyway, better safe than sorry. I remember some of the crazy stuff in the JS exploits before compartments and I don't know that I'm creative enough to tell you conclusively whether it's possible here or not (though I certainly expect the structured clone thing helps out here).
If it's any help with risk analysis: my gut tells me that, if I spent a day on it, I'd have about a 15% chance of finding some clever way of running arbitrary JS via this mechanism.
Per IRC with Jed yesterday, calling this sec-audit (as you'd first need to escalate privileges in the child), and landing on inbound:

remote:   https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=34b8529501d038d82583bf53ce78fa9cd2ac307c
Keywords: sec-audit
To clarify, I suggested sec-audit because of my level of doubt that the code is exploitable in its current form; but, given comment #17, this may have been unwise.

I'm going to mark this sec-low (inadequate input validation but not allowing code injection), but at worst it would be sec-moderate (because it's not exploitable by Web content without another sec bug), and neither of those need sec-approval to land under current policy.
Keywords: sec-low
(In reply to Jed Davis [:jld] {⏰UTC-7} from comment #19)
> To clarify, I suggested sec-audit because of my level of doubt that the code
> is exploitable in its current form; but, given comment #17, this may have
> been unwise.

Yeah, sorry that my comment was unclear. :-(

> I'm going to mark this sec-low (inadequate input validation but not allowing
> code injection), but at worst it would be sec-moderate (because it's not
> exploitable by Web content without another sec bug), and neither of those
> need sec-approval to land under current policy.

I agree sec-low/sec-moderate makes sense. Removing sec-audit.
Keywords: sec-audit
https://hg.mozilla.org/mozilla-central/rev/34b8529501d0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Gijs, should we be uplifting this fix up to beta?
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8829244 [details] [diff] [review]
fix list of content pref service methods callable by child,

Approval Request Comment
[Feature/Bug causing the regression]: n/a - security defense in depth fix
[User impact if declined]: This kind of thing will make us sad when pwn2own 2017 (and/or actual in-the-wild exploits) abuse it
[Is this code covered by automated tests?]: yeeees... but I don't have an awful lot of confidence in said tests. Would be worth some QE poking.
[Has the fix been verified in Nightly?]: not explicitly - it's a code change that should be imperceptible. Verification amounts to "does it still work?"
[Needs manual test from QE? If yes, steps to reproduce]:  yes:

1. verify that page zoom still works correctly
a. open page on mozilla.org, zoom to 200%
b. in the same tab, open google.com, check zoom resets
c. open page on mozilla.org again (don't go back, use a bookmark / link / whatever to navigate forward), check zoom goes back to 200%

2. verify that per-site download/upload directories still work correctly:
a. open e.g. an attachment to bugzilla from a particular directory
b. upload an attachment to some other website from a different directory
c. click the browse button on bugzilla again and check that it starts at the same directory from which you uploaded the previous bugzilla attachment (ie the one from (a) rather than the one from (b))

3. verify that per-site dictionary language setting still works correctly:
a. install a second dictionary for a language of your choice (use the context menu on a text field like the comment field on bugzilla), let's call it B.
b. set spellcheck on bugzilla to language B. Check that it works.
c. navigate to some other website like Facebook
d. it should use the previous default (A).
e. in that tab, go to bugzilla again, check that it goes back to spellchecking with language B.


[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: not very
[Why is the change risky/not risky?]: it's a fairly straightforward JS-only patch and it's early in beta/aurora
[String changes made/needed]: no.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8829244 - Flags: approval-mozilla-beta?
Attachment #8829244 - Flags: approval-mozilla-aurora?
Comment on attachment 8829244 [details] [diff] [review]
fix list of content pref service methods callable by child,

restrict content pref service methods callable from child, aurora53+, beta52+

Let's get this in 52.0b2.
Attachment #8829244 - Flags: approval-mozilla-beta?
Attachment #8829244 - Flags: approval-mozilla-beta+
Attachment #8829244 - Flags: approval-mozilla-aurora?
Attachment #8829244 - Flags: approval-mozilla-aurora+
Group: toolkit-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.