Issue metadata
Sign in to add a comment
|
Security: Integer Overflow when Processing WebAssembly Locals |
||||||||||||||||||||||
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.
,
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();
,
Mar 8 2018
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
,
Mar 8 2018
Fixed. Should be backmerged to all channels when we have canary coverage (it's minimal risk).
,
Mar 8 2018
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
,
Mar 8 2018
How critical is this for M65?
,
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.
,
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.
,
Mar 9 2018
,
Mar 9 2018
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
,
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.
,
Mar 12 2018
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
,
Mar 12 2018
,
Mar 12 2018
The NextAction date has arrived: 2018-03-12
,
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.
,
Mar 12 2018
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.
,
Mar 29 2018
,
Apr 6 2018
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.
,
Apr 17 2018
,
Apr 25 2018
,
Apr 25 2018
,
Jun 15 2018
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
,
Dec 4
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by clemensh@chromium.org
, Mar 8 2018Components: -Blink>JavaScript Blink>JavaScript>WebAssembly
Labels: Pri-1
Owner: clemensh@chromium.org
Status: Started (was: Unconfirmed)