Closed
Bug 1296649
Opened 8 years ago
Closed 8 years ago
Crash [@ JSScript::code] or Crash [@ js::jit::ICEntry::pc] with use-after-free
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla52
People
(Reporter: decoder, Assigned: h4writer)
References
Details
(5 keywords, Whiteboard: [jsbugmon:][adv-main50+])
Crash Data
Attachments
(3 files)
2.44 KB,
text/plain
|
Details | |
28.13 KB,
patch
|
jandem
:
review+
ritu
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
18.09 KB,
patch
|
h4writer
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision fe895421dfbe (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --thread-count=2 --disable-oom-functions --baseline-eager --ion-offthread-compile=off --ion-pgo=on --ion-eager):
See attachment.
Backtrace:
received signal SIGSEGV, Segmentation fault.
JSScript::code (this=0xf2d68890) at js/src/jsscript.h:1345
#0 JSScript::code (this=0xf2d68890) at js/src/jsscript.h:1345
#1 JSScript::offsetToPC (offset=<optimized out>, this=0xf2d68890) at js/src/jsscript.h:1371
#2 js::jit::ICEntry::pc (script=0xf2d68890, this=<optimized out>) at js/src/jit/SharedIC.h:302
#3 js::jit::DoTypeMonitorFallback (cx=0xf7949000, payload=0x0, stub=0xf4232070, value=..., res=...) at js/src/jit/SharedIC.cpp:3987
#4 0xf7be51fb in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
eax 0x49494949 1229539657
ebx 0xf4232070 -199024528
ecx 0xfffd023c -196036
edx 0xf2d68890 -220821360
esi 0xf2d68890 -220821360
edi 0xf7949000 -141258752
ebp 0xfffd02f4 4294771444
esp 0xfffd0220 4294771232
eip 0x82d7433 <js::jit::DoTypeMonitorFallback(JSContext*, void*, js::jit::ICTypeMonitor_Fallback*, JS::HandleValue, JS::MutableHandleValue)+115>
=> 0x82d7433 <js::jit::DoTypeMonitorFallback(JSContext*, void*, js::jit::ICTypeMonitor_Fallback*, JS::HandleValue, JS::MutableHandleValue)+115>: mov 0x8(%eax),%edx
0x82d7436 <js::jit::DoTypeMonitorFallback(JSContext*, void*, js::jit::ICTypeMonitor_Fallback*, JS::HandleValue, JS::MutableHandleValue)+118>: lea 0x10(%eax,%edx,4),%eax
Marking s-s and sec-critical due to use-after-free pattern.
Reporter | ||
Comment 1•8 years ago
|
||
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
Comment 2•8 years ago
|
||
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Updated•8 years ago
|
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Comment hidden (obsolete) |
Testcase probably turned intermittent during the process of bisection, so please disregard comment 3.
Comment 5•8 years ago
|
||
Naveed, can you get someone to look at this?
Comment 7•8 years ago
|
||
Jan please take a look or assign.
Flags: needinfo?(nihsanullah) → needinfo?(jdemooij)
Comment 8•8 years ago
|
||
This has to do with shared stubs. We call DoGetPropGeneric from Ion, this ends up triggering a CGC that moves the script. Then we call the type monitor fallback stub, but we crash when we use its IonICEntry::script_ as it wasn't updated.
Hannes do you know if we have something to handle this?
Flags: needinfo?(jdemooij) → needinfo?(hv1989)
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #8)
> Hannes do you know if we have something to handle this?
This is from before JSScripts were movable. I'll look into a solution Thursday or Friday.
Assignee: nobody → hv1989
Updated•8 years ago
|
tracking-firefox51:
--- → +
Assignee | ||
Comment 10•8 years ago
|
||
The actual change is in SharedIC.cpp/h. I added a trace method to IonICEntry which is called to fix the JSScripts. This solves the issue.
At the same time I created BaselineICEntry. That way one cannot interchangeable use IonICEntry/ICEntry (since ICEntry does less in the trace method). I could have used a virtual functions, but that increases the size of ICEntry. Therefore I created BaselineICEntry/IonICEntry distinction. Both have a trace function, but the ICEntry hasn't. That way we can be sure the correct trace function is called.
Flags: needinfo?(hv1989)
Attachment #8792007 -
Flags: review?(jdemooij)
Comment 11•8 years ago
|
||
Comment on attachment 8792007 [details] [diff] [review]
Patch (nightly + aurora)
Review of attachment 8792007 [details] [diff] [review]:
-----------------------------------------------------------------
Great!
Attachment #8792007 -
Flags: review?(jdemooij) → review+
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8792007 [details] [diff] [review]
Patch (nightly + aurora)
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
No idea. It would definitely be intermittent and take a lot of time to get all the timings right. And even harder to be able to do something with it.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.
Which older supported branches are affected by this flaw?
FF48+
If not all supported branches, which bug introduced the flaw?
bug 1259180
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I didn't create them yet. The actual change is minimal. And the rest of the patch are mechanical changes.
How likely is this patch to cause regressions; how much testing does it need?
I think it is a quite easy/well understood/safe patch.
Attachment #8792007 -
Flags: sec-approval?
Comment 13•8 years ago
|
||
[Tracking Requested - why for this release]:
Crash Signature: [@ JSScript::code][js::jit::ICEntry::pc] → [@ JSScript::code][@ js::jit::ICEntry::pc]
status-firefox52:
--- → affected
status-firefox-esr45:
--- → unaffected
tracking-firefox50:
--- → ?
tracking-firefox52:
--- → ?
Comment 15•8 years ago
|
||
Comment on attachment 8792007 [details] [diff] [review]
Patch (nightly + aurora)
sec-approval+ for trunk. We should nominate patches for Beta and Aurora as well.
Attachment #8792007 -
Flags: sec-approval? → sec-approval+
Updated•8 years ago
|
Assignee | ||
Comment 16•8 years ago
|
||
Attachment #8798500 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8792007 -
Attachment description: Patch → Patch (nightly + aurora)
Assignee | ||
Updated•8 years ago
|
Priority: -- → P1
Assignee | ||
Comment 17•8 years ago
|
||
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8792007 [details] [diff] [review]
Patch (nightly + aurora)
Approval Request Comment
[Feature/regressing bug #]:
Bug 1259180
[User impact if declined]:
Potentially exploitable
[Describe test coverage new/current, TreeHerder]:
Landed a few hours ago.
[Risks and why]:
Quite low. It patch seems massive, but is mostly s/ICEntry/BaselineICEntry/. The logic change is quite small.
[String/UUID change made/needed]:
/
Attachment #8792007 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8798500 [details] [diff] [review]
Beta patch
Approval Request Comment
[Feature/regressing bug #]:
Bug 1259180
[User impact if declined]:
Potentially exploitable
[Describe test coverage new/current, TreeHerder]:
Landed a few hours ago.
[Risks and why]:
Quite low. It patch seems massive, but is mostly s/ICEntry/BaselineICEntry/. The logic change is quite small.
[String/UUID change made/needed]:
/
Attachment #8798500 -
Flags: approval-mozilla-beta?
Comment 20•8 years ago
|
||
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8792007 [details] [diff] [review]
Patch (nightly + aurora)
Sec-crit, Aurora51+
Attachment #8792007 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8798500 [details] [diff] [review]
Beta patch
Sec-crit, Beta50+
Attachment #8798500 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Updated•8 years ago
|
Comment 24•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx50
Updated•8 years ago
|
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main50+]
Updated•8 years ago
|
Comment 25•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx51
Updated•8 years ago
|
Keywords: csectype-uaf
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
•