Closed
Bug 1263549
Opened 8 years ago
Closed 8 years ago
Inlined RegExpPrototypeOptimizable and RegExpInstanceOptimizable reads from wrong address.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
Details
(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main48-])
Attachments
(1 file)
2.42 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
Related to bug 1263532. In inlined RegExpPrototypeOptimizable, optimizable shape address is calculated with following: > masm.loadJSContext(temp); > masm.loadPtr(Address(temp, JSContext::offsetOfCompartment()), temp); > masm.loadPtr(Address(temp, JSCompartment::offsetOfRegExps()), temp); > masm.loadPtr(Address(temp, RegExpCompartment::offsetOfOptimizableRegExpPrototypeShape()), > temp); But C++ version is following: > Shape* shape = cx->compartment()->regExps.getOptimizableRegExpPrototypeShape(); `regExps` member is *not* a pointer: > js::RegExpCompartment regExps; thus the inlined version should be following: > masm.loadJSContext(temp); > masm.loadPtr(Address(temp, JSContext::offsetOfCompartment()), temp); > masm.loadPtr(Address(temp, > JSCompartment::offsetOfRegExps() + > RegExpCompartment::offsetOfOptimizableRegExpInstanceShape()), temp); Same for RegExpInstanceOptimizable. The loaded wrong value is used only for comparison, and discarded instantly, so this won't be exploitable, but marking s-s just in case. This means that inlined version always jumps to OOL. fixing this might help performance issues.
Adding this to javascript-security-sensitive group. (This was previously, and still is, "Security-Sensitive Core Bug")
Group: javascript-core-security
Assignee | ||
Comment 3•8 years ago
|
||
This doesn't introduce any visible behavior change, so no testcase attached.
Assignee: nobody → arai.unmht
Attachment #8739900 -
Flags: review?(hv1989)
Updated•8 years ago
|
Group: core-security
Comment 4•8 years ago
|
||
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2) > Adding this to javascript-security-sensitive group. (This was previously, > and still is, "Security-Sensitive Core Bug") "double-grouping" is worse than leaving it alone. Moving it to the Javascript bug group helps the javascript folks see it, but if you leave it in the default group also then only the INTERSECTION of the JS team and the triage team can see it.
(In reply to Daniel Veditz [:dveditz] from comment #4) > "double-grouping" is worse than leaving it alone. Moving it to the > Javascript bug group helps the javascript folks see it, but if you leave it > in the default group also then only the INTERSECTION of the JS team and the > triage team can see it. I didn't have the permissions to remove it from the "core-security" group.
Comment 6•8 years ago
|
||
Comment on attachment 8739900 [details] [diff] [review] Fix inlined RegExpPrototypeOptimizable and RegExpInstanceOptimizable. Review of attachment 8739900 [details] [diff] [review]: ----------------------------------------------------------------- Good find! ::: js/src/jit/CodeGenerator.cpp @@ +2151,5 @@ > masm.loadJSContext(temp); > masm.loadPtr(Address(temp, JSContext::offsetOfCompartment()), temp); > + masm.loadPtr(Address(temp, > + JSCompartment::offsetOfRegExps() + > + RegExpCompartment::offsetOfOptimizableRegExpPrototypeShape()), temp); can you create a variable for the offset so this line would fit on one line? something like: > size_t offsetOfOptimizableShapes = JSCompartment::offsetOfRegExps() + > RegExpCompartment::offsetOfOptimizableRegExpPrototypeShape()); > masm.loadPtr(Address(temp, offsetOfOptimizableShapes, temp); @@ +2220,5 @@ > masm.loadJSContext(temp); > masm.loadPtr(Address(temp, JSContext::offsetOfCompartment()), temp); > + masm.loadPtr(Address(temp, > + JSCompartment::offsetOfRegExps() + > + RegExpCompartment::offsetOfOptimizableRegExpInstanceShape()), temp); same as above. Please create local variable for the offset.
Attachment #8739900 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 7•8 years ago
|
||
With this bug, it can load a single pointer-sized value from wrong address, but the value is used only for comparing with other pointer value (the Shape of JS object). The result of the comparison is also not observable from script, so this should be sec-other (Bugs that may not be exploitable security issues)
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/00cb5de040be36971fa941cb52fcc679fe67878d Bug 1263549 - Fix inlined RegExpPrototypeOptimizable and RegExpInstanceOptimizable. r=h4writer
Comment 9•8 years ago
|
||
Small improvement on octane-regexp! https://arewefastyet.com/#machine=29&view=single&suite=octane&subtest=RegExp&start=1460471175&end=1460530464 https://arewefastyet.com/#machine=28&view=single&suite=octane&subtest=RegExp&start=1460471175&end=1460530464
Comment 10•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/00cb5de040be
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main48-]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•