New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

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



Sign in to add a comment

Security: Integer Overflow when Processing WebAssembly Locals

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

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
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.
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();

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 8

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

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).
Project Member

Comment 5 by sheriffbot@chromium.org, Mar 8

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
Cc: awhalley@chromium.org
How critical is this for M65? 
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. 
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.
Project Member

Comment 9 by sheriffbot@chromium.org, Mar 9

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

Comment 10 by sheriffbot@chromium.org, Mar 9

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
Pls merge your change to M66 branch ASAP so we can pick it up for next M66 Dev/Beta release. Thank you.
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 12

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

Labels: -Merge-Approved-66
The NextAction date has arrived: 2018-03-12
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.
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.
Labels: -Lets Arch-ARM
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.
Labels: Release-0-M66
Labels: CVE-2018-6092
Labels: CVE_description-missing
Project Member

Comment 22 by sheriffbot@chromium.org, Jun 15

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

Sign in to add a comment