New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users
Status: Fixed
Owner:
Closed: Sep 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux, Android, Windows, iOS, Chrome, Mac, Fuchsia
Pri: 1
Type: Bug-Security

Blocking:
issue 766253



Sign in to add a comment
Security: WebAsm OOB ArrayBuffer
Project Member Reported by meacer@chromium.org, Sep 18 Back to list
Split from  bug 766253 :

"""
WebAsm instance builder reads imports from an attacker-controlled object in v8/src/wasm/wasm-module.cc:1625 ProcessImports(). Imports can be getters, which run while the instance is being built and is not in a consistent state. If the getter builds another instance for the same module, then the instances will share a WasmCompiledModule, but will have different ArrayBuffers for memory. Compiled module will reference one memory buffer. If the second memory grows, then the compiled module gets confused and relocates to OOB memory. For trunk, the code has moved to wasm/module-compiler.cc. Exploit in wasm_xpl.js.
"""

 
wasm_xpl.js
7.7 KB View Download
Blocking: 766253
Cc: gdeepti@chromium.org bradnelson@chromium.org mtrofin@chromium.org
Owner: eholk@chromium.org
Eric, Mircea, Deepti, can you take a look at this.

Cc: gzo...@gmail.com
+gzobqq@gmail.com, the original reporter.
Can we get access to the quoted 766253?
Yes, I CCed you.
Owner: mtrofin@chromium.org
Status: Started
Cc: awhalley@chromium.org
I assume we want this backported to 60 and 61, as well as 62, correct?
Project Member Comment 9 by bugdroid1@chromium.org, Sep 19
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/1bc42ab44cd62b860153249e281e8d3b40314cc3

commit 1bc42ab44cd62b860153249e281e8d3b40314cc3
Author: Mircea Trofin <mtrofin@chromium.org>
Date: Tue Sep 19 02:45:11 2017

[wasm] Sanitize imports

Sanitize imports before we start the instance building process. This
avoids the possibility of exiting to JS while building instances,
and allowing JS to observe an inconsistent state of the wasm world -
e.g. incomplete specialization chains.

We now validate we never exit to JS during that process.

Bug:  chromium:766260 

Change-Id: I34930c8b70bdac16af464b3f62a2b6a38107acb3
Reviewed-on: https://chromium-review.googlesource.com/671480
Commit-Queue: Mircea Trofin <mtrofin@chromium.org>
Reviewed-by: Eric Holk <eholk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48074}
[modify] https://crrev.com/1bc42ab44cd62b860153249e281e8d3b40314cc3/src/wasm/module-compiler.cc
[modify] https://crrev.com/1bc42ab44cd62b860153249e281e8d3b40314cc3/src/wasm/module-compiler.h
[modify] https://crrev.com/1bc42ab44cd62b860153249e281e8d3b40314cc3/src/wasm/wasm-objects.cc
[modify] https://crrev.com/1bc42ab44cd62b860153249e281e8d3b40314cc3/src/wasm/wasm-objects.h

Labels: Merge-Request-62 Merge-Request-61 Merge-Request-60
Pls apply appropriate OSs label.
Labels: OS-Android OS-Chrome OS-Fuchsia OS-iOS OS-Linux OS-Mac OS-Windows
Project Member Comment 13 by sheriffbot@chromium.org, Sep 19
Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: Less than 24 days to go before AppStore submit on M62
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
+  awhalley@ for merge review
Project Member Comment 15 by sheriffbot@chromium.org, Sep 19
Labels: M-61
Project Member Comment 16 by sheriffbot@chromium.org, Sep 19
Labels: Pri-1
Project Member Comment 17 by sheriffbot@chromium.org, Sep 19
Status: Fixed
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: keta...@chromium.org cma...@chromium.org josa...@chromium.org abdulsyed@chromium.org amineer@chromium.org
+ Release TPMs as FYI (awhalley@ is currently looking).
Note that this (and the other bugs split from  issue 766253 ) were responsibly disclosed, so while these should be picked up if we respin for other issues I don't consider them a driver for a respin.
Labels: -Merge-Review-62 Merge-Approved-62
Approving this for M62 (branch: 3202)
Labels: -Merge-Approved-62
M62 merge done: https://chromium-review.googlesource.com/c/v8/v8/+/673549


Project Member Comment 22 by sheriffbot@chromium.org, Sep 20
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Comment 23 Deleted
Labels: -Merge-Request-61 Merge-Approved-61
Approving merge to M61.
Labels: -Merge-Approved-61 Merge-Review-61
ketakid@ - 
ketakid@ - this has had very little bake time, I'd be very hesitant to take in 61, is there a particular driver?

(and sorry if the reason was my (now deleted) comment 23, which was intended for a different bug) 
Folks, are we ready to merge this in M61?
Labels: -Merge-Request-60
Removing request for M60 since there are no more M60 planned releases

Ketaki to confirm on M61 merge timing/approval 
Labels: -Merge-Review-61 Merge-Rejected-61
What is the business/urgent case to merge this? I'd prefer to target M-62 which is targeted few weeks from now 

Feel free to re request if there is a very strong reason that can't wait few weeks 
Labels: -M-61 M-62
The only source of urgency is the security aspect - a malicious program can access random data in the same process (renderer).

M62 has it already, see #21.
Let's target M62
Cc: hablich@chromium.org
+hablich

I don't understand why this was merge rejected from 61. This is a security vulnerability that I believe affects 61 as well. 
See https://bugs.chromium.org/p/chromium/issues/detail?id=766260#c19. Retrospectively I would also have preferred to merge this but not respin for it.

We are branching for 6.3 today so 6.1 is obsolete. 
Labels: CVE-2017-15401
Labels: -Restrict-View-SecurityNotify
Labels: allpublic
Sign in to add a comment