Closed Bug 1153688 Opened 5 years ago Closed 5 years ago
Type confusion between object and symbol in XPCVariant
Assertion failure: val.isObject() (invalid type of jsval!), at js/xpconnect/src/XPCVariant.cpp:301
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.
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.
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.
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.
Summary: "Assertion failure: val.isObject() (invalid type of jsval!)" with Symbol → Type confusion between object and symbol in XPCVariant
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.
Comment on attachment 8593079 [details] [diff] [review] v1 sec-approval+ for trunk and I gave Aurora and Beta approvals as well.
You need to log in before you can comment on or make changes to this bug.