[wasm] Shared js-to-wasm wrappers call to instance-specific wasm-to-js wrapper |
|||||||||
Issue description
Scenario: A wasm module exports an imported function.
During compilation of the module, we create js-to-wasm wrappers for the exports.
During instantiation, we create a wasm-to-js wrapper for each import which is a JS object (i.e. not a wasm export).
We then patch the js-to-wasm wrapper to call this wasm-to-js wrapper.
Problem:
The wasm-to-js wrapper does not embed the JS object (which would be even worse); instead it calls via the ImportedFunctionCallables on the instance.
However, the wrapper is specialized to the import: We generate different code depending on whether it's a JSFunction with matching argument count, or other callable JS objects.
The shared js-to-wasm wrapper will always call the latest created wasm-to-js wrapper, even though other instances might require different code, because they have other imports. This can lead to arbitrary read and write access to the stack.
Reproducer:
load('test/mjsunit/wasm/wasm-constants.js');
load('test/mjsunit/wasm/wasm-module-builder.js');
const builder = new WasmModuleBuilder();
sig1 = makeSig([kWasmI32, kWasmI32, kWasmI32], [kWasmI32]);
const imp_idx = builder.addImport('q', 'imp', kSig_i_i);
builder.addExport('exp', imp_idx);
const module = builder.toModule();
function bad(a, b, c, d, e, f, g, h) {
print(JSON.stringify([a, b, c, d, e, f, g, h]));
}
const instance1 = new WebAssembly.Instance(module, {q: {imp: bad}});
const instance2 = new WebAssembly.Instance(module, {q: {imp: i => i}});
print(instance1.exports.exp(5));
,
May 16 2018
Detailed report: https://clusterfuzz.com/testcase?key=5862688748732416 Job Type: linux_d8_dbg Platform Id: linux Crash Type: DCHECK failure Crash Address: Crash State: object->IsJSReceiver() in json-stringifier.cc Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=linux_d8_dbg&range=53057:53058 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5862688748732416 See https://github.com/google/clusterfuzz-tools for more information.
,
May 16 2018
Looks like this was introduced after the M67 branch.
,
May 16 2018
,
May 17 2018
,
May 17 2018
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it. If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/3637e15f408fdaad21763c42ad457eab3c205433 commit 3637e15f408fdaad21763c42ad457eab3c205433 Author: Clemens Hammacher <clemensh@chromium.org> Date: Thu May 17 16:34:49 2018 [wasm] Don't extract call target from WasmExportedFunction We need to change WasmExportedFunction to call imported functions via the import table, so there will be no embedded call target. This also removes the necessity to generate an unreachable call after the runtime call for js-incompatible signatures. R=titzer@chromium.org Bug: chromium:843563 , v8:6668 Change-Id: I82cb31930f6b61ad59fde63a8c5ae631da3d1a14 Reviewed-on: https://chromium-review.googlesource.com/1063771 Commit-Queue: Clemens Hammacher <clemensh@chromium.org> Reviewed-by: Ben Titzer <titzer@chromium.org> Cr-Commit-Position: refs/heads/master@{#53239} [modify] https://crrev.com/3637e15f408fdaad21763c42ad457eab3c205433/src/compiler/wasm-compiler.cc [modify] https://crrev.com/3637e15f408fdaad21763c42ad457eab3c205433/src/wasm/module-compiler.cc [modify] https://crrev.com/3637e15f408fdaad21763c42ad457eab3c205433/src/wasm/wasm-objects.cc [modify] https://crrev.com/3637e15f408fdaad21763c42ad457eab3c205433/src/wasm/wasm-objects.h [modify] https://crrev.com/3637e15f408fdaad21763c42ad457eab3c205433/test/inspector/inspector.status
,
May 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/71c05457e285394a9d11ba046b34ab81d57d9f55 commit 71c05457e285394a9d11ba046b34ab81d57d9f55 Author: Clemens Hammacher <clemensh@chromium.org> Date: Fri May 18 12:56:26 2018 [wasm] Call imports via import table in js-to-wasm wrappers The js-to-wasm wrappers are shared across instances, so we cannot directly call the instance-specific wasm-to-js wrappers. Instead, we need to call via the import table. R=titzer@chromium.org Bug: chromium:843563 Change-Id: Ia882604f6769472fe2eb69176cbed728215ced29 Reviewed-on: https://chromium-review.googlesource.com/1064610 Reviewed-by: Ben Titzer <titzer@chromium.org> Commit-Queue: Clemens Hammacher <clemensh@chromium.org> Cr-Commit-Position: refs/heads/master@{#53254} [modify] https://crrev.com/71c05457e285394a9d11ba046b34ab81d57d9f55/src/compiler/wasm-compiler.cc [modify] https://crrev.com/71c05457e285394a9d11ba046b34ab81d57d9f55/src/compiler/wasm-compiler.h [modify] https://crrev.com/71c05457e285394a9d11ba046b34ab81d57d9f55/src/wasm/module-compiler.cc [add] https://crrev.com/71c05457e285394a9d11ba046b34ab81d57d9f55/test/mjsunit/regress/wasm/regress-843563.js
,
May 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/6d87fbc756561cb0920382534fbe3132795fc0f0 commit 6d87fbc756561cb0920382534fbe3132795fc0f0 Author: Clemens Hammacher <clemensh@chromium.org> Date: Fri May 18 13:00:36 2018 [wasm] Don't store imported WasmCode pointers in code table When processing imports of an instance, we were storing pointers to exported (and re-imported) wasm functions in the code table of the importing module. This is dangerous since imports are instance specific. Avoid ever storing call targets for imports in the NativeModule. Instead, read the call targets from the imports table of the instance. R=mstarzinger@chromium.org Bug: chromium:843563 Change-Id: Id9f43a6c127025a5feaa81b2be75c001bc0bea81 Reviewed-on: https://chromium-review.googlesource.com/1065774 Commit-Queue: Clemens Hammacher <clemensh@chromium.org> Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Cr-Commit-Position: refs/heads/master@{#53256} [modify] https://crrev.com/6d87fbc756561cb0920382534fbe3132795fc0f0/src/wasm/module-compiler.cc [modify] https://crrev.com/6d87fbc756561cb0920382534fbe3132795fc0f0/src/wasm/wasm-code-manager.h [modify] https://crrev.com/6d87fbc756561cb0920382534fbe3132795fc0f0/src/wasm/wasm-debug.cc [modify] https://crrev.com/6d87fbc756561cb0920382534fbe3132795fc0f0/test/cctest/wasm/test-run-wasm.cc
,
May 18 2018
,
May 19 2018
ClusterFuzz has detected this issue as fixed in range 53253:53254. Detailed report: https://clusterfuzz.com/testcase?key=5862688748732416 Job Type: linux_d8_dbg Platform Id: linux Crash Type: DCHECK failure Crash Address: Crash State: object->IsJSReceiver() in json-stringifier.cc Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=linux_d8_dbg&range=53057:53058 Fixed: https://clusterfuzz.com/revisions?job=linux_d8_dbg&range=53253:53254 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5862688748732416 See https://github.com/google/clusterfuzz-tools for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
May 19 2018
ClusterFuzz testcase 5862688748732416 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Jun 5 2018
,
Aug 24
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 |
|||||||||
Comment 1 by ClusterFuzz
, May 16 2018