Closed Bug 856384 Opened 8 years ago Closed 8 years ago

"ASSERTION: PostCreate failed" and crash with Object.seal(win)

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox21 --- unaffected
firefox22 + fixed
firefox23 + verified
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: jruderman, Assigned: Waldo)

References

Details

(5 keywords, Whiteboard: [fuzzblocker][adv-main22-])

Attachments

(3 files)

Attached file testcase
###!!! ASSERTION: PostCreate failed! This is known to cause inconsistent state for some class types and may even cause a crash in combination with a JS GC. Fix the failing PostCreate ASAP!: 'Error', file js/xpconnect/src/XPCWrappedNative.cpp, line 713

###!!! ASSERTION: tearoff not empty in dtor: '!(GetInterface()||GetNative()||GetJSObjectPreserveColor())', file js/xpconnect/src/XPCInlines.h, line 552

Plus some errors in chrome JS.

I suspect this is a regression from bug 789897.
Attached file stacks (gdb)
This can lead to all kinds of GC-related crashes.
Group: core-security
Keywords: sec-critical
Whiteboard: [fuzzblocker]
Assignee: general → jwalden+bmo
I bet the window isn't used to being sealed because it's always been wrapped via the outer window proxy, which, despite being transparent, never would have transmitted preventExtensions until now.
Yeah, we can't possibly support sealing of objects with a resolve hook.. at least not without first calling the resolve hook for all relevant property names!
(In reply to Boris Zbarsky (:bz) from comment #4)
> Yeah, we can't possibly support sealing of objects with a resolve hook.. at
> least not without first calling the resolve hook for all relevant property
> names!

Sure we can.  The resolve/enumerate contract is that anything that can be resolved, the enumerate hook has to define onto the object.  (At least, when called with JSITER_ALL, at least.  I can't remember if everything must be resolved onto the object when that's not there.)  When the stuff to enumerate, and the stuff resolved, is inconstant...um, yeah, need to think more.

In the longer WebIDL run the window object becomes a normal object with GSP being the cracktastic proto-chain-member here, so in theory you could maybe make the window object preventExtensions-able.  Maybe.  In the short run maybe we should just say "no".
> The resolve/enumerate contract is that anything that can be resolved, the enumerate hook
> has to define onto the object.

Does seal() call enumerate?

> In the longer WebIDL run the window object becomes a normal object with GSP being the
> cracktastic proto-chain-member

We're already there, afaik.  The only weirdness on the Window is the resolve hook that randomly resolves interface objects, standard classes, etc.
ObjectImpl::preventExtensions calls GetPropertyNames(cx, self, JSITER_HIDDEN | JSITER_OWNONLY, &props).  And seal has to call that, as its first step.
Ah, ok.

So I looked at the testcase, and the problem is it's sealing a WindowProxy, not a Window.  I don't see a sane way a WindowProxy can be sealable.
(In reply to Boris Zbarsky (:bz) from comment #8)
> Ah, ok.
> 
> So I looked at the testcase, and the problem is it's sealing a WindowProxy,
> not a Window.  I don't see a sane way a WindowProxy can be sealable.

Well, we could just forward it to the inner, right? What does the spec say to do here?
> Well, we could just forward it to the inner, right?

We could, yes.  However note that the inner has, per spec, indexed properties that come and go.

> What does the spec say to do here?

Funny man.  ;)
Test fails without patch:

367 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/base/test/test_window_extensible.html | preventExtensions(window) should throw a TypeError, instead got: didn't throw - got false, expected true
368 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/base/test/test_window_extensible.html | Windows are extensible after an extensibility "change" attempt - got false, expected true
JavaScript error: http://mochi.test:8888/tests/SimpleTest/TestRunner.js, line 264: TypeError: $(...).contentWindow.location is not extensible

and passes with.  Ideally the test would continue in an unfixed build, but I'm not sure I see how things would go wrong quite well enough to do that.  I'm not sure it's worth the time to figure out what exactly is going wrong and how, to do that.

I very much dislike the proxy handler trap-defining setup.  It seems very ad-hoc how you know what traps to override, in which classes, and if you miss one, you just don't know unless a fuzzer hits it.  Which so far has been easy, but I wouldn't count on it in all cases.
Attachment #731955 - Flags: review?(tschneidereit)
Attachment #731955 - Flags: review?(bobbyholley+bmo)
Comment on attachment 731955 [details] [diff] [review]
Make nsOuterWindowProxy say always [[Extensible]]

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

Yes
Attachment #731955 - Flags: review?(tschneidereit) → review+
Comment on attachment 731955 [details] [diff] [review]
Make nsOuterWindowProxy say always [[Extensible]]

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

r=me
Attachment #731955 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 731955 [details] [diff] [review]
Make nsOuterWindowProxy say always [[Extensible]]

The regressing changes landed just early enough to be before the imminent uplift, and this patch would land now after the uplift.  So, sec-approval dance.  Blargh.  :-\  If I'd been thinking at all I'd have held off a few more days til after uplift; sorry for not thinking here.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Moderate to hard.  There's not an obvious straight-line path from issue to exploit, but it's probably not difficult to find *a* path with a little work.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Given this issue's going to exist for all of a few days, I didn't think it worth trying to obscure the issue at all in the patch/test.

Which older supported branches are affected by this flaw?
Only Aurora, assuming uplift gets the regressor but won't get this patch.

If not all supported branches, which bug introduced the flaw?
Bug 789897

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes, this patch.

How likely is this patch to cause regressions; how much testing does it need?
It's clear and obvious enough of a fix -- shouldn't regress anything.
Attachment #731955 - Flags: sec-approval?
Attachment #731955 - Flags: sec-approval? → sec-approval+
Comment on attachment 731955 [details] [diff] [review]
Make nsOuterWindowProxy say always [[Extensible]]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 789897
User impact if declined: Various internal invariants violated, consequences unknown for them, quite possibly you can make bad stuff happen (see the assert noted here).
Testing completed (on m-c, etc.): Hasn't landed on m-c yet, because of stupid mis-timing on my part and having the regressor and followups split around a merge date.  :-\
Risk to taking this patch (and alternatives if risky): None.
String or IDL/UUID changes made by this patch: None.
Attachment #731955 - Flags: approval-mozilla-aurora?
This patch looks extremely safe, but is there any reason to approve before this lands to m-c per normal process?
I thought normal process was landing closer to simultaneously everywhere, for security bug things.  Am I mistaken?
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> I thought normal process was landing closer to simultaneously everywhere,
> for security bug things.  Am I mistaken?

We don't require that. Normally, things land on m-c and are then nominated for branches.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0bab8308f561
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Attachment #731955 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main22-]
Group: core-security
Verified fixed with the latest beta debug build, on a Mac OSX 10.8.4 machine. No assertion failure is now present.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20130716 Firefox/23.0
Build ID: 20130716071311
You need to log in before you can comment on or make changes to this bug.