Bobby Holley (:bholley) Assignee
	Description • 12 years ago

I realized recently after landing bug 667388 that there's a potential security issue here.

The PUNCTURE machinery correctly prevents code from cloning objects that it shouldn't be able to touch. However, after doing so it just unwraps the objects. So we don't do any principal pushing (a la NoWaiverWrapper::enter).

This means that untrusted code could fool caps if it could arrange getters and setters that don't leave scripted frames on the stack. Is this a security issue, Blake, or is there some reason we can sneak by?

It'd be kind of a shame to add extra goop here, especially since we're just about to be able to kill all this stuff with compartment-per-global. But we might have to.

When doing the structured clone, the stack of objects can contain objects from various compartments. The general idea is that anything on the object stack is safe to access (i.e., we can enter its compartment). So the simplest solution would just be to push a principal each time we enter a compartment in the structured clone code. We could probably accomplish this with some sort of runtime callback.

What are your thoughts, Blake?

PS: This whole discussion got kicked off when we started wondering why we're entering a compartment here: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsStructuredCloneContainer.cpp#84 . Justin said that Blake told him to put that in, but had no idea what it was doing. If you have an extra moment Blake, I'd be curious as to what's going on there.

	Daniel Veditz [:dveditz]
	Comment 1 • 12 years ago

You're not saying this is a regression from bug 667388, that's just when you happened to notice the problem? I'm assuming this has been a problem since since Firefox 4/compartments?

How soon is the compartment-per-global change coming, and are you sure it will fix this?

	Bobby Holley (:bholley) Assignee
	Comment 2 • 12 years ago

No, this is a regression from bug 667388. Before that, we'd just throw when trying to structured clone objects.

So the regression is only on nightly. Assuming Blake agrees, I think we should land the fix I described in comment 0 before landing any compartment-per-global stuff. Though in order to test this, we'll have to write a pretty tricky testcase. Hopefully blake can tell me where to look for similar things.

	Daniel Veditz [:dveditz]
	Comment 3 • 12 years ago

We should try to fix this before Firefox 14 move up to the Aurora train.

	Bobby Holley (:bholley) Assignee
	Comment 4 • 12 years ago

(In reply to Daniel Veditz [:dveditz] from comment #3)
> We should try to fix this before Firefox 14 move up to the Aurora train.

Sure. Before we do that though, I need Blake to confirm that this is actually a security issue and point me to the relevant bugs where I can find similar testcases.

	moz_bug_r_a4
	Comment 5 • 12 years ago

(In reply to Bobby Holley (:bholley) from comment #0)
> Is this a security issue, Blake, or is there some reason we can sneak by?

Yes, it is a security issue.  I'll attach an arbitrary code execution testcase.

	moz_bug_r_a4
	Comment 6 • 12 years ago

This uses bug 344495's trick.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 7 • 12 years ago

I don't know why a runtime callback is needed... there's already APIs to push principals on caps, but yeah, sorry I missed this in review.

	Bobby Holley (:bholley) Assignee
	Comment 8 • 12 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #7)
> I don't know why a runtime callback is needed... there's already APIs to
> push principals on caps

Oh, sweet. Which ones?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 9 • 12 years ago

Hmm, maybe I spoke too quickly. I was thinking of adding the principal pushing code in the structured clone callbacks where we have access to all of XPCOM.

	Bobby Holley (:bholley) Assignee
	Comment 10 • 12 years ago

Attaching a patch that fixes moz_bug_r_a4's testcase. Flagging Blake for review.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 11 • 12 years ago

Comment on attachment 612444 [details] [diff] [review]
Push principals when entering compartments in structured clone. v1

Given that the push principals API is dying soon and you're only using it in two places, I don't see the appeal of adding a JS API for this. The structured clone writer can do this work without the weight of an additional API (though the work can definitely be shared between the two callsites). Or is there another reason to prefer the JS API for this?

	Bobby Holley (:bholley) Assignee
	Comment 12 • 12 years ago

Comment on attachment 612444 [details] [diff] [review]
Push principals when entering compartments in structured clone. v1

Blake and I talked about this on IRC. Reflagging for review.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 13 • 12 years ago

Comment on attachment 612444 [details] [diff] [review]
Push principals when entering compartments in structured clone. v1

Review of attachment 612444 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly small changes (but a couple of non-nits). Let me know if you want me to take another look.

::: js/src/jsapi.h
@@ +2731,1 @@
>      void* bytes[sizeof(void*) == 4 && MOZ_ALIGNOF(uint64_t) == 8 ? 16 : 13];

I'd prefer getters to exposing this directly.

::: js/src/jsclone.cpp
@@ +514,5 @@
> +{
> +  public:
> +    bool enter(JSContext *cx, JSObject *target) {
> +
> +        // First, enter the compartment.

Nit: extra newline.

@@ +524,5 @@
> +            return true;
> +
> +        // Push.
> +        const JSSecurityCallbacks *cb = cx->runtime->securityCallbacks;
> +        return cb->pushContextPrincipal(cx, cx->compartment->principals);

Probably need to use findObjectPrincipals until CPG.

@@ +529,5 @@
> +    };
> +
> +    ~AutoEnterCompartmentAndPushPrincipal() {
> +
> +        // Pop the principal if necessary.

Nit: extra newline.

	Bobby Holley (:bholley) Assignee
	Comment 14 • 12 years ago

Addressed review comments and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ddf1e9e068a2

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 15 • 12 years ago

(In reply to Bobby Holley (:bholley) from comment #13)
> Blake and I talked about this on IRC.

It'd be super cool if you could put a brief summary of the IRC discussion in the bug, so that bug watchers (present and future!) can have an idea what changed between comment 12 and comment 14.

	Bobby Holley (:bholley) Assignee
	Comment 16 • 12 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #16)
> (In reply to Bobby Holley (:bholley) from comment #13)
> > Blake and I talked about this on IRC.
> 
> It'd be super cool if you could put a brief summary of the IRC discussion in
> the bug, so that bug watchers (present and future!) can have an idea what
> changed between comment 12 and comment 14.

There's not much to report, really. I think Blake just misunderstood the patch and thought the callbacks amounted to spidermonkey providing an API to consumers, rather than the reverse.

From IRC:
> bholley: I don't understand your comment in the principal pushing bug
> bholley: how can the JS structured clone writer do the work?
> bholley: is there a way that spidermonkey can do arbitrary caps stuff?
> mrbkap: can you send me the # again?
> bholley: 739825
> mrbkap: Wow, I was totally lost.

	Bobby Holley (:bholley) Assignee
	Comment 17 • 12 years ago

Looks green. Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/6120308406f3

	Bobby Holley (:bholley) Assignee
	Comment 18 • 12 years ago

Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/6120308406f3

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 19 • 12 years ago

Thanks Bobby! One less bug to look at during critsmash \o/

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 20 • 12 years ago

This only affected trunk, right?

If so, lets open this.