976704 - Consider making Opaque security wrappers deny CALL

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=fbe647e5dc2e

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=4ddc6770673a

Comment 3

[Bobby Holley (:bholley)]

Attachment: Make opaque security wrappers non-callable. v3

Green on linux64 try. Flagging for review.

Comment 4

[Gabor Krizsanits (INACTIVE)]

I'd like to think about this a little, but probably this is a good idea.

Comment 5

[Gabor Krizsanits (INACTIVE)]

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.

Comment 6

[Bobby Holley (:bholley)]

(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

[Gabor Krizsanits (INACTIVE)]

(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

[Gabor Krizsanits (INACTIVE)]

(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

[Gabor Krizsanits (INACTIVE)]

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.

Comment 10

[Bobby Holley (:bholley)]

(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

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 12

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=8176cde1dca1

Comment 13

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf86e72970a

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4bf86e72970a