777196 - MessageWakeupService + nsContentPrefService allow content processes to get/set prefs

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

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?

Comment 3

[Josh Matthews [:jdm]]

It's conceivable they could matter for desktop e10s.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7ef4465f20bd

All hail our new b2g-inbound overlords!

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

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

\o/

:)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Dave Hylands [:dhylands]]

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.

Comment 13

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 14

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the issue is surely unrelated, there's no way for this to cause anything like those failures.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18e46e06c80f

Comment 18

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18e46e06c80f