Closed
Bug 1153688
Opened 10 years ago
Closed 10 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•10 years ago
|
||
Comment 2•10 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•10 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•10 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•10 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•10 years ago
|
||
What are the security implications of this?
Comment 7•10 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•10 years ago
|
Assignee: nobody → bugs
| Assignee | ||
Comment 8•10 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•10 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•10 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•10 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•10 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•10 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•10 years ago
|
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Updated•10 years ago
|
status-firefox37:
--- → wontfix
status-firefox-esr31:
--- → unaffected
tracking-firefox38:
--- → +
tracking-firefox39:
--- → +
tracking-firefox40:
--- → +
Comment 14•10 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•10 years ago
|
Attachment #8593079 -
Flags: approval-mozilla-b2g37?
| Assignee | ||
Comment 15•10 years ago
|
||
Comment 16•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 17•10 years ago
|
||
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•10 years ago
|
Flags: needinfo?(jruderman)
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
Updated•10 years ago
|
Whiteboard: [adv-main38+]
Updated•10 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•