Closed Bug 1296649 Opened 4 years ago Closed 4 years ago

Crash [@ JSScript::code] or Crash [@ js::jit::ICEntry::pc] with use-after-free

Categories

(Core :: JavaScript Engine, defect, P1)

x86
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
firefox49 --- wontfix
firefox-esr45 --- unaffected
firefox50 + verified
firefox51 + verified
firefox52 + verified

People

(Reporter: decoder, Assigned: h4writer)

References

Details

(6 keywords, Whiteboard: [jsbugmon:][adv-main50+])

Crash Data

Attachments

(3 files)

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.
Attached file Testcase
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Testcase probably turned intermittent during the process of bisection, so please disregard comment 3.
Naveed, can you get someone to look at this?
A few weeks later, I will ni? Naveed... :-)
Flags: needinfo?(nihsanullah)
Jan please take a look or assign.
Flags: needinfo?(nihsanullah) → needinfo?(jdemooij)
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)
(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
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 on attachment 8792007 [details] [diff] [review]
Patch (nightly + aurora)

Review of attachment 8792007 [details] [diff] [review]:
-----------------------------------------------------------------

Great!
Attachment #8792007 - Flags: review?(jdemooij) → review+
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?
[Tracking Requested - why for this release]:
Crash Signature: [@ JSScript::code][js::jit::ICEntry::pc] → [@ JSScript::code][@ js::jit::ICEntry::pc]
Tracking 52+ for this sec-critical crash.
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+
Attached patch Beta patchSplinter Review
Attachment #8798500 - Flags: review+
Attachment #8792007 - Attachment description: Patch → Patch (nightly + aurora)
Priority: -- → P1
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?
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?
https://hg.mozilla.org/mozilla-central/rev/f7baf71da7ef
Status: NEW → RESOLVED
Closed: 4 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+
Group: javascript-core-security → core-security-release
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx50
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main50+]
JSBugMon: This bug has been automatically verified fixed on Fx51
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.