New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-03-12
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment
link

Issue 819869: Security: Integer Overflow when Processing WebAssembly Locals

Reported by natashenka@google.com, Mar 8 2018 Project Member

Issue description

When v8 decodes the locals of a function, it performs a check:

if ((count + type_list->size()) > kV8MaxWasmFunctionLocals) {
        decoder->error(decoder->pc() - 1, "local count too large");
        return false;
      }

On a 32-bit platform, this check can be bypassed due to an integer overflow. This allows the number of function locals to be large, and can lead to memory corruption when the locals are allocated.

A PoC is attached. 

This bug is subject to a 90 day disclosure deadline. After 90 days elapse
or a patch has been made broadly available, the bug report will become
visible to the public.
 
locals
2.9 KB View Download

Comment 1 by clemensh@chromium.org, Mar 8 2018

Cc: titzer@chromium.org
Components: -Blink>JavaScript Blink>JavaScript>WebAssembly
Labels: Pri-1
Owner: clemensh@chromium.org
Status: Started (was: Unconfirmed)
Thanks for reporting! Will work on a fix.

Comment 2 by clemensh@chromium.org, Mar 8 2018

Reduced reproducer:

load('test/mjsunit/wasm/wasm-constants.js');
load('test/mjsunit/wasm/wasm-module-builder.js');

var builder = new WasmModuleBuilder();
builder.addFunction(undefined, kSig_i_i)
    .addLocals({i32_count: 0xffffffff})
    .addBody([]);
builder.instantiate();

Comment 3 by bugdroid1@chromium.org, Mar 8 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a71e5f9a7b3c968fd961683e305ff1cc9314754c

commit a71e5f9a7b3c968fd961683e305ff1cc9314754c
Author: Clemens Hammacher <clemensh@chromium.org>
Date: Thu Mar 08 17:00:55 2018

[wasm] Avoid integer overflow on function locals check

On 32-bit systems, the computation {count + type_list->size()} can
overflow, leading to memory corruption later on.

R=titzer@chromium.org

Bug:  chromium:819869 
Change-Id: Ic81d201e58211e3989b4e945cd52e98dc951fbda
Reviewed-on: https://chromium-review.googlesource.com/955025
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51817}
[modify] https://crrev.com/a71e5f9a7b3c968fd961683e305ff1cc9314754c/src/wasm/baseline/liftoff-compiler.cc
[modify] https://crrev.com/a71e5f9a7b3c968fd961683e305ff1cc9314754c/src/wasm/function-body-decoder-impl.h
[add] https://crrev.com/a71e5f9a7b3c968fd961683e305ff1cc9314754c/test/mjsunit/regress/wasm/regress-819869.js

Comment 4 by clemensh@chromium.org, Mar 8 2018

Labels: Security_Impact-Stable Merge-Request-66 Merge-Request-65 Security_Severity-High OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
NextAction: 2018-03-12
Status: Fixed (was: Started)
Fixed. Should be backmerged to all channels when we have canary coverage (it's minimal risk).

Comment 5 by sheriffbot@chromium.org, Mar 8 2018

Project Member
Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 6 by cmasso@google.com, Mar 8 2018

Cc: awhalley@chromium.org
How critical is this for M65?

Comment 7 by clemensh@chromium.org, Mar 8 2018

It leads to an out of bounds write. Don't know if you can use this to compromise the renderer, I am not a security expert. We should include it in upcoming respins though. Especially given that the fix is really small and simple.

Comment 8 by awhalley@google.com, Mar 9 2018

Let's give this some time on Canary and then merge to 66. Once it's been out in a beta we can look at merging to 65 in case there are any respins.

Comment 9 by sheriffbot@chromium.org, Mar 9 2018

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 10 by sheriffbot@chromium.org, Mar 9 2018

Project Member
Labels: -Merge-Request-66 Merge-Approved-66 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M66. Please go ahead and merge the CL to branch 3359 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 11 by gov...@chromium.org, Mar 9 2018

Pls merge your change to M66 branch ASAP so we can pick it up for next M66 Dev/Beta release. Thank you.

Comment 12 by bugdroid1@chromium.org, Mar 12 2018

Project Member
Labels: merge-merged-6.6
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/28cd41e26f60089929a31245f675b852f27edc5e

commit 28cd41e26f60089929a31245f675b852f27edc5e
Author: Clemens Hammacher <clemensh@chromium.org>
Date: Mon Mar 12 09:30:03 2018

Merged: [wasm] Avoid integer overflow on function locals check

On 32-bit systems, the computation {count + type_list->size()} can
overflow, leading to memory corruption later on.

R=‚Äčahaas@chromium.org
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true

Bug:  chromium:819869 
Change-Id: I7be8660d25af4ab5a9129b44b24d0820997f3517
Originally-reviewed-on: https://chromium-review.googlesource.com/955025
Reviewed-on: https://chromium-review.googlesource.com/958343
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.6@{#13}
Cr-Branched-From: d500271571b92cb18dcd7b15885b51e8f437d640-refs/heads/6.6.346@{#1}
Cr-Branched-From: 265ef0b635f8761df7c89eb4e8ec9c1a6ebee184-refs/heads/master@{#51624}
[modify] https://crrev.com/28cd41e26f60089929a31245f675b852f27edc5e/src/wasm/baseline/liftoff-compiler.cc
[modify] https://crrev.com/28cd41e26f60089929a31245f675b852f27edc5e/src/wasm/function-body-decoder-impl.h
[add] https://crrev.com/28cd41e26f60089929a31245f675b852f27edc5e/test/mjsunit/regress/wasm/regress-819869.js

Comment 13 by clemensh@chromium.org, Mar 12 2018

Labels: -Merge-Approved-66

Comment 14 by monor...@bugs.chromium.org, Mar 12 2018

The NextAction date has arrived: 2018-03-12

Comment 15 by clemensh@chromium.org, Mar 12 2018

This has been in the latest canary (67.0.3368.0), and we need to respin M65 anyway. Given that this is a minimal-risk fix, we should merge it to 65 ASAP imo.

Comment 16 by awhalley@google.com, Mar 12 2018

Labels: Lets
I agree it's a low risk fix, though it's a bit fresh for a stable merge right now I'm afraid. Let's revisit when it's been out in dev.

Comment 17 by gov...@chromium.org, Mar 29 2018

Labels: -Lets Arch-ARM

Comment 18 by gov...@chromium.org, Apr 6 2018

Labels: -Merge-Review-65 Merge-Rejected-65
Rejecting merge to M65 as we're not planning any further M65 releases. 
awhalley@, Please let me know if there is any concern here. Thank you.

Comment 19 by awhalley@google.com, Apr 17 2018

Labels: Release-0-M66

Comment 20 by awhalley@chromium.org, Apr 25 2018

Labels: CVE-2018-6092

Comment 21 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-missing

Comment 22 by sheriffbot@chromium.org, Jun 15 2018

Project Member
Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 23 by awhalley@chromium.org, Dec 4

Labels: -CVE_description-missing CVE_description-submitted

Sign in to add a comment