Closed
Bug 1252855
Opened 9 years ago
Closed 9 years ago
Sandboxing and setting prefs from chrome-privileged content JS: need to settle on a policy / implementation
Categories
(Core :: Security: Process Sandboxing, defect)
Core
Security: Process Sandboxing
Tracking
()
RESOLVED
FIXED
mozilla48
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)
Assignee | ||
Comment 2•9 years ago
|
||
I'm a chump, so I'll sign up, sure. :-)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(mrbkap)
Comment 3•9 years ago
|
||
Should we also restrict the prefs on the whitelist to be of simple types (char/bool/int)? This sounds good to me!
Comment 4•9 years ago
|
||
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");
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
(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)
Comment 8•9 years ago
|
||
(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)
Assignee | ||
Comment 9•9 years ago
|
||
(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)
Assignee | ||
Comment 10•9 years ago
|
||
(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)
Comment 12•9 years ago
|
||
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)
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40505/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40505/
Attachment #8731331 -
Flags: review?(mrbkap)
Attachment #8731331 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 15•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40507/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40507/
Attachment #8731332 -
Flags: review?(margaret.leibovic)
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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 18•9 years ago
|
||
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 19•9 years ago
|
||
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+
Assignee | ||
Comment 20•9 years ago
|
||
(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.
Comment 21•9 years ago
|
||
(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.
Comment 22•9 years ago
|
||
Assignee | ||
Comment 23•9 years ago
|
||
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.
Assignee | ||
Comment 24•9 years ago
|
||
(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
Comment 25•9 years ago
|
||
Comment 26•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1f9078fce583
https://hg.mozilla.org/mozilla-central/rev/7bfa7db399f6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•