861530 - Assertion: "id got defined somewhere else?" with Xrays and _content

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

At some point, the test case from 826471 started failing with this assertion (accessing _content on a sandbox).

bholley via mail: "That Xray code ends up passing |wrapper| (an XrayWrapper) as |obj| to nsWindowSH::NewResolve, which would ordinarily expect a Window reflector. When the resolve hook defines properties on |obj|, we end up in the defineProperty hook of the XrayWrapper, which detects that we're actually mid-resolve here, and subsequently goes and defines the property on the holder. So this check is basically making sure that if the resolve hook claims to have succeeded, that the property made it safely through the whole mess and ended up on the holder.

Looking at the s_content_id stuff in nsDOMClassInfo.cpp, that appears to be the exact bug we're catching, since it looks like the resolve code defines the property on |windowObj|, rather than on |obj| like it's supposed to."

I don't know if this is actually a security problem or not, but it sounds sketchy.

Comment 1

[Daniel Veditz [:dveditz]]

maybe moz_bug_r_a4 or codyc can have some fun with this one.

Comment 2

[moz_bug_r_a4]

Could you please cc me on bug 826471?

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to moz_bug_r_a4 from comment #2)
> Could you please cc me on bug 826471?

Done.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: handle Xrays on _content more correctly

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 739191 [details] [diff] [review]
handle Xrays on _content more correctly

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

r=bholley with comment fixed or an appropriate explanation.

::: dom/base/nsDOMClassInfo.cpp
@@ +5376,5 @@
> +        return NS_ERROR_DOM_SECURITY_ERR;
> +      }
> +    } else {
> +      windowObj = obj;
> +    }

Why can't this just be:
JSObject* windowObj = js::UnwrapObjectChecked(obj, /* stopAtOuter = */ false);
NS_ENSURE_TRUE(windowObj, NS_ERROR_DOM_SECURITY_ERROR);

?

Comment 6

[Andrew McCreight [:mccr8]]

I don't know how that ObjectIsNativeWrapper guard interacts with unwrapping, so I was just cargo culting.  If you think it should be okay to do what you said, then I'll try that out and see if it works.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> I don't know how that ObjectIsNativeWrapper guard interacts with unwrapping,
> so I was just cargo culting.  If you think it should be okay to do what you
> said, then I'll try that out and see if it works.

ObjectIsNativeWrapper just checks if the thing is an XrayWrapper that subsumes its target. CheckedUnwrap will just unwrap wrappers unless it hits one that doesn't subsume its target. So the only difference in behavior between the two would be if |obj| was a security wrapper (one that did not subsume its target) - i.e. a cross-origin Xray wrapper. But we'd never have such a situation, because all XOWs filter before property access so they could never get to the point where they might resolve _content. So this should be fine. And even if that case somehow happened, my proposed behavior (throwing) is certainly better than the behavior in the patch (compartment mismatch).

Comment 8

[Bobby Holley (:bholley)]

Actually, once I fix bug 862380, it will be possible to trigger the resolution of filtered names if they get resolved in the enumerate hook. But we're still safe here, and regardless _content isn't in nsWindowSH::Enumerate.

Comment 9

[Andrew McCreight [:mccr8]]

What security rating would you give this, Bobby?  I have no idea how bad this is.

Comment 10

[Bobby Holley (:bholley)]

I don't see an immediate security problem here.

Comment 11

[Andrew McCreight [:mccr8]]

I'll just land the test as part of this, too.
  is(sandbox._content, sandbox.content, "_content and content returned the same thing");

Comment 12

[Bobby Holley (:bholley)]

Note that I'm going to make these properties chrome-only soon: bug 864845.

Can we write this test such that it won't break when that happens?

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to Bobby Holley (:bholley) from comment #12)
> Can we write this test such that it won't break when that happens?

The test is in test_sandbox_bindings.xul, which is a chrome mochitest, so IIUC that should be okay.  On the other hand, my test in bug 863390 will break, so I'll just wait until you fix bug 864845.

Comment 14

[Andrew McCreight [:mccr8]]

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

Comment 15

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2893ed05e739

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2893ed05e739