Closed
Bug 976704
Opened 10 years ago
Closed 10 years ago
Consider making Opaque security wrappers deny CALL
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Keywords: compat, dev-doc-needed)
Attachments
(1 file)
11.62 KB,
patch
|
gkrizsanits
:
review+
mrbkap
:
superreview+
|
Details | Diff | Splinter Review |
Our historical object capability semantics have been such that actors could always invoke any function they had a reference to. There's nothing all that wrong with this, and it's reasonable from a theoretical perspective. But denying things is always valuable defense-in-depth. So we should deny CALL when we can get away with it. I'm not very optimistic about the compat story on doing this for COW-ed functions. But we probably have more flexibility for non-subsuming wrappers to non-chrome functions (i.e. nsExpandedPrincipal, and cross-origin functions exposed by sloppy more-privileged code). And now's a great time to try, since we're beginning to make more use of nsExpandedPrincipal. If the privileged code wants to expose callbacks to the less-privileged code, there are a couple of other ways to do it: * Using new contentWindow.Function() to create a callback, having that callback fire a CustomEvent, and using addEventListener to listen for it from the privileged code. * Using the exportHelpers that gabor wrote. I'm going to take a crack at this.
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=fbe647e5dc2e
Assignee | ||
Comment 2•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4ddc6770673a
Assignee | ||
Comment 3•10 years ago
|
||
Green on linux64 try. Flagging for review.
Attachment #8381878 -
Flags: superreview?(mrbkap)
Attachment #8381878 -
Flags: review?(gkrizsanits)
Comment 4•10 years ago
|
||
I'd like to think about this a little, but probably this is a good idea.
Comment 5•10 years ago
|
||
I guess my problem is that we introduce yet another quirk with this patch into the security layer for questionable benefits. I would really like to have this behavior for ALL the non-subsuming wrappers. But that is hard, if even possible for now... For example in the jetpack content-script like cases where you have a scope with nsEP that might have access to some functions from chrome. This patch would make sure that even if you leak out a function from the nsEP scope it cannot be called from content, but the real dangerous case that a chrome functions is leaked to the content from there is not guarded at all. If I had to explain to an addon developer why does our security layer behave like that I would have hard time reasoning. I would prefer to keep the symmetry for all the non-subsuming wrappers as much as possible. So far I'm not sure that we should do this.
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5) > I guess my problem is that we introduce yet another quirk with this patch > into the security layer for questionable benefits. Well, I think the benefits are clear - we want defense-in-depth to protect privileged code from being manipulated by content that it doesn't trust. It's just that they don't go as far as we'd like. > I would really like to > have this behavior for ALL the non-subsuming wrappers. Me too :-) > But that is hard, if > even possible for now... Yeah. I still have hopes of fixing it at some point later, but you're right that it's some distance off. > For example in the jetpack content-script like cases where you have a scope > with nsEP that might have access to some functions from chrome. This patch > would make sure that even if you leak out a function from the nsEP scope it > cannot be called from content, but the real dangerous case that a chrome > functions is leaked to the content from there is not guarded at all. I think they're both dangerous cases, really. The severity of the latter doesn't diminish the importance of protecting against the former. > If I > had to explain to an addon developer why does our security layer behave like > that I would have hard time reasoning. That's a fair point. But I think it's a pretty benign inconsistency, and not the kind that's likely to bother anyone. Developers just try something, run into a security exception, and then try it a different way. > I would prefer to keep the symmetry > for all the non-subsuming wrappers as much as possible. So far I'm not sure > that we should do this. Symmetry is definitely nice. But the key argument in my mind is that this is a change that's easy to make now and might be very hard to make later. This is defense-in-depth - we don't know the attack vector ahead of time, but we might as well plug the holes that are easy and within reach. Our membrane is our best line of universal defense against tricky vectors, so I want it to be as tight as possible.
Comment 7•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #6) > Well, I think the benefits are clear - we want defense-in-depth to protect > privileged code from being manipulated by content that it doesn't trust. > It's just that they don't go as far as we'd like. > I think they're both dangerous cases, really. The severity of the latter > doesn't diminish the importance of protecting against the former. Hard to argue against this. Alright let's do this then.
Comment 8•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #6) > That's a fair point. But I think it's a pretty benign inconsistency, and not > the kind that's likely to bother anyone. Developers just try something, run > into a security exception, and then try it a different way. Actually exactly that's what I'm afraid of. And to achieve what they want in this case is to expose a chrome function instead, because that works... But anyway, in the hope of we are moving forward to a world where call is banned for every non-subsuming wrapper let's do this. One question: how much work would that be to remove that quirk about XBLs? Is it feasible at all (given the cloning / possible performance cost)?
Comment 9•10 years ago
|
||
Comment on attachment 8381878 [details] [diff] [review] Make opaque security wrappers non-callable. v3 Review of attachment 8381878 [details] [diff] [review]: ----------------------------------------------------------------- I would like to hear Blake's thoughts on this, but I'm sort of OK with this change. And the patch looks fine.
Attachment #8381878 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8) > One question: how much work would that be to remove that quirk about XBLs? > Is it feasible at all > (given the cloning / possible performance cost)? Totally feasible. Calls from content to in-content XBL are rare, and we don't care much about the performance. It should be an easy patch. I've filed bug 978665 about it.
Comment 11•10 years ago
|
||
Comment on attachment 8381878 [details] [diff] [review] Make opaque security wrappers non-callable. v3 Review of attachment 8381878 [details] [diff] [review]: ----------------------------------------------------------------- I'm in favor of this: in practice it shouldn't matter. I think one of the reasons this used to make sense was that without it, COWs would have had needed another way of exposing functions. ::: js/xpconnect/wrappers/AccessCheck.h @@ +42,5 @@ > + static bool deny(js::Wrapper::Action act, JS::HandleId id) { > + return false; > + } > + static bool allowNativeCall(JSContext *cx, JS::IsAcceptableThis test, JS::NativeImpl impl) > + { Nit: this bracing style doesn't match the other two functions.
Attachment #8381878 -
Flags: superreview?(mrbkap) → superreview+
Assignee | ||
Comment 12•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8176cde1dca1
Assignee | ||
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf86e72970a
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4bf86e72970a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•10 years ago
|
Keywords: compat,
dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•