Closed Bug 1252855 Opened 4 years ago Closed 4 years ago

Sandboxing and setting prefs from chrome-privileged content JS: need to settle on a policy / implementation

Categories

(Core :: Security: Process Sandboxing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

Details

Attachments

(2 files)

In content processes, we can't set prefs because of sandboxing.

Unfortunately, sometimes we need to; we run code in the content process as well, and sometimes that code involves prefs.

In bug 1183296 I tried adding generic get/set messages for prefs so we can do this in a generic fashion. I was discouraged by :billm, and later in bug 1109714 George seems to have been similarly discouraged by :mrbkap to not go that route, either. 

However, in bug 1132925 we added such a messaging API to reader mode, and a similar one seems to have sprung up for pdfjs as well (going purely by searching our codebase for ":setintpref" -- maybe there are other instances?). Now in bug 1166365 I've (twice!) r-'d Eitan for trying to do this, and it turns out, in the latest case, just for trying to use this reader mode thing. Sorry, Eitan. :-\


I'd like us to decide what we do about this (so that the same "standards" are in use everywhere and people don't get r-'d for stuff that was OK elsewhere) and if there is a way to make this less painful.

Ultimately it seems like if we disallow the generic case, we're going to implement a gazillion very focused/narrow pref setting messages for the usecases we care about, but it's going to be a hassle every time.

Can we instead implement a generic mechanism with a unified whitelist that people have to add the prefs they're manipulating to? Would that be more acceptable from a security perspective and not lead to people having to reinvent the wheel every time they have code in the content process that wants a pref set?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(mrbkap)
Yes, let's do what you suggest. As long as there's a whitelist, this is safe. Can you do the work here Gijs?
Flags: needinfo?(wmccloskey)
I'm a chump, so I'll sign up, sure. :-)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(mrbkap)
Should we also restrict the prefs on the whitelist to be of simple types (char/bool/int)? This sounds good to me!
I was wondering if the plan is to bake this into the Preferences singleton so that accesses to preferences is still done in the same way. We'd probably want to avoid a preference diverging depending on how it is obtained.
One other thought: it should be clear when you set a pref this way that it can race with other content processes. This probably isn't a big deal for most of the prefs people are setting. But we do have to be careful about patterns like:

let old = getPref("xyz");
setPref("xyz", old + ",foobar");
On the sandboxing side, once we're shipping it for real, we'll change the code that sets the policy in the parent, so that there is a hard coded minimum unless an environment variable is set.
This doesn't increase risk, because you can already turn off the sandbox completely with an environment variable.

None of this means that content settable prefs couldn't potentially be used in some sort of sandbox escape, but at least you couldn't just set a pref to turn the sandbox off completely.
(In reply to Haik Aftandilian [:haik] from comment #4)
> I was wondering if the plan is to bake this into the Preferences singleton
> so that accesses to preferences is still done in the same way. We'd probably
> want to avoid a preference diverging depending on how it is obtained.

Which "Preferences singleton" are you talking about? Our prefs API is a mess so it's very much not clear without more details. :-)
Flags: needinfo?(haftandilian)
(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Haik Aftandilian [:haik] from comment #4)
> > I was wondering if the plan is to bake this into the Preferences singleton
> > so that accesses to preferences is still done in the same way. We'd probably
> > want to avoid a preference diverging depending on how it is obtained.
> 
> Which "Preferences singleton" are you talking about? Our prefs API is a mess
> so it's very much not clear without more details. :-)

The one setup here :)

https://dxr.mozilla.org/mozilla-central/rev/eb25b90a05c194bfd4f498ff3ffee7440f85f1cd/modules/libpref/Preferences.cpp#515

It looks like when the content process starts up, it gets a read-only copy of the preferences from the parent using PContentChild::SendReadPrefsArray. Later, code reads a preference with say Preferences::GetInt(). (Just started looking at this code, so forgive me if I missed something there.)
Flags: needinfo?(haftandilian)
(In reply to Haik Aftandilian [:haik] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > (In reply to Haik Aftandilian [:haik] from comment #4)
> > > I was wondering if the plan is to bake this into the Preferences singleton
> > > so that accesses to preferences is still done in the same way. We'd probably
> > > want to avoid a preference diverging depending on how it is obtained.
> > 
> > Which "Preferences singleton" are you talking about? Our prefs API is a mess
> > so it's very much not clear without more details. :-)
> 
> The one setup here :)
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> eb25b90a05c194bfd4f498ff3ffee7440f85f1cd/modules/libpref/Preferences.cpp#515
> 
> It looks like when the content process starts up, it gets a read-only copy
> of the preferences from the parent using PContentChild::SendReadPrefsArray.
> Later, code reads a preference with say Preferences::GetInt(). (Just started
> looking at this code, so forgive me if I missed something there.)

I was actually going to implement something on the JS side of things, which is the only places I've seen doing something in this space. It's not clear to me if/that we need something on the C++ side of things. There are several downsides to putting this into C++, to wit:

1) we wouldn't be able to modify the list of allowed prefs in artifact / frontend-only builds, which will make it harder to contribute to anything that talks to prefs on the frontend content side
2) we can't really shim this into set{Bool,Char,Int}Pref directly because those methods are synchronous. It would lead to broken expectations for bits of code that run in both processes if we were sync some of the time but not at other times. Because of issues that were brought up earlier on in this bug regarding state synchronization between different content processes, pretending this is sync would not be wise, and we shouldn't actually make it sync because pref observers on the parent process side of things might trigger other sync messages leading to deadlocks. We have an opportunity in making this API async here, and we should take it, IMO.
3) Making the API async would mean that it would be nice to have some form of callbacks/promises. From content process I'd like to do something like:

Preferences.setPrefInParent(prefName, prefValue).then(function() {
  // we'll be called when the pref set has propagated to this and other child processes.
});

AIUI this is difficult to implement on the C++ side.


Bill/Blake, does this sound sane? Or were you expecting an implementation in Preferences.cpp and friends and did we just have different expectations here? :-)
Flags: needinfo?(wmccloskey)
Flags: needinfo?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #3)
> Should we also restrict the prefs on the whitelist to be of simple types
> (char/bool/int)? This sounds good to me!

This would make sense to me. complex prefs are a bit of a nightmare anyway, and trying to shoehorn in the serialization etc. for all the types we use it for would be difficult.
I think we want a new, specialized JSM whose API makes it pretty clear that it's only for setting specific prefs and that races may result. It probably would be nice if you could use it from the parent or child processes though.
Flags: needinfo?(wmccloskey)
Yeah, I was also thinking of the JS side. As long as all of the callers are aware of the possible races, I think we're on the same page.
Flags: needinfo?(mrbkap)
Comment on attachment 8731331 [details]
MozReview Request: Bug 1252855 - allow setting a specific list of prefs from the content process, r?mrbkap,margaret

https://reviewboard.mozilla.org/r/40505/#review37127

This looks great. I have one question, but r=me with it fixed.

For b2g, we used UUIDs for message ids in a few places, but an easy incremented int is just as good afaict.

::: toolkit/modules/AsyncPrefs.jsm:76
(Diff revision 1)
> +      let error = maybeReturnErrorForReset(pref);
> +      if (error) {
> +        return Promise.reject(error);
> +      }
> +
> +      let msgId = ++this._uniqueId;

Where is this intialized?
Attachment #8731331 - Flags: review?(mrbkap) → review+
Comment on attachment 8731331 [details]
MozReview Request: Bug 1252855 - allow setting a specific list of prefs from the content process, r?mrbkap,margaret

https://reviewboard.mozilla.org/r/40505/#review37467

::: toolkit/modules/AsyncPrefs.jsm:108
(Diff revision 1)
> +      number: "setIntPref",
> +      boolean: "setBoolPref",
> +      string: "setCharPref",
> +    },
> +
> +    set: Task.async(function(pref, value) {

It kinda blows my mind that there's a parallel parent/child process API. I was going to ask how this would work on Fennec, where we only have one process, but I can see here that if this `set` method is called from the parent process it will do the right thing.

Very clever.
Attachment #8731331 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8731332 [details]
MozReview Request: Bug 1252855 - make reader mode use AsyncPrefs, r?margaret

https://reviewboard.mozilla.org/r/40507/#review37469

Very nice.
Attachment #8731332 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8731331 [details]
MozReview Request: Bug 1252855 - allow setting a specific list of prefs from the content process, r?mrbkap,margaret

https://reviewboard.mozilla.org/r/40505/#review37523

::: mobile/android/chrome/content/browser.js:16
(Diff revision 1)
>  
>  Cu.import("resource://gre/modules/AppConstants.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/AddonManager.jsm");
> +Cu.import("reosurce://gre/modules/AsyncPrefs.jsm");

Typo: s/reosurce/resource/

::: toolkit/modules/AsyncPrefs.jsm:58
(Diff revision 1)
> +
> +var AsyncPrefs;
> +if (kInChildProcess) {
> +  AsyncPrefs = {
> +    set: Task.async(function(pref, value) {
> +      let error = maybeReturnErrorForSet(pref, value);

For the kInChildProcess case, are the calls to maybeReturnErrorForSet and maybeReturnErrorForReset redundant? I was wondering if they could be skipped to avoid them being called twice for each set or reset call from the child.
Attachment #8731331 - Flags: review+
(In reply to Haik Aftandilian [:haik] from comment #19)
> Comment on attachment 8731331 [details]
> MozReview Request: Bug 1252855 - allow setting a specific list of prefs from
> the content process, r?mrbkap,margaret
> 
> https://reviewboard.mozilla.org/r/40505/#review37523
> 
> ::: mobile/android/chrome/content/browser.js:16
> (Diff revision 1)
> >  
> >  Cu.import("resource://gre/modules/AppConstants.jsm");
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  Cu.import("resource://gre/modules/Services.jsm");
> >  Cu.import("resource://gre/modules/AddonManager.jsm");
> > +Cu.import("reosurce://gre/modules/AsyncPrefs.jsm");
> 
> Typo: s/reosurce/resource/

Oof, good catch. That also explains the android orange in the try push. :-)

> ::: toolkit/modules/AsyncPrefs.jsm:58
> (Diff revision 1)
> > +
> > +var AsyncPrefs;
> > +if (kInChildProcess) {
> > +  AsyncPrefs = {
> > +    set: Task.async(function(pref, value) {
> > +      let error = maybeReturnErrorForSet(pref, value);
> 
> For the kInChildProcess case, are the calls to maybeReturnErrorForSet and
> maybeReturnErrorForReset redundant? I was wondering if they could be skipped
> to avoid them being called twice for each set or reset call from the child.

They are redundant in the sense that the parent will still call these as well, but sending the messages without checking would lead to more IPC traffic, and more potential for abuse, which was what we were trying to avoid, so it seems reasonable to just check this in both cases. Furthermore, we'd need to do some of the checks that they do in the child process anyway, because not every kind of value can be sent in an IPC message (ie you couldn't send the nsIURI object we're trying to use in the testcase, or a window/document reference, etc.). The methods are essentially O(1) anyway, and it would be... surprising... to see this API used for hot code (ie many pref sets) from the child process, so this small downside of the "better safe than sorry" approach seems OK to me.
(In reply to :Gijs Kruitbosch from comment #20)
> They are redundant in the sense that the parent will still call these as
> well, but sending the messages without checking would lead to more IPC
> traffic, and more potential for abuse, which was what we were trying to
> avoid, so it seems reasonable to just check this in both cases. Furthermore,
> we'd need to do some of the checks that they do in the child process anyway,
> because not every kind of value can be sent in an IPC message (ie you
> couldn't send the nsIURI object we're trying to use in the testcase, or a
> window/document reference, etc.). The methods are essentially O(1) anyway,
> and it would be... surprising... to see this API used for hot code (ie many
> pref sets) from the child process, so this small downside of the "better
> safe than sorry" approach seems OK to me.

Sounds good. Thanks for the explanation.
Let's make sure android is actually OK with this before I land:

https://hg.mozilla.org/try/pushloghtml?changeset=054dbe37dcd1

Also, I removed the toolkit content process script reference, as I realized it was pointless - it seems nothing really shares that scope, so consumers had to Cu.import it anyway.

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #16)
> Comment on attachment 8731331 [details]
> MozReview Request: Bug 1252855 - allow setting a specific list of prefs from
> the content process, r?mrbkap,margaret
> 
> https://reviewboard.mozilla.org/r/40505/#review37127
> 
> This looks great. I have one question, but r=me with it fixed.
> 
> For b2g, we used UUIDs for message ids in a few places, but an easy
> incremented int is just as good afaict.
> 
> ::: toolkit/modules/AsyncPrefs.jsm:76
> (Diff revision 1)
> > +      let error = maybeReturnErrorForReset(pref);
> > +      if (error) {
> > +        return Promise.reject(error);
> > +      }
> > +
> > +      let msgId = ++this._uniqueId;
> 
> Where is this intialized?

This got messed up in refactor #42 - replaced with gUniqueId.
(In reply to :Gijs Kruitbosch from comment #23)
> Let's make sure android is actually OK with this before I land:
> 
> https://hg.mozilla.org/try/pushloghtml?changeset=054dbe37dcd1

Eh, I need food - I meant:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=054dbe37dcd1
https://hg.mozilla.org/mozilla-central/rev/1f9078fce583
https://hg.mozilla.org/mozilla-central/rev/7bfa7db399f6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.