1520827 - Constant pools can be inserted in the middle of Jump tables

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Nicolas B. Pierron [:nbp]]

This bug enables attackers to do random code execution on ARM, ARM64, MIPS32 and MIPS64 backends and first got introduced with IonMonkey (Firefox 18), and now can be exploited through IonMonkey and the WASM Baseline compiler [1]. The WASM Baseline Compiler make these attacks even more reliable, as the code is not discarded on GC and the offset of the jump tables are even more predictable.

The issue is that we might insert a constant pool in the middle of a jump table, which implies that constant of the Jump tables can be used and interpreted as Program Counter. Combined with constant pools being mapped as executable, this implies that we have random code execution if this PC is replaced by the address of a previously generated constant pool.

[1] https://searchfox.org/mozilla-central/search?q=writeCodePointer&path=

Comment 1

[Ryan VanderMeulen [:RyanVM]]

From talking to nbp on IRC, it sounds like we won't have a patch for this until 65 is already at the point of RC and the patch likely won't be trivial. Given the age of this bug and no knowledge of active exploits in the wild, I'm wondering if this is something we should really be trying to rush this cycle vs. aiming for 66/60.6esr instead.

Comment 2

[Nicolas B. Pierron [:nbp]]

For information, Bug 1520169 is one exhibit of this issue where the code of the branch skipping the constant pool is being interpreted as a program counter.

I am currently discussing with Jan if we can make a simpler patch to work-around this issue.

Comment 3

[Lars T Hansen [:lth]]

Hm, are you sure about the wasm baseline compiler? It is careful to call masm.flush and use AutoForbidNops to avoid exactly this problem. https://searchfox.org/mozilla-central/source/js/src/wasm/WasmBaselineCompile.cpp#4644

Comment 4

[Daniel Veditz [:dveditz]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)

Given the age of this bug and no knowledge of active exploits in the wild, I'm wondering if this is something we should really be trying to rush this cycle vs. aiming for 66/60.6esr instead.

I'm sympathetic to those concerns, but we also have to look at the patch (once we have one!) and guess how much fingerpointing it does. If it doesn't make 65 it might then require a point release. Also the bug in comment 2 is public and based on the name found running public tests, so it's potentially rediscoverable. On the other hand it sounds like this only affects mobile processors (do we even ship anything on MIPS?) so that limits the potential for damage and hacker interest.

We should wait for the patch before making a final decision.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure contiguous jump tables. r=

This patch fixes Bug 1520169. IonMonkey was already armored against constant pool insertions on ARM32 but not WasmBaseline.

I will run locally the jit-tests with the ARM simulator to check that everything compiles fine and run perfectly, and comment back once completed.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9037318 [details] [diff] [review]
Ensure contiguous jump tables. r=

(forwarding the review to Sean based on Jan's request)

Comment 7

[Nicolas B. Pierron [:nbp]]

Jan pointed out that WASM Baseline is using MacroAssembler::flush [1] which is equivalent to always writing the constant pool, and as such is not subject to this issue.

Therefore there is no ARM issue, and only ARM64 and MIPS*.

[1] https://searchfox.org/mozilla-central/source/js/src/wasm/WasmBaselineCompile.cpp#4645-4647

Comment 8

[Ryan VanderMeulen [:RyanVM]]

We only recently started publishing ARM64 Fennec nightlies to the Play Store (and Windows ARM64 nightlies are even less widely distributed). Given that, I think we can just let this ride with the next cycle instead of trying to rush it into 65/60.5esr.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure contiguous jump tables.

Delta:
 - Fix visitOutOfLineTableSwitch for ARM64 and MIPS*. (Bug 1521048)

Apparently I forgot these uses from comment 0 link.
Once more ARM32 is looking good.

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure contiguous jump tables.

Delta: Fix copy&pasta issue. Now the patch is verified & working on ARM64.

Comment 13

[Lars T Hansen [:lth]]

According to something Sean said to me some time ago (unless I completely misremember it, or misremember who told me), AutoForbidPools are not meant to be flexible enough to handle large arguments like you'll have for a jump table. Can we have a verification that this is indeed OK?

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Lars T Hansen [:lth] from comment #13)

According to something Sean said to me some time ago (unless I completely misremember it, or misremember who told me), AutoForbidPools are not meant to be flexible enough to handle large arguments like you'll have for a jump table. Can we have a verification that this is indeed OK?

My understanding is that AutoForbidsPool will call enterPool and leavePool and generate a pool in-place if the enterPool estimated code size is larger than the remaining set of instructions which can be skipped. So the only reason for the above argument would be if we were not capable of predicting an upper-bound for the generated code.

In this case, we can easily predict ahead the upper bound for the generated code, computed based on the jump table length.

Comment 15

[Lars T Hansen [:lth]]

Comment on attachment 9037553 [details] [diff] [review]
Ensure contiguous jump tables.

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

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +4653,3 @@
>      // Prevent nop sequences to appear in the jump table.
>      AutoForbidNops afn(&masm);
> +    AutoForbidPools afp(&masm, labels.length() * (sizeof(void*) / insnSize));

Surely we don't care about the instruction size but the pointer size?

Comment 16

[Nicolas B. Pierron [:nbp]]

(In reply to Lars T Hansen [:lth] from comment #15)

• AutoForbidPools afp(&masm, labels.length() * (sizeof(void*) / insnSize));

Surely we don't care about the instruction size but the pointer size?

Indeed, hence the sizeof(void*) expressed in terms of instruction size.

Comment 17

[Sean Stangl [:sstangl]]

Comment on attachment 9037553 [details] [diff] [review]
Ensure contiguous jump tables.

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

r+ with comments.

Regarding security, this only affects MIPS, which we don't support.

ARM64 as deployed on Fennec and elsewhere is unaffected because the problematic code is in IonMonkey, and IonMonkey is disabled. (This patch is part of the work to enable IonMonkey.)

::: js/src/jit/CodeGenerator.cpp
@@ +11705,5 @@
> +#endif
> +  }
> +
> +#if defined(JS_CODEGEN_ARM64) || defined(JS_CODEGEN_MIPS32) ||  \
> +  defined(JS_CODEGEN_MIPS64)

MIPS platforms don't have AutoForbidPools or AutoForbidNops, but they're easy to define.

@@ +11707,5 @@
> +
> +#if defined(JS_CODEGEN_ARM64) || defined(JS_CODEGEN_MIPS32) ||  \
> +  defined(JS_CODEGEN_MIPS64)
> +  const size_t insnSize = 4;
> +  AutoForbidNops afn(&masm);

insertNopFill() already inhibits NOPs if pools are disabled, so I think just AutoForbidPool is sufficient.

Of course, this isn't actually specified anywhere, and ultimately it's OK to leave it even though the behavior is redundant. Whichever way you decide, please just make that consistent across the changes in this patch.

@@ +11708,5 @@
> +#if defined(JS_CODEGEN_ARM64) || defined(JS_CODEGEN_MIPS32) ||  \
> +  defined(JS_CODEGEN_MIPS64)
> +  const size_t insnSize = 4;
> +  AutoForbidNops afn(&masm);
> +  AutoForbidPools afp(&masm, labels.length() * (sizeof(void*) / insnSize));

With regards to Lars' comment, it's true that AutoForbidPools isn't intended for large regions (specifically, because you can no longer guarantee that short jumps are encodeable), but for a jump table that isn't a concern.

::: js/src/jit/arm64/CodeGenerator-arm64.cpp
@@ +814,5 @@
>  
>    masm.haltingAlign(sizeof(void*));
> +  const size_t insnSize = 4;
> +  AutoForbidNops afn(&masm);
> +  AutoForbidPools afp(&masm, mir->numCases() * (sizeof(void*) / insnSize));

In ARM64-specific code, that constant is named `vixl::kInstructionSize`.

Same comment with the AutoForbidNops.

The AutoForbidNops should definitely go prior to the haltingAlign() -- remember that AutoForbidPools can *cause* an immediate pool flush, at which point not only is the jump label potentially unaligned, but also arbitrary constants have been placed immediately prior to the first jump label.

You'll have to do (mir->numCases + 1) to capture the possible alignment.

::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
@@ +1760,5 @@
>  
>    masm.haltingAlign(sizeof(void*));
> +  const size_t insnSize = 4;
> +  AutoForbidNops afn(&masm);
> +  AutoForbidPools afp(&masm, mir->numCases() * (sizeof(void*) / insnSize));

Same comments as with ARM64, plus make sure MIPS actually compiles.

Comment 18

[Nicolas B. Pierron [:nbp]]

Apparently MIPS is not even concerned as they do not use a buffer with constant pools.
So we can open this bug as this only concern ARM64 Ion, which is at the moment disabled.

Comment 19

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure contiguous jump tables.

This is the ammended patch which at the end only contains ARM64 specific changes.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88420eeebda0
Ensure contiguous jump tables. r=sstangl

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/88420eeebda0

Comment 22

[Nicolas B. Pierron [:nbp]]

As this issue only concern the ARM64 - IonMonkey backend, I am updating the status flag to "disabled" on anything prior to Firefox 66.