Closed Bug 860591 Opened 7 years ago Closed 7 years ago

"ABORT: what kind of wrapper is this?" with events

Categories

(Core :: XPConnect, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

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

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [adv-main22-])

Attachments

(5 files, 3 obsolete files)

Attached file testcase
###!!! ABORT: what kind of wrapper is this?: 'IS_SLIM_WRAPPER(cur)', file js/xpconnect/src/XPCQuickStubs.cpp, line 633
Attached file stack
Component: DOM: Events → XPConnect
So unsurprisingly "cur" is a WebIDL object.

The code looks like this:

628             if (NS_SUCCEEDED(getNative(native, entries, cur, iid, ppThis, pThisRef, vp))) {
629                 if (lccx) {
630                     // This only matters for unwrapping of this objects, so we
631                     // shouldn't end up here for the new DOM bindings.
632                     NS_ABORT_IF_FALSE(IS_SLIM_WRAPPER(cur),
633                                       "what kind of wrapper is this?");
634                     lccx->SetWrapper(cur);
635                 }

It seems to me that that comment is obviously wrong.  I'm a little surprised we never hit this before...  I guess in the past we always got rid of quicktubs for the shared interfaces (replacing them with new-bindings methods on all the protos) before we started converting some instances.  We thought of it as an optimization, but it was actually a necessary correctness fix!

Can we do that for events?  Olli, are there already some webidl events on Aurora 22?
Flags: needinfo?(bugs)
Assignee: nobody → bzbarsky
Attached patch Aurora version (obsolete) — Splinter Review
This is the same except for removing the existing uievent and mouseevent quickstubs
Blocks: 822399
Whiteboard: [need review]
Attachment #736303 - Flags: review?(bugs)
Comment on attachment 736296 [details] [diff] [review]
Install WebIDL quickstubs for event interfaces as needed.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Unclear.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The tests are pretty clear.

Which older supported branches are affected by this flaw? Aurora 22.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Aurora backport is attached.

How likely is this patch to cause regressions; how much testing does it need?  Risk is medium; I'd like to get as much testing as I can.
Attachment #736296 - Flags: sec-approval?
Comment on attachment 736303 [details] [diff] [review]
Aurora version

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 822399 
User impact if declined: Invariant violation with unclear consequences
Testing completed (on m-c, etc.): Passes attached tests.
Risk to taking this patch (and alternatives if risky): Medium risk; this stuff is
  fiddly.  The main alternative is turning off WebIDL bindings for events on
  Aurora, but that may be riskier.
String or IDL/UUID changes made by this patch:  None.
Attachment #736303 - Flags: approval-mozilla-aurora?
> Olli, are there already some webidl events on Aurora 22?
Yes
(see the various blocking bugs on bug 776864)
Flags: needinfo?(bugs)
Comment on attachment 736296 [details] [diff] [review]
Install WebIDL quickstubs for event interfaces as needed.

sec-approval+.

Can you suggest a security rating for this issue?
Attachment #736296 - Flags: sec-approval? → sec-approval+
I have no idea, actually.  I'm not sure whether the lccx actually depends on the object being an XPConnect wrapper and if so what will go wrong in an opt build if it's not.
Attachment #736296 - Flags: review?(bugs) → review+
Attachment #736303 - Flags: review?(bugs) → review+
This was pushed in https://hg.mozilla.org/integration/mozilla-inbound/rev/8488f69f8f91 and then backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1f956ddfa7a4 because those base classes the patch uses are totally wrong.  I'll fix the patch.
Attachment #736296 - Attachment is obsolete: true
Attached patch Updated Aurora patch (obsolete) — Splinter Review
Attachment #736303 - Attachment is obsolete: true
Attachment #736303 - Flags: approval-mozilla-aurora?
Attachment #736866 - Flags: approval-mozilla-aurora?
Attachment #736865 - Flags: review?(bugs) → review+
We'll approve once this sticks on m-c for a couple of days.
So I pushed this to try, and there is one test "failure":

12:14:30 INFO - 9135 ERROR TEST-UNEXPECTED-PASS | /tests/dom/imptests/webapps/DOMCore/tests/approved/test_interfaces.html | Event interface: calling initEvent(DOMString,boolean,boolean) on new CustomEvent("foo") with too few arguments must throw TypeError

I'm removing the relevant line from dom/imptests/failures/webapps/DOMCore/tests/approved/test_interfaces.html.json before I push this.  Will post an Aurora patch that includes that too.
Attachment #736866 - Attachment is obsolete: true
Attachment #736866 - Flags: approval-mozilla-aurora?
Attachment #737073 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/4f911913dadf
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Attachment #737073 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [adv-main22-]
Keywords: regression
Group: core-security
You need to log in before you can comment on or make changes to this bug.