Crash [@ __pthread_kill] with [@ free] on the stack

RESOLVED FIXED in Firefox -esr45

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gkw, Assigned: jandem)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla52
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr4550+ fixed, firefox50+ fixed, firefox51+ fixed, firefox52+ fixed)

Details

(Whiteboard: [jsbugmon:ignore][post-critsmash-triage][adv-main50+][adv-esr45.5+], crash signature)

Attachments

(4 attachments)

Reporter

Description

3 years ago
The following testcase crashes on mozilla-central revision da986c9f1f72 (build with --enable-debug --enable-more-deterministic --32, run with --fuzzing-safe --no-threads --ion-eager --no-ggc):

function h(f, m) {
    for (var j = 0; j < 9; ++j) {
        f(m[j]);
    }
}
function g() {
    new new Function("arguments.callee.arguments")(1);
}
h(g, []);


Backtrace:

0   libsystem_kernel.dylib        	0x9de41572 __pthread_kill + 10
1   libsystem_pthread.dylib       	0x964c8654 pthread_kill + 101
2   libsystem_c.dylib             	0x96cd6c38 abort + 156
3   libsystem_malloc.dylib        	0x92ddcf31 szone_error + 457
4   libsystem_malloc.dylib        	0x92ddcf65 free_list_checksum_botch + 50
5   libsystem_malloc.dylib        	0x92dd4328 tiny_free_list_remove_ptr + 123
6   libsystem_malloc.dylib        	0x92dd2c11 szone_free_definite_size + 1682
7   libsystem_malloc.dylib        	0x92dd220e free + 301
8   js-dbg-32-dm-clang-darwin-da986c9f1f72	0x00364953 js::jit::IonCannon(JSContext*, js::RunState&) + 995 (Ion.cpp:378)
/snip

For detailed crash information, see attachment.

Setting s-s because free is on the stack, may involve some form of memory error.
Reporter

Comment 2

3 years ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/c97757afd190
user:        Jan de Mooij
date:        Sat Jun 11 15:01:22 2016 +0200
summary:     Bug 1272598 part 4 - Remove script and dataBytes from ArgumentsData. r=luke

Jan, is bug 1272598 a likely regressor?
Blocks: 1272598
Flags: needinfo?(jdemooij)
Reporter

Comment 3

3 years ago
Note that this can be fairly intermittent.
Whiteboard: [jsbugmon:update] → [jsbugmon:ignore]
Assignee

Comment 4

3 years ago
Gary, I'm unable to reproduce this on OS X with the revision and flags in comment 0, after trying at least 30 times. Do you have a different testcase, maybe a less reduced one?

If not I can try this with rr on Linux (next week, when I'm home).
Reporter

Comment 5

3 years ago
Try with a configuration similar to:

# Full configuration command with needed environment variables is:
# LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin14.5.0 --disable-jemalloc --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests
Reporter

Comment 6

3 years ago
I also saved a local coredump (150MB) that you can take a look should that be necessary, let me know.
Assignee

Comment 7

3 years ago
OK I can reproduce this now. (--disable-jemalloc probably did the trick as malloc is reporting a UAF or so on stderr.) Investigating...
Assignee

Comment 8

3 years ago
OK so InlineFrameIterator::readFrameArgsAndLocals is calling argOp for new.target (if we're constructing), resulting in us overflowing the space we allocated for the arguments data. Treating new.target like an argument here seems like a footgun. Bug 1272598 probably exposed this but the actual bug predates that (maybe it's from bug 1141865).

Good find, Gary, this is pretty bad.
Assignee

Updated

3 years ago
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Assignee

Comment 9

3 years ago
Posted patch PatchSplinter Review
This fixes the issue in comment 8. readFrameArgsAndLocals now returns new.target separately (like |this|), instead of calling ArgOp for it. I also had to change RematerializedFrame to stop storing new.target in slots_.
Flags: needinfo?(jdemooij)
Attachment #8799821 - Flags: review?(shu)
Attachment #8799821 - Flags: review?(efaustbmo)

Updated

3 years ago
Attachment #8799821 - Flags: review?(shu) → review+
Comment on attachment 8799821 [details] [diff] [review]
Patch

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

::: js/src/jit/JitFrameIterator.h
@@ +717,5 @@
>              s.skip();
>  
> +        if (newTarget) {
> +            // For now, only support reading new.target when we are reading
> +            // overflown arguments.

This is pretty scary. Are all usecases OK with this?
Attachment #8799821 - Flags: review?(efaustbmo) → review+
Tracking 52+ for this issue = Comment 8 mentions the severity.
Assignee

Comment 12

3 years ago
(In reply to Eric Faust [:efaust] from comment #10)
> This is pretty scary. Are all usecases OK with this?

Yeah, ReadFrame_Formals is not used anywhere, so it seemed easiest to add this assert (actually it seems we only need ReadFrame_Actuals, as ReadFrame_Overflown is only used for some debugging code that could be simplified...).
Assignee

Comment 13

3 years ago
Comment on attachment 8799821 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
This patch lets an attacker write an ObjectValue (8 bytes) out of bounds. It's probably not trivial to exploit, but it could be used to corrupt memory maybe.

> 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?
All.

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

> How likely is this patch to cause regressions; how much testing does it need?
Not very likely, a few Nightlies + fuzzing during that time should be sufficient.
Attachment #8799821 - Flags: sec-approval?
Assignee

Comment 14

3 years ago
(In reply to Jan de Mooij [:jandem] from comment #13)
> This patch lets an attacker write an ObjectValue (8 bytes) out of bounds.

Er, this *bug*, obviously...
I need release management to weigh in here on whether we could take this on beta and/or ESR45 before I give a sec-approval. We'd want to take this on branches immediately after taking this on trunk.
Flags: needinfo?(sledru)
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
(In reply to Al Billings [:abillings] from comment #15)
> I need release management to weigh in here on whether we could take this on
> beta and/or ESR45 before I give a sec-approval. We'd want to take this on
> branches immediately after taking this on trunk.

This is a sec-high and this meets the Beta/ESR bar. It would be nice if it can land today/early tomm so it gets included in 50.0b9 build.
Flags: needinfo?(rkothari)
Yes, happy to take this in esr45 as well.
Flags: needinfo?(lhenry)
Comment on attachment 8799821 [details] [diff] [review]
Patch

sec-approval+ for trunk. Please make branch patches as well.
Attachment #8799821 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(sledru)
Assignee

Comment 20

3 years ago
Comment on attachment 8799821 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: ES6 classes, enabled in 45.
[User impact if declined]: Sec issues, crashes.
[Describe test coverage new/current, TreeHerder]: Fixes the fuzz test.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.
Attachment #8799821 - Flags: approval-mozilla-esr45?
Attachment #8799821 - Flags: approval-mozilla-beta?
Attachment #8799821 - Flags: approval-mozilla-aurora?
Assignee

Comment 21

3 years ago
Assignee

Comment 22

3 years ago
Assignee

Comment 23

3 years ago
(In reply to Jan de Mooij [:jandem] from comment #20)
> [Feature/regressing bug #]: ES6 classes, enabled in 45.

Actually, this is a regression from new.target, it landed before 45.
https://hg.mozilla.org/mozilla-central/rev/3428128df117
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Group: javascript-core-security → core-security-release
Comment on attachment 8799821 [details] [diff] [review]
Patch

Sec-high, Aurora51+, Beta50+, ESR45+
Attachment #8799821 - Flags: approval-mozilla-esr45?
Attachment #8799821 - Flags: approval-mozilla-esr45+
Attachment #8799821 - Flags: approval-mozilla-beta?
Attachment #8799821 - Flags: approval-mozilla-beta+
Attachment #8799821 - Flags: approval-mozilla-aurora?
Attachment #8799821 - Flags: approval-mozilla-aurora+
I'm hitting conflicts trying to apply the beta patch.
Flags: needinfo?(jdemooij)
Assignee

Comment 28

3 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/1796b06d333cd97b7aec26d41dca1d5d2b348f78

Sorry about that. This file ~never changes, and now two patches touched the same lines. Grmbl.
Flags: needinfo?(jdemooij)
Flags: qe-verify-
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][post-critsmash-triage]
Whiteboard: [jsbugmon:ignore][post-critsmash-triage] → [jsbugmon:ignore][post-critsmash-triage][adv-main50+][adv-esr45.5+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.