Closed Bug 1444668 Opened 3 years ago Closed 3 years ago

Write beyond bounds caused by overlarge offset in WASM assembler

Categories

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

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 60+ fixed
firefox59 --- wontfix
firefox60 + fixed
firefox61 + fixed

People

(Reporter: mozillabugs, Assigned: jandem)

Details

(Keywords: csectype-bounds, csectype-intoverflow, sec-high, Whiteboard: [adv-main60+][adv-esr52.8+])

Attachments

(3 files, 4 obsolete files)

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.
Flags: sec-bounty?
> 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
"large enough to caused" => large enough to cause.
Group: core-security → javascript-core-security
Benjamin, can you confirm this issue and find the right person to fix it as soon as possible?
Flags: needinfo?(bbouvier)
Priority: -- → P1
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.
Assignee: nobody → bbouvier
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bbouvier)
(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.
Attached patch stopgap.patch (obsolete) — Splinter Review
Stopgap solution using MaxCodeBytesPerProcess (and fixing another potential overflow in same class).
Attachment #8959594 - Flags: review?(jdemooij)
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.
Attachment #8959594 - Flags: review?(jdemooij)
> 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?
(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.
(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.
(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.
Flags: needinfo?(jdemooij)
Attached patch Patch (obsolete) — Splinter Review
Also r? luke because this is related to bug 1444673.
Assignee: bbouvier → jdemooij
Flags: needinfo?(jdemooij)
Attachment #8960506 - Flags: review?(luke)
Attachment #8960506 - Flags: review?(bbouvier)
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.
Attachment #8960506 - Flags: review?(luke)
Attachment #8960506 - Flags: review?(bbouvier)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8959594 - Attachment is obsolete: true
Attachment #8960506 - Attachment is obsolete: true
Attachment #8960603 - Flags: review?(luke)
Attachment #8960603 - Flags: review?(bbouvier)
Comment on attachment 8960603 [details] [diff] [review]
Patch

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

Awesome, thanks!
Attachment #8960603 - Flags: review?(luke) → review+
Duplicate of this bug: 1444673
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8960603 - Attachment is obsolete: true
Attachment #8960603 - Flags: review?(bbouvier)
Attachment #8960648 - Flags: review?(bbouvier)
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))`?
Attachment #8960648 - Flags: review?(bbouvier) → review+
(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.
Attached patch PatchSplinter Review
[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.
Attachment #8960648 - Attachment is obsolete: true
Attachment #8961306 - Flags: sec-approval?
Attachment #8961306 - Flags: review+
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.
Whiteboard: [checkin on 4/10]
Attachment #8961306 - Flags: sec-approval? → sec-approval+
Flags: sec-bounty? → sec-bounty+
I'll send this to Try, land it today, and post branch patches.
Whiteboard: [checkin on 4/10]
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.
Attachment #8961306 - Flags: approval-mozilla-beta?
Attached patch Patch for ESR52Splinter Review
[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.
Attachment #8966576 - Flags: review+
Attachment #8966576 - Flags: approval-mozilla-esr52?
https://hg.mozilla.org/mozilla-central/rev/d4aad4680624
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8961306 [details] [diff] [review]
Patch

Fix a JS sec-high. Approved for 60.0b12 and ESR 52.8.0.
Attachment #8961306 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8966576 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main60+][adv-esr52.8+]
(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.
Flags: qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.