1114569 - Assertion failure: ionRecovery_.empty(), at js/src/vm/Stack.cpp:1415

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 7b33ee7fd162 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --enable-debug, run with --fuzzing-safe --thread-count=2 --ion-offthread-compile=off --ion-eager):

try {
  setObjectMetadataCallback(true);
  function test() {
    function c()
      $ERROR('#1: 1e+01 === 10');    
    function gen() {}
    c(gen);
  }
  test();
} catch(exc2) {}
test();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000adb8c1 in js::jit::JitActivation::~JitActivation (this=0x7fffffffcce0, __in_chrg=<optimized out>) at js/src/vm/Stack.cpp:1415
1415	    MOZ_ASSERT(ionRecovery_.empty());
#0  0x0000000000adb8c1 in js::jit::JitActivation::~JitActivation (this=0x7fffffffcce0, __in_chrg=<optimized out>) at js/src/vm/Stack.cpp:1415
#1  0x000000000073da40 in EnterIon (data=..., cx=0x1a13590) at js/src/jit/Ion.cpp:2450
#2  js::jit::IonCannon (cx=0x1a13590, state=...) at js/src/jit/Ion.cpp:2530
#3  0x0000000000a6f87b in js::RunScript (cx=0x1a13590, state=...) at js/src/vm/Interpreter.cpp:412
#4  0x0000000000a70525 in js::Invoke (cx=0x1a13590, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:501
#5  0x0000000000a7181d in js::Invoke (cx=0x1a13590, thisv=..., fval=..., argc=<optimized out>, argv=0x7fffffffdb08, rval=JSVAL_VOID) at js/src/vm/Interpreter.cpp:538
#6  0x00000000006a86bf in js::jit::DoCallFallback (cx=0x7fffffffdab8, frame=0x7fffffffdb40, stub_=<optimized out>, argc=0, vp=0x7fffffffdaf8, res=JSVAL_VOID) at js/src/jit/BaselineIC.cpp:9435
#7  0x00007ffff7e368fd in ?? ()
[...]
rax	0x0	0
rbx	0x7fffffffcce0	140737488342240
rcx	0x7ffff6cb792d	140737333917997
rdx	0x0	0
rsi	0x7ffff6f8baa0	140737336883872
rdi	0x7ffff6f8a180	140737336877440
rbp	0x7fffffffcc10	140737488342032
rsp	0x7fffffffcbe0	140737488341984
r8	0x7ffff7fe8740	140737354041152
r9	0x72746e65632d616c	8247338199356891500
r10	0x7fffffffc970	140737488341360
r11	0x7ffff6c3fc90	140737333427344
r12	0x7fffffffce90	140737488342672
r13	0x7fffffffd000	140737488343040
r14	0x1b00cc0	28314816
r15	0x1a13598	27342232
rip	0xadb8c1 <js::jit::JitActivation::~JitActivation()+305>
=> 0xadb8c1 <js::jit::JitActivation::~JitActivation()+305>:	movl   $0x7b,0x0
   0xadb8cc <js::jit::JitActivation::~JitActivation()+316>:	callq  0x4049c0 <abort@plt>

Comment 1

[Nicolas B. Pierron [:nbp]]

I don't know yet, but I can bet this bug is also related to Bug 1073033 :P

Comment 2

[Nicolas B. Pierron [:nbp]]

My blind guess is that this test case is investigating the stack, and creates a RecoverInstruction vector, but the vector is not used for any bailout as we are throwing an exception. Thus we fail in the destructor of the JitActivation with the sanity check which ensure that all recover instruction vector were used by their frames.

I am flagging this bug as sec-critical as the non-consumed RInstructionResult vector which are registered on the JitActivation are mapped with their frame pointer.  So one person can build an attack by following these steps:
 - Make a frame aligned on /fp_x/ which has a non-escaped lambda & non-escaped scope-chain.
 - The lambda is called, and makes a call which inspect the stack.
 - Then an error is used to skip the use of the RInstructionVector.
 - Back in baseline, call another function, which is running under ion and which has a frame aligned on /fp_x/.
 - Then this frame is causing a bailout, and re-use the RInstructionVector which was not popped previously.

This can be used for injecting different Value to get these interpreted differently, such as injecting pointer.  This can also read uninitialized memory if the RInstructionResult vector is smaller than the one from the previous frame.

 - which are being inspected by some code deeper, and then throw.  The throw will skip the recovery of the frame, and thus will return into baseline.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: [Inbound/Aurora] Discard RInstructionResults while handling exceptions.

This patch pop the RInstructionResult vector out of the JitActivation when
an exception is handled. (see comment 2)

Comment 4

[Nicolas B. Pierron [:nbp]]

The following testcase crashes on mozilla-central revision 7b33ee7fd162 (build with --enable-optimize --enable-debug, run with --ion-offthread-compile=off --ion-eager):

try {
  function test(a) {
    a = 1 + (a | 0);
    function c() {
      test.arguments[0];
      $ERROR('#1: 1e+01 === 10');
    }
    c();
  }
  test();
} catch(exc2) {}
test();


As the above example show, this issue is not restricted to the optimization added in Bug 1073033.  I am afraid this issue might be as old as the introduction of the RInstructionResult on the JitActivation (Bug 1062869).

I am running a bisect to verify if Fx35 is affected.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> The following testcase crashes on mozilla-central revision 7b33ee7fd162

Hum … I might have done something wrong because I cannot reproduce this crash with the above test case :/

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8540245 [details] [diff] [review]
[Inbound/Aurora] Discard RInstructionResults while handling exceptions.

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

The patch highlight that an exploit should make use of exceptions, and it should use recover instructions.  Which is a big part of what is described in comment 2.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The test is included in the patch so far. (waiting to know if we decide to backport the patch or not, knowing that the test case only failed because of Bug 1073033)

> Which older supported branches are affected by this flaw?

I am not sure if older versions are affected, and think that fuzzers might not have found similar issues before, then this might be since Bug 1062869.

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

Bug 1062869

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

Backports should be easy to make, only one function got renamed since Bug 1062869.

> How likely is this patch to cause regressions; how much testing does it need?

Unlikely, the function which is called is supposed to work well even if the element does not exists in the vector.

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Comment on attachment 8540245 [details] [diff] [review]
> Discard RInstructionResults while handling exceptions.
> 
> > [Security approval request comment]
> > How easily could an exploit be constructed based on the patch?
> 
> The patch highlight that an exploit should make use of exceptions, and it
> should use recover instructions.  Which is a big part of what is described
> in comment 2.
> 
> > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
> 
> The test is included in the patch so far. (waiting to know if we decide to
> backport the patch or not, knowing that the test case only failed because of
> Bug 1073033)
> 
> > Which older supported branches are affected by this flaw?
> 
> I am not sure if older versions are affected, and think that fuzzers might
> not have found similar issues before, then this might be since Bug 1062869.

s/and think/and if we think/

> 
> > If not all supported branches, which bug introduced the flaw?
> 
> Bug 1062869
> 
> > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
> 
> Backports should be easy to make, only one function got renamed since Bug
> 1062869.
> 
> > How likely is this patch to cause regressions; how much testing does it need?
> 
> Unlikely, the function which is called is supposed to work well even if the
> element does not exists in the vector.

Comment 8

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8540245 [details] [diff] [review]
[Inbound/Aurora] Discard RInstructionResults while handling exceptions.

Sec-approval+ for trunk. Please don't check in a test yet though.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: [Beta] Discard RInstructionResults while handling exceptions.

Comment 11

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4c70d12ab91

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b4c70d12ab91

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 14

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8543293 [details] [diff] [review]
[Beta] Discard RInstructionResults while handling exceptions.

Approval Request Comment
[Feature/regressing bug #]:
Bug 1062869

[User impact if declined]:
Might cause crashes.

[Describe test coverage new/current, TBPL]:
landed on inbound.

[Risks and why]: 
This can be used to forge use-after-free of JS memory. (see comment 2)

[String/UUID change made/needed]:
N/A

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8540245 [details] [diff] [review]
[Inbound/Aurora] Discard RInstructionResults while handling exceptions.

Approval Request Comment
(see comment 14)

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Checked in IRC with Al and we can wait on this for 35 since we're about to build RC and stability is a concern. We should get it uplifted into Aurora (36) though and I've marked it tracking to help ensure that happens in time.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8543293 [details] [diff] [review]
[Beta] Discard RInstructionResults while handling exceptions.

See comment, we'll take the Aurora patch.

Comment 18

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ab03636659f