Closed Bug 861530 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox23 --- fixed
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Keywords: sec-audit, Whiteboard: [adv-main23+])

Attachments

(1 file)

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.
maybe moz_bug_r_a4 or codyc can have some fun with this one.
Could you please cc me on bug 826471?
(In reply to moz_bug_r_a4 from comment #2)
> Could you please cc me on bug 826471?

Done.
Attachment #739191 - Flags: review?(bobbyholley+bmo)
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);

?
Attachment #739191 - Flags: review?(bobbyholley+bmo) → review+
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.
(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).
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.
What security rating would you give this, Bobby?  I have no idea how bad this is.
Flags: needinfo?(bobbyholley+bmo)
I don't see an immediate security problem here.
Flags: needinfo?(bobbyholley+bmo)
Keywords: sec-audit
I'll just land the test as part of this, too.
  is(sandbox._content, sandbox.content, "_content and content returned the same thing");
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?
Depends on: 865260
(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.
https://hg.mozilla.org/mozilla-central/rev/2893ed05e739
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Whiteboard: [adv-main23+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.