1444668 - Write beyond bounds caused by overlarge offset in WASM assembler

Status: RESOLVED FIXED

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

Description

[mozillabugs]

The WASM assembler can generate an out-of-range value for |ModuleGenerator::callFarJumps_.jump.offset_|. If it does, it causes SetInt32() (Patching-x86-shared.h) to attempt to write beyond bounds. When this occurs, SetInt32() is called via the following chain:

   SetInt32()
   ^--- SetRel32() (Patching-x86-shared.h) 
        ^--- patchFarJump() (Assembler-x86-shared.h)
		     ^--- ModuleGenerator::finishCode() (WasmGenerator.cpp)

Attached is a POC that causes this write.


The out-of-range value for |offset_| (0xffffffff820206cb in the attached POC) appears to occur because the POC's WASM program is large enough to caused js::jit::X86Encoding::BaseAssembler::m_formatter.m_buffer to contain more than 0x7fffffff bytes.

When ModuleGenerator::finishCode() attempts to link the code, it eventually calls X86Encoding::BaseAssembler::X86InstructionFormatter::immediateRel32(), which contains this code:

   5093: m_buffer.putIntUnchecked(0);
   5094: return JmpSrc(m_buffer.size());

However, m_buffer.size() returns a |size_t| (64 bits on 64 bit builds) while JmpSrc() takes an |int32_t|. The code here should (but does not) catch this out-of-range value. (Probably some other code should catch it earlier, but I don't understand the assembler well enough to identify that code).


In any case, the bad value propagates back to the caller farJumpWithPatch() (Assembler_x86_shared.h), which calls CodeOffset() (Assembler_shared.h), which puts it, along with the relevant function index, into |callFarJumps_.jump.offset_| (seer ModuleGenerator::linkCallSites()).

Later, when ModuleGenerator::finishCode() calls masm_.patchFarJump(), pathFarJump() computes

    code + farJump.offset()

which points well before the start of |code| because offset() is negative.

The result is an attempt to write beyond bounds. This crashes FF 58.0.1 and 58.0.2 x64 release. On at least the debug version of 58.0.2, however, it successfully overwrites something else on the heap.


I found this bug while attempting to force an overflow elsewhere in the WASM assembler. I will report that bug separately.


Use the PoC by starting FF 58.0.1/.2 x64, attaching a debugger to it, and setting a BP in ModuleGenerator::finishCode() where it processes |callFarJumps_|. Put the POC files on a webserver. Load test40.html in FF (you might need to attach the debugger again if a new content process appears), and wait for the BP to fire. When it does, examine |callFarJumps_| and notice that at least one entry contains a negative offset. Trace the code until you hit the

   mov   dword ptr [r10-4], edx

and watch it either write far beyond bounds or crash with an access violation.

Comment 1

[mozillabugs]

Attachment: POC. Use as directed and call me in the morning

Comment 2

[mozillabugs]

> I found this bug while attempting to force an overflow elsewhere in the WASM
> assembler. I will report that bug separately.

https://bugzilla.mozilla.org/show_bug.cgi?id=1444673

Comment 3

[mozillabugs]

"large enough to caused" => large enough to cause.

Comment 4

[Nicolas B. Pierron [:nbp]]

Benjamin, can you confirm this issue and find the right person to fix it as soon as possible?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Great catch, thanks! I think we should:

- either report masm.oom() when the size > INT32_MAX, which I'm going to try.
- or replace JmpSrc/JmpDst by a JmpOffset which has a size_t, and audit usage up to callers.

The first solution would be a better stopgap solution, and the second one would be better in the long term.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> - either report masm.oom() when the size > INT32_MAX, which I'm going to try.

See bug 1444673 comment 8, I think we should use a lower/safer limit.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: stopgap.patch

Stopgap solution using MaxCodeBytesPerProcess (and fixing another potential overflow in same class).

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8959594 [details] [diff] [review]
stopgap.patch

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

Thanks for fixing this! Some comments below.

::: js/src/jit/x86-shared/AssemblerBuffer-x86-shared.h
@@ +87,5 @@
>  
>          void ensureSpace(size_t space)
>          {
> +            if (MOZ_UNLIKELY(m_buffer.length() > SIZE_MAX - space ||
> +                             !m_buffer.reserve(m_buffer.length() + space)))

This cannot overflow because we call this with |space <= 16| everywhere as far as I can see. So on 32-bit we'd have to allocate almost 4 GB (our whole address space) and that's not possible because we also have the binary and a ton of other stuff in there.

So I think we should MOZ_ASSERT space <= 16 or so, with a comment saying that we only allow small values to avoid overflow risk here.

Usually I don't mind adding overflow checks, but this is called for every instruction we emit so I want to be a bit more careful.

@@ +124,5 @@
>          }
>  
>          bool oom() const
>          {
> +            return m_oom || size() > MaxCodeBytesPerProcess;

I was hoping we could check MaxCodeBytesPerProcess when we actually resize the buffer, but doing that in ensureSpace is unfortunate because we call that for every instruction.

What we could do is add our own alloc policy where we check this and then use that instead of SystemAllocPolicy for our AssemblerBuffer. Are you interested in doing this? I can steal this if you want.

Comment 9

[mozillabugs]

> This cannot overflow because we call this with |space <= 16| everywhere as far as I can see. So on 32-bit we'd have to allocate 
> almost 4 GB (our whole address space) and that's not possible because we also have the binary and a ton of other stuff in 
> there.

But allocating >= 4GB is no problem on x64 builds. What am I missing?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to mozillabugs from comment #9)
> But allocating >= 4GB is no problem on x64 builds. What am I missing?

On 64-bit platforms, size_t is 8 bytes so it's fine to allocate 4 GB (or even 4 TB) and the same thing applies.

Comment 11

[mozillabugs]

(In reply to Jan de Mooij [:jandem] from comment #10)
> (In reply to mozillabugs from comment #9)
> > But allocating >= 4GB is no problem on x64 builds. What am I missing?
> 
> On 64-bit platforms, size_t is 8 bytes so it's fine to allocate 4 GB (or
> even 4 TB) and the same thing applies.

Ah, I see:

   void ensureSpace(size_t space)

Thanks.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jan de Mooij [:jandem] from comment #8)
> What we could do is add our own alloc policy where we check this and then
> use that instead of SystemAllocPolicy for our AssemblerBuffer. Are you
> interested in doing this? I can steal this if you want.

Yeah, feel free to steal; I'd be happy to review it though.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Patch

Also r? luke because this is related to bug 1444673.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8960506 [details] [diff] [review]
Patch

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

Actually bytesNeeded adds extra bytes for the relocation tables so the release asserts in this patch are not entirely correct.

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8960603 [details] [diff] [review]
Patch

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

Awesome, thanks!

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Patch

Updated patch. We now also check bytesNeeded() in MacroAssembler::finish and OOM if that exceeds MaxCodeBytesPerProcess. I also moved the size() release assert to that function.

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8960648 [details] [diff] [review]
Patch

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

LGTM, thanks.

::: js/src/jit/shared/IonAssemblerBuffer.h
@@ +194,5 @@
>      }
>  
>    private:
>      Slice* newSlice(LifoAlloc& a) {
> +        if (size() + sizeof(Slice) > MaxCodeBytesPerProcess) {

if size() grows very quick, could it overflow? Just to be sure, could we check `if (size() > MaxCodeBytesPerProcess - sizeof(Slice))`?

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #19)
> if size() grows very quick, could it overflow? Just to be sure, could we
> check `if (size() > MaxCodeBytesPerProcess - sizeof(Slice))`?

The patch is making sure this won't overflow :) But sure I can change it anyway.

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not trivial but might not be too hard, as the PoC here shows.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, it's pretty obvious from the code.

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

Comment 22

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

I'm worried about exposure here. I'm like to wait until April 10 to check this in so it isn't exposed on trunk for too long. 

I'd like Beta and ESR52 patches prepared ahead of time and nominated to go in after this lands on trunk.

Comment 23

[Jan de Mooij [:jandem]]

I'll send this to Try, land it today, and post branch patches.

Comment 24

[Jan de Mooij [:jandem]]

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

Comment 25

[Jan de Mooij [:jandem]]

Comment on attachment 8961306 [details] [diff] [review]
Patch

This applies to beta, will post a separate patch for ESR52.

Approval Request Comment
[Feature/Bug causing the regression]: Older bug.
[User impact if declined]: Security issues.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Should only affect very unlikely cases that involve very large buffers. This code is exercised by tons of tests.
[String changes made/needed]: None.

Comment 26

[Jan de Mooij [:jandem]]

Attachment: Patch for ESR52

[Approval Request Comment]
User impact if declined: Security issues.
Fix Landed on Version: 61, backporting to 60.
Risk to taking this patch (and alternatives if risky): Very low.
String or UUID changes made by this patch: None.

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8961306 [details] [diff] [review]
Patch

Fix a JS sec-high. Approved for 60.0b12 and ESR 52.8.0.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/29b2342e2a32
https://hg.mozilla.org/releases/mozilla-esr52/rev/666fc84ec72d

Comment 30

[Andrei Vaida [:avaida]]

(In reply to Jan de Mooij [:jandem] from comment #25)
> [Is this code covered by automated tests?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.
> [List of other uplifts needed for the feature/fix]: None.
> [Is the change risky?]: Low risk.

Does not require manual testing, per Jan.