Closed
Bug 1082450
Opened 10 years ago
Closed 10 years ago
Disallow exposing privileged functions via COWs
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Keywords: addon-compat)
Attachments
(9 files, 1 obsolete file)
4.62 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
2.79 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
2.54 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
1.45 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
7.82 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
6.88 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
2.39 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
5.69 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
1.37 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
When I went to land bug 1065186, I started getting nervous about the fact that the privileged Array.prototype will no longer be remapped to the content scope. This shouldn't be a problem in general, but it made me think that we could really tighten down a bit in that area. I don't think we'll ever be able to drop support for invoking chrome callables from content, so I think we should focus on mechanisms of obtaining those callables in the first place. I've got some patches that I'll be pushing to try.
Assignee | ||
Comment 1•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=438e07595781
Assignee | ||
Comment 2•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=82479ad2c84f
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8504957 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8504958 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8504959 -
Flags: review?(jmaher)
Attachment #8504959 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 6•10 years ago
|
||
In the next patch, we deny access to any accessor property, so this is now obsolete for COWs. We also do something like this for new-style XOWs, but that's exhaustively covered in test_crossOriginObjects.html.
Attachment #8504960 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8504962 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8504964 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8504965 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 11•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=78f75a9403d8
Comment 12•10 years ago
|
||
Comment on attachment 8504957 [details] [diff] [review] Part 1 - Rip exposedProps out of Social API code. v1 Review of attachment 8504957 [details] [diff] [review]: ----------------------------------------------------------------- This generally looks OK. Wants a try push to be sure, but you did that already. ::: toolkit/components/social/FrameWorker.jsm @@ -153,5 @@ > - onmessage: "rw", > - postMessage: "r", > - close: "r", > - toString: "r" > - }, Are you sure the socialapi use is the only use of the port here? I thought there were other consumers, but perhaps I'm wrong? ::: toolkit/components/social/FrameWorkerContent.js @@ +124,5 @@ > userAgent: workerWindow.navigator.userAgent, > + }, sandbox); > + Object.defineProperty(Cu.waiveXrays(navigator), 'onLine', > + {configurable: true, enumerable: true, > + get: Cu.exportFunction(() => workerWindow.navigator.onLine, sandbox)}); Nit: maybe use a (..., { foo: bar baz: quux }); notation here in order to make it wrap a little nicer? :-) ::: toolkit/components/social/MozSocialAPI.jsm @@ +122,5 @@ > + }, targetWindow, {cloneFunctions: true}); > + > + // Jump through hoops to define the accessor property. > + let abstractPortPrototype = Object.getPrototypeOf(Object.getPrototypeOf(port)); > + let desc = Object.getOwnPropertyDescriptor(port.__proto__.__proto__, 'onmessage'); Ew.
Attachment #8504957 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8504958 [details] [diff] [review] Part 2 - Use automation machinery instead of exposedProps in Mock{Color,File}Picker.jsm. v1 Review of attachment 8504958 [details] [diff] [review]: ----------------------------------------------------------------- Could you also remove the __exposedProps__ part from MockColorPicker?
Attachment #8504958 -
Flags: review?(gkrizsanits) → review+
Comment 14•10 years ago
|
||
Comment on attachment 8504959 [details] [diff] [review] Part 3 - Remove exposedProps from reftest harness. v1 Review of attachment 8504959 [details] [diff] [review]: ----------------------------------------------------------------- I assume the read-only part of these properties are not very important anyway.
Attachment #8504959 -
Flags: review?(gkrizsanits) → review+
Updated•10 years ago
|
Attachment #8504960 -
Flags: review?(gkrizsanits) → review+
Comment 15•10 years ago
|
||
Comment on attachment 8504961 [details] [diff] [review] Part 5 - Correctly propagate exceptions from ExposedPropertiesOnly::check. v1 Review of attachment 8504961 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp @@ +286,5 @@ > return true; > // COWs fail silently for GETs, and that also happens to be the only case > // where we might want to redirect the lookup to the home prototype chain. > + *bp = (act == Wrapper::GET || act == Wrapper::ENUMERATE || > + act == Wrapper::GET_PROPERTY_DESCRIPTOR) && !JS_IsExceptionPending(cx); So how can we get here exactly with exception already pending?
Comment 16•10 years ago
|
||
Comment on attachment 8504962 [details] [diff] [review] Part 6 - Deny access to accessor properties on COWs. v1 Review of attachment 8504962 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/AccessCheck.cpp @@ +343,5 @@ > return false; > } > > + // Inspect the property on the underlying object to check for red flags. > + if (!JS_GetPropertyDescriptorById(cx, wrappedObject, id, &desc)) This probably not the most interesting case, but shouldn't we check also if the underlying object is a js proxy here that could interrupt this call and fool us?
Attachment #8504962 -
Flags: review?(gkrizsanits) → review+
Updated•10 years ago
|
Attachment #8504964 -
Flags: review?(gkrizsanits) → review+
Comment 17•10 years ago
|
||
Comment on attachment 8504959 [details] [diff] [review] Part 3 - Remove exposedProps from reftest harness. v1 Review of attachment 8504959 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/tools/reftest/reftest.js @@ +600,5 @@ > var sandbox = new Components.utils.Sandbox(aURL.spec); > var xr = CC[NS_XREAPPINFO_CONTRACTID].getService(CI.nsIXULRuntime); > var appInfo = CC[NS_XREAPPINFO_CONTRACTID].getService(CI.nsIXULAppInfo); > sandbox.isDebugBuild = gDebug.isDebugBuild; > + sandbox.xulRuntime = CU.cloneInto({widgetToolkit: xr.widgetToolkit, OS: xr.OS}, sandbox); where do we get XPCOMABI from then?
Attachment #8504959 -
Flags: review?(jmaher) → review-
Updated•10 years ago
|
Attachment #8504965 -
Flags: review?(gkrizsanits) → review+
Comment 18•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #17) > where do we get XPCOMABI from then? xulRuntime property will be an object from the sandbox itself because of the cloneInto, hence there will be no restriction from the sandbox to access its properties. So when few lines later XPCOMABI is set on it, that should be accessible from the sandbox without any further __exposedProps__ magic.
Comment 19•10 years ago
|
||
Comment on attachment 8504961 [details] [diff] [review] Part 5 - Correctly propagate exceptions from ExposedPropertiesOnly::check. v1 Review of attachment 8504961 [details] [diff] [review]: ----------------------------------------------------------------- r=me since it's correct, just want to understand the case where we actually need this...
Attachment #8504961 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15) > Comment on attachment 8504961 [details] [diff] [review] > Part 5 - Correctly propagate exceptions from ExposedPropertiesOnly::check. v1 > > Review of attachment 8504961 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp > @@ +286,5 @@ > > return true; > > // COWs fail silently for GETs, and that also happens to be the only case > > // where we might want to redirect the lookup to the home prototype chain. > > + *bp = (act == Wrapper::GET || act == Wrapper::ENUMERATE || > > + act == Wrapper::GET_PROPERTY_DESCRIPTOR) && !JS_IsExceptionPending(cx); > > So how can we get here exactly with exception already pending? The exceptions thrown in the Part 6 and Part 7 hit this, because of the way that ChromeObjectWrapper calls enter() multiple times in funny ways. This will all be obsolete very soon (hopefully next week) when we rm ChromeObjectWrapper.cpp in bug 1081985 .(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16) > This probably not the most interesting case, but shouldn't we check also if > the underlying object is a js proxy here that could interrupt this call and > fool us? We never generate COWs for proxies (at least since bug 1064437 - see ForceCOWBehavior).
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #12) > Are you sure the socialapi use is the only use of the port here? I thought > there were other consumers, but perhaps I'm wrong? I don't know this code really. CI is green, which is all I can say.
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #18) > (In reply to Joel Maher (:jmaher) from comment #17) > > where do we get XPCOMABI from then? > > xulRuntime property will be an object from the sandbox itself because of the > cloneInto, hence there will be no restriction from the sandbox to access its > properties. So when few lines later XPCOMABI is set on it, that should be > accessible from the sandbox without any further __exposedProps__ magic. What gabor said. I'll upload an additional patch that's a bit clearer though.
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8504959 -
Attachment is obsolete: true
Attachment #8505426 -
Flags: review?(jmaher)
Comment 24•10 years ago
|
||
Comment on attachment 8505426 [details] [diff] [review] Part 3 - Remove exposedProps from reftest harness. v2 r=gabor Review of attachment 8505426 [details] [diff] [review]: ----------------------------------------------------------------- thanks
Attachment #8505426 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 25•10 years ago
|
||
Need this to get valgrind builds green.
Attachment #8505432 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 26•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7f458a3dfa8f
Updated•10 years ago
|
Attachment #8505432 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 27•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1f19fa816c97 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/687aa81bd41c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/84aa4151a88a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb6ba88d36c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c86f6441049a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/39ca4817b51c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/df4ebb4aa707 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/76dca963dd49 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/204a676d3c8e
Comment 28•10 years ago
|
||
This patch appears to break the add-on https://addons.mozilla.org/en-US/firefox/addon/self-destructing-cookies/versions/0.4.7-pre2. GOOD ---- 20141015060116 https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9d6c298b5a BAD --- 20141015060615 https://hg.mozilla.org/integration/mozilla-inbound/rev/204a676d3c8e Mozilla/5.0 (Windows NT 6.3; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1f19fa816c97 https://hg.mozilla.org/mozilla-central/rev/687aa81bd41c https://hg.mozilla.org/mozilla-central/rev/84aa4151a88a https://hg.mozilla.org/mozilla-central/rev/2bb6ba88d36c https://hg.mozilla.org/mozilla-central/rev/c86f6441049a https://hg.mozilla.org/mozilla-central/rev/39ca4817b51c https://hg.mozilla.org/mozilla-central/rev/df4ebb4aa707 https://hg.mozilla.org/mozilla-central/rev/76dca963dd49 https://hg.mozilla.org/mozilla-central/rev/204a676d3c8e
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 30•10 years ago
|
||
Can this be backed out since this is breaking lots of jetpack and gaia integration tests?
Flags: needinfo?(bobbyholley)
Comment 32•10 years ago
|
||
> Can this be backed out since this is breaking lots of jetpack and gaia integration tests?
It would be _really_ helpful to actually point to the relevant tests. I could have helped hunt this down earlier today if you'd done that.
Backing stuff out for breaking secret tests is ... I don't know. What is the developer supposed to do after that, exactly?
Comment 33•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #32) > > Can this be backed out since this is breaking lots of jetpack and gaia integration tests? > > It would be _really_ helpful to actually point to the relevant tests. I > could have helped hunt this down earlier today if you'd done that. > > Backing stuff out for breaking secret tests is ... I don't know. What is > the developer supposed to do after that, exactly? Sorry, this is being tracked in bug 1083571 and has more info. I agree that it sucks that this suite is hidden, and we're working hard on getting this unhidden.
Assignee | ||
Comment 34•10 years ago
|
||
Given that the gaia stuff is a test-only issue and no actual device functionality was broken, I don't think this warrants a backout. I'm helping folks fix the tests today.
Keywords: checkin-needed
Assignee | ||
Comment 35•10 years ago
|
||
(In reply to Kevin Grandon :kgrandon from comment #30) > Can this be backed out since this is breaking lots of jetpack and gaia > integration tests? I am in touch with timdream and the gaia folks, and am helping them fix the tests. I will look at the jetpack things next.
Flags: needinfo?(bobbyholley)
Comment 36•10 years ago
|
||
This also broke the tests for j2me.js, per https://github.com/andreasgal/j2me.js/issues/464.
Assignee | ||
Updated•10 years ago
|
Keywords: addon-compat
You need to log in
before you can comment on or make changes to this bug.
Description
•