Crash [@ js::jit::JitFrameIterator::script] with OOM and invalid pointer

RESOLVED FIXED in Firefox 50

Status

()

--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla52
x86_64
Linux
crash, regression, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox-esr45 unaffected, firefox50+ fixed, firefox51+ fixed, firefox52 fixed)

Details

(Whiteboard: [jsbugmon:ignore][post-critsmash-triage], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
The following testcase crashes on mozilla-central revision e78975b53563 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2):

See attachment.



Backtrace:

 received signal SIGSEGV, Segmentation fault.
js::jit::JitFrameIterator::script (this=0x7fffffffad10) at js/src/jit/JitFrames.cpp:213
#0  js::jit::JitFrameIterator::script (this=0x7fffffffad10) at js/src/jit/JitFrames.cpp:213
#1  0x00000000006b4a92 in js::jit::JitFrameIterator::checkInvalidation (this=this@entry=0x7fffffffad10, ionScriptOut=ionScriptOut@entry=0x7fffffffa5c0) at js/src/jit/JitFrames.cpp:149
#2  0x00000000006b626b in js::jit::JitFrameIterator::ionScript (this=0x7fffffffad10) at js/src/jit/JitFrames.cpp:2208
#3  0x00000000006b68c1 in js::jit::JitFrameIterator::safepoint (this=0x7fffffffad10) at js/src/jit/JitFrames.cpp:2226
#4  0x00000000006b6928 in js::jit::JitFrameIterator::machineState (this=this@entry=0x7fffffffad10) at js/src/jit/JitFrames.cpp:297
#5  0x00000000006bc1e4 in js::jit::InlineFrameIterator::resetOn (this=0x7fffffffae40, iter=0x7fffffffad10) at js/src/jit/JitFrames.cpp:2290
#6  0x00000000006c96a3 in js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:784
#7  0x00007ffff7fe6646 in ?? ()
[...]
#12 0x0000000000000000 in ?? ()
rax	0x807ffeffb240	141287227372096
rbx	0x7fffffffad10	140737488334096
rcx	0x30	48
rdx	0x7fffffffb1d0	140737488335312
rsi	0x7fffffffa5c0	140737488332224
rdi	0x7fffffffad10	140737488334096
rbp	0x7fffffffa590	140737488332176
rsp	0x7fffffffa590	140737488332176
r8	0x30	48
r9	0x7ffff24fffa0	140737258717088
r10	0x2	2
r11	0x7ffff7e9bdf8	140737352678904
r12	0x7fffffffa5c0	140737488332224
r13	0x7fffffffa8c0	140737488332992
r14	0x7fffffffad10	140737488334096
r15	0x7fffffffad10	140737488334096
rip	0x6b49f9 <js::jit::JitFrameIterator::script() const+25>
=> 0x6b49f9 <js::jit::JitFrameIterator::script() const+25>:	mov    0x10(%rax),%rdi
   0x6b49fd <js::jit::JitFrameIterator::script() const+29>:	callq  0x6b2030 <js::jit::ScriptFromCalleeToken(js::jit::CalleeToken)>


The testcase here is intermittent. Sometimes it crashes after only 1-2 seconds, sometimes it runs for 30 seconds before crashing. The memory access looks sec-critical, marking as such.
(Reporter)

Comment 1

2 years ago
Created attachment 8778950 [details]
Testcase
Naveed can you help find someone to work on this sec-critical bug?
Flags: needinfo?(nihsanullah)
David can you help out here ? Maybe you can help find an owner to investigate this since it is sec-critical and may affect beta and other versions.
status-firefox48: --- → ?
status-firefox49: --- → ?
status-firefox51: --- → ?
tracking-firefox50: --- → +
Flags: needinfo?(dbolter)
Jandem, any thoughts?
Flags: needinfo?(dbolter) → needinfo?(jdemooij)
(Assignee)

Comment 5

2 years ago
I'll take a look tomorrow.
Flags: needinfo?(nihsanullah)
(Assignee)

Comment 6

2 years ago
Created attachment 8783926 [details] [diff] [review]
Patch

Oops, regression from bug 1272598. We shouldn't jump to masm.exceptionLabel() when ArgumentsObject::finishForIon fails, because we didn't enter an exit frame.

This patch changes the code to just fall back to the slow path.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8783926 - Flags: review?(nicolas.b.pierron)
(Assignee)

Updated

2 years ago
status-firefox48: ? → unaffected
status-firefox49: ? → unaffected
status-firefox51: ? → affected
status-firefox-esr45: --- → unaffected
tracking-firefox51: --- → ?
Attachment #8783926 - Flags: review?(nicolas.b.pierron) → review+
tracking-firefox51: ? → +
Jan, can we land this? Is this blocked on something to request sec-approval and uplift?
Flags: needinfo?(jdemooij)
It only affects 50 and 51. If someone asked for sec-approval+ to land on trunk and Aurora approval, I'd give it.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1301608
(Assignee)

Comment 10

2 years ago
Comment on attachment 8783926 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not trivial but not that difficult...

> 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?
Aurora, beta (50+).

> If not all supported branches, which bug introduced the flaw?
Bug 1272598.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply or be easy.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Flags: needinfo?(jdemooij)
Attachment #8783926 - Flags: sec-approval?
(Assignee)

Comment 11

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #10)
> > Which older supported branches are affected by this flaw?
> Aurora, beta (50+).

Only aurora, actually, since the merge was moved to next week.
Comment on attachment 8783926 [details] [diff] [review]
Patch

sec-approval+ for trunk.
Please request Aurora approval now so we can get this onto 50 before it becomes Beta as well.
Attachment #8783926 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 14

2 years ago
Comment on attachment 8783926 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1272598.
[User impact if declined]: Crashes on OOM, potential security bugs.
[Describe test coverage new/current, TreeHerder]: Fixes the fuzz test.
[Risks and why]: Low risk. Only affects an OOM case and hitting that here should be unlikely.
[String/UUID change made/needed]: None.
Attachment #8783926 - Flags: approval-mozilla-beta?
Attachment #8783926 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d29c970e4d7d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8783926 [details] [diff] [review]
Patch

Sec-crit issue, Aurora51+, Beta50+
Attachment #8783926 - Flags: approval-mozilla-beta?
Attachment #8783926 - Flags: approval-mozilla-beta+
Attachment #8783926 - Flags: approval-mozilla-aurora?
Attachment #8783926 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1297142
Group: javascript-core-security → core-security-release
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1299103
Flags: qe-verify-
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.