MessageWakeupService + nsContentPrefService allow content processes to get/set prefs

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cjones, Assigned: gsvelto)

Tracking

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:-)

Details

Attachments

(1 attachment, 1 obsolete attachment)

There's no reason to ever allow content processes to set prefs, and they can cause bad things to happen by doing so.  Even non-maliciously, due to race conditions.  We've never supported content processes setting prefs; I'm not sure how nsContentPrefService got through review.  We should disable that.

There's some sensitive-ish data in prefs, but I'm not sure that locking that down is all that important.
Poking a little more, the prefs allowed to be get/set are

      const NAME_WHITELIST = ["browser.upload.lastDir", "spellcheck.lang"];

Whew!

It looks like "browser.upload.lastDir" isn't going to work with lower-rights processes so we can probably kill that.

I don't see why we would allow content to set spellcheck.lang so that should probably go too.
blocking-basecamp: --- → -
(Assignee)

Updated

6 years ago
Blocks: 838097
(Assignee)

Comment 2

5 years ago
I have done some further investigation on this topic due to bug 850219 and as far as I can tell we should be able to safely kill the whitelist without causing any regressions. The only instance we have that might be setting content preferences from a content process is B2G and I'm pretty sure we're not using either of the white-listed preferences:

- browser.upload.lastDir is used by the file-picker on Firefox desktop; B2G has its own file picker which uses web-activities so no issues here

- spellcheck.lang is used by nsIEditorSpellCheck which is also not used in B2G as we have our own editing functionality which is entirely implemented as content JS code

So my opinion on this is that the white list should go and nsContentPrefService2 should not duplicate this piece of functionality. Marco, since nsContentPrefService is being deprecated shall we bother removing the white list (and related functionality) as per this bug or shall we just wait for it to go away for good? Should also bug 838097 also be closed as we won't need this functionality in nsContentPrefService2?
Flags: needinfo?(mak77)

Comment 3

5 years ago
It's conceivable they could matter for desktop e10s.
(In reply to Josh Matthews [:jdm] from comment #3)
> It's conceivable they could matter for desktop e10s.

I don't think in this case we should leave unused and unmaintainded code cause someone in future may use it. There are various reasons, mostly that we are already unsure about the whitelist doing a good job about security and not being abused as a store.
Moreover this code lives in the old service, there is completely new code for the new service and regardless would be better to integrate properly the functionality in the new code than to keep part of the old code alive. It was just a hack, it needs better design directions.

(In reply to Gabriele Svelto [:gsvelto] from comment #2)
> Marco, since nsContentPrefService is being deprecated shall we bother
> removing the white list (and related functionality) as per this bug or shall
> we just wait for it to go away for good? Should also bug 838097 also be
> closed as we won't need this functionality in nsContentPrefService2?

I think we should remove that code, so it's not wrongly used.
bug 838097 can likely be closed then.
Flags: needinfo?(mak77)
(Assignee)

Comment 5

5 years ago
Created attachment 780853 [details] [diff] [review]
[PATCH WIP] Prevent non-chrome processes from accessing the content preferences

This patch removes the whitelist and all associated parent <-> child IPC functionality. The patch is not yet complete as I have to also fix the related unit-tests.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #780853 - Flags: feedback?(mak77)
Comment on attachment 780853 [details] [diff] [review]
[PATCH WIP] Prevent non-chrome processes from accessing the content preferences

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

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +14,5 @@
>  /**
>   * Remotes the service. All the remoting/electrolysis code is in here,
>   * so the regular service code below remains uncluttered and maintainable.
>   */
>  function electrolify(service) {

Do we still need this special code if it's not being used anywhere?
Maybe we should just throw in the constructor if it's the child process.
Attachment #780853 - Flags: feedback?(mak77) → feedback+
(Assignee)

Comment 7

5 years ago
Created attachment 783010 [details] [diff] [review]
[PATCH] Prevent non-chrome processes from accessing the content preferences

This is essentially the same patch as before minus the IPC unit tests which are not relevant anymore. Since I'm touching a moz.build file this probably requires a build-system peer review too so I'll look for one.

I've given this patch a quick spin on the try servers and it's looking good:

https://tbpl.mozilla.org/?tree=Try&rev=27a72e0584cc
Attachment #780853 - Attachment is obsolete: true
Attachment #783010 - Flags: review?(mak77)

Updated

5 years ago
Attachment #783010 - Flags: review?(mak77) → review+
(Assignee)

Comment 8

5 years ago
Try runs look as green as the Dutch countryside in the middle of spring (minus intermittent errors), time for checking in:

https://tbpl.mozilla.org/?tree=Try&rev=2515f546ee26
https://tbpl.mozilla.org/?tree=Try&rev=7b162536eb44
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/7ef4465f20bd

All hail our new b2g-inbound overlords!
Keywords: checkin-needed
(Assignee)

Comment 10

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #9)
> All hail our new b2g-inbound overlords!

\o/

:)
Backed out for asserts on OSX :(
https://hg.mozilla.org/integration/b2g-inbound/rev/ff4aca0cf3ce

These are the same maddening random asserts that we were hitting in bug 898713 (via bug 873378) that led to bug 887868 being backed out. I wish I knew what was going on with it, but it seems like there's some underlying graphics issue at play.
https://tbpl.mozilla.org/php/getParsedLog.php?id=26032281&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=26032309&tree=B2g-Inbound
Since this bug is talking about prefs, I thought I'd mention that bug 870951 is a bug which causes asserts due to a child process trying to set a pref.
(Assignee)

Comment 13

5 years ago
(In reply to Dave Hylands [:dhylands] from comment #12)
> Since this bug is talking about prefs, I thought I'd mention that bug 870951
> is a bug which causes asserts due to a child process trying to set a pref.

Different type of prefs but same problem :) These are content prefs and they use a different code path compared to regular preferences but in both cases child processes are not allowed to set them. Up until now we whitelisted a couple of these and let the child process set them but here I'm removing that functionality. BTW in the past I've hit a problem similar to what you're seeing in that bug so I'm going to comment there too.
(Assignee)

Comment 14

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> Backed out for asserts on OSX :(
> https://hg.mozilla.org/integration/b2g-inbound/rev/ff4aca0cf3ce
> 
> These are the same maddening random asserts that we were hitting in bug
> 898713 (via bug 873378) that led to bug 887868 being backed out. I wish I
> knew what was going on with it, but it seems like there's some underlying
> graphics issue at play.
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26032281&tree=B2g-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26032309&tree=B2g-Inbound

So do you think the issue is unrelated to my patch? I'd like to help and fix those oranges but I haven't got a recent Mac to test on (only an unsupported PowerPC-based oldie which even if supported would probably take a couple of days to do a build).
the issue is surely unrelated, there's no way for this to cause anything like those failures.
This doesn't appear to be reproducing the assertions in comment 11 anymore, so I will land this again next time I'm pushing to inbound.
https://tbpl.mozilla.org/?tree=Try&rev=fa554c580d6a
(Assignee)

Comment 18

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> This doesn't appear to be reproducing the assertions in comment 11 anymore,
> so I will land this again next time I'm pushing to inbound.

Thanks Ryan!
https://hg.mozilla.org/mozilla-central/rev/18e46e06c80f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.