Closed
Bug 1153688
Opened 9 years ago
Closed 9 years ago
Type confusion between object and symbol in XPCVariant
Categories
(Core :: XPConnect, defect)
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)
Details
(Keywords: assertion, sec-critical, testcase, Whiteboard: [adv-main38+])
Attachments
(3 files)
79 bytes,
text/html
|
Details | |
3.43 KB,
text/plain
|
Details | |
1.57 KB,
patch
|
bholley
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
Assertion failure: val.isObject() (invalid type of jsval!), at js/xpconnect/src/XPCVariant.cpp:301
Reporter | ||
Comment 1•9 years ago
|
||
Comment 2•9 years ago
|
||
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).
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
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...
Comment 6•9 years ago
|
||
What are the security implications of this?
Comment 7•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 8•9 years ago
|
||
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.
Comment 9•9 years ago
|
||
Thanks.
Keywords: sec-critical
Summary: "Assertion failure: val.isObject() (invalid type of jsval!)" with Symbol → Type confusion between object and symbol in XPCVariant
Assignee | ||
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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+
Assignee | ||
Comment 12•9 years ago
|
||
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)
Assignee | ||
Comment 13•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Updated•9 years ago
|
status-firefox37:
--- → wontfix
status-firefox-esr31:
--- → unaffected
tracking-firefox38:
--- → +
tracking-firefox39:
--- → +
tracking-firefox40:
--- → +
Comment 14•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8593079 -
Flags: approval-mozilla-b2g37?
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/75ae5ac861f2
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/75ae5ac861f2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 17•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f1b9d7872e19
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → fixed
status-firefox-esr38:
--- → affected
Flags: in-testsuite?
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(jruderman)
Updated•9 years ago
|
Whiteboard: [adv-main38+]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•