Closed
Bug 861530
Opened 12 years ago
Closed 12 years ago
Assertion: "id got defined somewhere else?" with Xrays and _content
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Keywords: sec-audit, Whiteboard: [adv-main23+])
Attachments
(1 file)
2.13 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
maybe moz_bug_r_a4 or codyc can have some fun with this one.
Comment 2•12 years ago
|
||
Could you please cc me on bug 826471?
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to moz_bug_r_a4 from comment #2) > Could you please cc me on bug 826471? Done.
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #739191 -
Flags: review?(bobbyholley+bmo)
Comment 5•12 years ago
|
||
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+
Assignee | ||
Comment 6•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
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.
Assignee | ||
Comment 9•12 years ago
|
||
What security rating would you give this, Bobby? I have no idea how bad this is.
Flags: needinfo?(bobbyholley+bmo)
Comment 10•12 years ago
|
||
I don't see an immediate security problem here.
Flags: needinfo?(bobbyholley+bmo)
Assignee | ||
Comment 11•12 years ago
|
||
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•12 years ago
|
||
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?
Assignee | ||
Comment 13•12 years ago
|
||
(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.
Assignee | ||
Comment 14•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=832da19f3b18
Assignee | ||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2893ed05e739
Flags: in-testsuite+
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2893ed05e739
Status: NEW → RESOLVED
Closed: 12 years ago
status-firefox23:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•12 years ago
|
status-b2g18:
--- → wontfix
status-firefox-esr17:
--- → wontfix
Updated•11 years ago
|
Whiteboard: [adv-main23+]
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•