Closed Bug 1153688 Opened 5 years ago Closed 5 years ago

Type confusion between object and symbol in XPCVariant

Categories

(Core :: XPConnect, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox37 --- wontfix
firefox38 + fixed
firefox39 + fixed
firefox40 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: jruderman, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-critical, testcase, Whiteboard: [adv-main38+])

Attachments

(3 files)

Attached file testcase
Assertion failure: val.isObject() (invalid type of jsval!), at js/xpconnect/src/XPCVariant.cpp:301
Attached file stack
XPCVariant doesn't support Symbols; as a stopgap, we could throw when trying to convert one, but longer term we'll need to support them properly (at least for CustomEvent; not sure if we use them anywhere else in the DOM).
Hmm.  So what should the C++-side reflection of a symbol look like?

Also, can we just change CustomEvent to storing a Value instead of a variant?  That seems like it would work pretty nicely in this particular case.
Flags: needinfo?(bugs)
Would need to then use some more JS API magic in CustomEvent implementation to keep the C++ API still
somewhat easy to use.

Or, let CustomEvent to hold to variant or JS::Value, so that C++ would use the Variant version of methods, and if JS::Value was actually stored, prevent Symbol->Variant conversion explicitly or something - a bit ugly.
Flags: needinfo?(bugs)
Which C++ API are we worried about?

The two GetDetail() callers I see in our C++ are in GetDetail and they expect to get a property bag.  More on this below.

We do have some places that InitCustomEvent with a variant; we could either store the nsIVariant and convert to a JS::Value lazily GetDetail or we could make those consumers use ToJSValue.  Looking at them:

1) nsGlobalWindow::DispatchResizeEvent starts out with a Value anyway.
2) DispatchCustomDOMEvent in BrowserElementParent starts out with a Value anyway.
3) nsXMLPrettyPrinter::PrettyPrint has a DocumentFragment it's trying to put in there.
4) nsTreeBodyFrame::FireRowCountChangedEvent and nsTreeBodyFrame::FireInvalidateEvent
   both put a propertybag in the detail.  This is the stuff the a11y code listens to.

So apart from that XUL tree stuff, using a Value here would be pretty straightforward...
What are the security implications of this?
The code goes on to treat the value as a JSObject*.  So now it's got a Symbol* that it thinks is a JSObject* and it starts operating on the latter.  My gut instinct is that this is sec-critical, honestly, since JSObject* basically has a vtable (via the Class), but I don't have a good feel for how much control an attacker might have over where the Symbol* points.
Assignee: nobody → bugs
Not sure if we should change CustomEvent or make Variant to support symbols... I think probably latter so that whoever uses Variants doesn't need to think about Symbols.
Thanks.
Keywords: sec-critical
Summary: "Assertion failure: val.isObject() (invalid type of jsval!)" with Symbol → Type confusion between object and symbol in XPCVariant
Attached patch v1Splinter Review
This should be enough. Don't even try to expose Symbols on C++ side, and let 
XPCVariant::VariantDataToJS to just access the jsval.
And in case of arrays, make sure array stores variants.
Not very efficient, but I don't think we care in this case.
Attachment #8593079 - Flags: review?(bobbyholley)
Comment on attachment 8593079 [details] [diff] [review]
v1

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

I guess so. I wish we could handle these better.
Attachment #8593079 - Flags: review?(bobbyholley) → review+
Jesse, want to fuzz test with the patch? I wouldn't be too surprised if Symbols cause issues also somewhere else than in XPCVariant.
Flags: needinfo?(jruderman)
Comment on attachment 8593079 [details] [diff] [review]
v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): whenever Symbol was implemented in JS engine,
based on https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol FF36
User impact if declined: use of jsval as object when it actually isn't such, so unexpected results.
Testing completed: local testing
Risk to taking this patch (and alternatives if risky): This is the safest patch I can think of, shouldn't be risky.
String or UUID changes made by this patch: NA

The commit message would be

"Bug 1153688, treat JS Symbol as void on C++ side of Variant, r=bholley"

which kind of hints still what the issue is. And the patch is rather obvious.
Attachment #8593079 - Flags: sec-approval?
Attachment #8593079 - Flags: approval-mozilla-beta?
Attachment #8593079 - Flags: approval-mozilla-b2g37?
Attachment #8593079 - Flags: approval-mozilla-aurora?
Comment on attachment 8593079 [details] [diff] [review]
v1

sec-approval+ for trunk and I gave Aurora and Beta approvals as well.
Attachment #8593079 - Flags: sec-approval?
Attachment #8593079 - Flags: sec-approval+
Attachment #8593079 - Flags: approval-mozilla-beta?
Attachment #8593079 - Flags: approval-mozilla-beta+
Attachment #8593079 - Flags: approval-mozilla-aurora?
Attachment #8593079 - Flags: approval-mozilla-aurora+
Attachment #8593079 - Flags: approval-mozilla-b2g37?
https://hg.mozilla.org/mozilla-central/rev/75ae5ac861f2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Flags: needinfo?(jruderman)
Whiteboard: [adv-main38+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.