[wasm] We call the start function with the wrong instance |
||||||||||||||
Issue descriptionHere, we create the start function as JS-to-wasm wrapper with the instance which is currently being built: https://cs.chromium.org/chromium/src/v8/src/wasm/module-compiler.cc?q=module-compiler.cc&sq=package:chromium&dr&l=1836 If the start function is an exported function of another wasm instance, we should use that instance instead. Otherwise, we execute code of module X on an instance of module Y. This can easily be used to construct OOB reads and writes. Reproducer: ================================================================================= load('test/mjsunit/wasm/wasm-constants.js'); load('test/mjsunit/wasm/wasm-module-builder.js'); (function testImportedStartFunctionUsesRightInstance() { print(arguments.callee.name); var global = 0; const set_global = n => global = n; const exp = (function() { const builder = new WasmModuleBuilder(); builder.addMemory(1, 1); builder.exportMemoryAs('mem'); const imp_index = builder.addImport('q', 'imp', kSig_v_i); builder.addFunction('f', kSig_v_v) .addBody([kExprI32Const, 11, kExprI32Const, 0, kExprI32StoreMem, 0, 0]) .exportFunc(); return builder.instantiate({q: {imp: set_global}}).exports; })(); const builder = new WasmModuleBuilder(); const imp_index = builder.addImport('q', 'imp', kSig_v_v); builder.addStart(imp_index); const module = builder.toModule(); assertEquals(0, new Uint32Array(exp.mem.buffer)[0], 'memory initially 0'); new WebAssembly.Instance(module, {q: {imp: exp.f}}); assertEquals(11, new Uint32Array(exp.mem.buffer)[0], 'memory changed to 11'); })(); =================================================================================
,
May 15 2018
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6308651846074368.
,
May 15 2018
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6318487723835392.
,
May 15 2018
Detailed report: https://clusterfuzz.com/testcase?key=6318487723835392 Job Type: linux_asan_d8 Platform Id: linux Crash Type: Null-dereference WRITE Crash Address: 0x00000000000b Crash State: v8::internal::Invoke v8::internal::CallInternal v8::internal::Execution::Call Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=linux_asan_d8&range=52435:52436 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6318487723835392 See https://github.com/google/clusterfuzz-tools for more information.
,
May 15 2018
,
May 15 2018
For M66, this is no issue, since we still embed the instance in the generated js-to-wasm code. But we will need to merge the fix (https://crrev.com/c/1059620) to M67.
,
May 15 2018
Great catch! I'm OK not blocking tomorrow's Beta on this, moving to Release Block Stable.
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/37e9017f8e3c87d7b1efcea6761fd313d72171f3 commit 37e9017f8e3c87d7b1efcea6761fd313d72171f3 Author: Clemens Hammacher <clemensh@chromium.org> Date: Tue May 15 16:17:29 2018 [wasm] Use correct instance when calling start function We were always using the instance we were currently building. If the start function is an exported wasm function of another instance, use the exporting instance instead. R=titzer@chromium.org Bug: chromium:843120 Change-Id: I141d272b947bef8e903be7208ddf6ce344e754c4 Reviewed-on: https://chromium-review.googlesource.com/1059620 Commit-Queue: Clemens Hammacher <clemensh@chromium.org> Reviewed-by: Ben Titzer <titzer@chromium.org> Cr-Commit-Position: refs/heads/master@{#53190} [modify] https://crrev.com/37e9017f8e3c87d7b1efcea6761fd313d72171f3/src/wasm/module-compiler.cc [modify] https://crrev.com/37e9017f8e3c87d7b1efcea6761fd313d72171f3/test/mjsunit/wasm/import-function.js
,
May 15 2018
Requesting backmerge already. Feel free to suggest a later date, but we will need to merge this to M67.
,
May 15 2018
This bug requires manual review: We are only 13 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 15 2018
Cl landed at #8 is not in canary yet. awhalley@ for M67 merge review after canary baking.
,
May 16 2018
ClusterFuzz has detected this issue as fixed in range 53189:53190. Detailed report: https://clusterfuzz.com/testcase?key=6318487723835392 Job Type: linux_asan_d8 Platform Id: linux Crash Type: Null-dereference WRITE Crash Address: 0x00000000000b Crash State: v8::internal::Invoke v8::internal::CallInternal v8::internal::Execution::Call Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=linux_asan_d8&range=52435:52436 Fixed: https://clusterfuzz.com/revisions?job=linux_asan_d8&range=53189:53190 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6318487723835392 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 16 2018
ClusterFuzz testcase 6318487723835392 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
May 16 2018
,
May 18 2018
govind@ - good for 67
,
May 18 2018
Approving merge to M67 branch 3396 based on comment #15. Please merge latest by Monday (05/18), 4:00 PM PT so we can take it in for next week last M67 Beta release. Thank you.
,
May 18 2018
I just tried a backmerge, but it's not trivial. https://crrev.com/c/1059620 is only effective in combination with https://crrev.com/c/1028053, which again depends on https://crrev.com/c/1047025. I uploaded a CL for merging all three as https://crrev.com/c/1065681. I tested it locally to verify that this passes all new tests. Ben or Michi, please decide whether it makes sense to merge this CL. Alternatively, we could just forbid importing function of another instance as start function for M67 to shut down this attack vector. govind@: please note that Monday is public holiday in Germany, so I am not sure if this merge will make it to the next beta release.
,
May 18 2018
Thank you clemensh@. awhalley@ (Security TPM) is going to take a look, so pls DO NOT merge, Changing it back to Merge-Review-67.
,
May 21 2018
awhalley@ based on #17, will it be safe to take this merge in for M67? OR can it wait untill M68?
,
May 21 2018
I'd also be interested in Ben or Michi's risk assessment. Given we're going to miss the last beta, I'd be OK erring on the side of caution and not rushing to merge this. If we can get some extra confidence, then we should look at merging to 67 post Stable and picking it up in a respin, but if that means it doesn't end up being fixed until 68, so be it. +inferno@ to sanity check the plan :-)
,
May 22 2018
Thanks Clemens for creating the combined CL for this. This is a pretty involved backmerge, since we changed the way the instance is loaded from from exported functions, so I would be cautious here. mstarzinger@ is on vacation until tomorrow, so I wouldn't feel comfortable backmerging without his goahead. Will ask tomorrow.
,
May 22 2018
As this is not a trivial merge, going to miss last beta release and M67 stable promotion is next week, I don't feel comfortable taking this merge in for M67. Can this wait for M68, awhalley@ & inferno@?
,
May 22 2018
Removing ReleaseBlock-Stable, we can re-assess what merges are needed once we've got some more eyes on the fix, but I'm OK not blocking the release.
,
May 22 2018
Rejecting merge to M67 based on comment #23. If needed, Pls re-request a merge to M67 when you all think it is safe to merge.
,
Aug 22
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 clemensh@chromium.org
, May 15 2018