Issue metadata
Sign in to add a comment
|
[wasm] Signature confusion in function table import/export/init |
||||||||||||||||||||||
Issue description
Both of the following snippets validate and produce this output:
=============================================
wasm-function[0]:1: RuntimeError: unreachable
RuntimeError: unreachable
at module_1.main (wasm-function[0]:1)
at module_0.main (wasm-function[0]:3)
at test.js:32:19
=============================================
Version 1 (which looks correct to me):
=============================================
let builder0 = new WasmModuleBuilder();
builder0.setName('module_0');
let sig_index = builder0.addType(kSig_i_i);
builder0.addFunction('main', kSig_i_i)
.addBody([
kExprGetLocal, 0, // --
kExprGetLocal, 0, // --
kExprCallIndirect, sig_index, kTableZero
]) // --
.exportAs('main');
builder0.setFunctionTableLength(3);
builder0.addExportOfKind('table', kExternalTable);
let module0 = new WebAssembly.Module(builder0.toBuffer());
let instance0 = new WebAssembly.Instance(module0);
let builder1 = new WasmModuleBuilder();
builder1.setName('module_1');
builder1.addFunction('main', kSig_i_i).addBody([kExprUnreachable]);
builder1.addImportedTable('z', 'table');
builder1.addFunctionTableInit(0, false, [0], true);
let module1 = new WebAssembly.Module(builder1.toBuffer());
let instance1 =
new WebAssembly.Instance(module1, {z: {table: instance0.exports.table}});
instance0.exports.main(0);
=============================================
Version 2 (which looks incorrect to me):
=============================================
let builder0 = new WasmModuleBuilder();
builder0.setName('module_0');
let sig_index = builder0.addType(kSig_i_v);
builder0.addFunction('main', kSig_i_i)
.addBody([
kExprGetLocal, 0, // --
kExprCallIndirect, sig_index, kTableZero
]) // --
.exportAs('main');
builder0.setFunctionTableLength(3);
builder0.addExportOfKind('table', kExternalTable);
let module0 = new WebAssembly.Module(builder0.toBuffer());
let instance0 = new WebAssembly.Instance(module0);
let builder1 = new WasmModuleBuilder();
builder1.setName('module_1');
builder1.addFunction('main', kSig_i_i).addBody([kExprUnreachable]);
builder1.addImportedTable('z', 'table');
builder1.addFunctionTableInit(0, false, [0], true);
let module1 = new WebAssembly.Module(builder1.toBuffer());
let instance1 =
new WebAssembly.Instance(module1, {z: {table: instance0.exports.table}});
instance0.exports.main(0);
=============================================
Notice the difference in the third line!
Tagging as security issue. If we really mess up signature checks here, this can have severe consequences.
,
Jul 13 2017
We are still investigating, I will update this bug appropriately as we find out more.
,
Jul 13 2017
Bug and high security impact confirmed. This bug allows to call wasm functions with non-matching signatures, hence the called code might assume a different number of arguments on the stack. This can at least be used to read stack content, maybe worse. Working on a fix, and more test cases. Bug exists since a long time, adding appropriate labels.
,
Jul 14 2017
Fix landed: https://chromium-review.googlesource.com/570278 (forgot to reference this bug). Requesting backmerge to all relevant versions. Will only backmerge once we have some canary coverage (i.e. after the weekend).
,
Jul 14 2017
This bug requires manual review: We are only 10 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 15 2017
Please confirm when you've verified this in canary on Monday. Will review then for M60. We are only 2 weeks away from M60 Stable. Rejecting merge to M59.
,
Jul 15 2017
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
,
Jul 16 2017
,
Jul 17 2017
The patch was contained in the last two canaries (3158 and 3159). Canaries looking good. PTAL.
,
Jul 17 2017
I would have been tempted to merge this back without canary coverage. Is there a policy governing when we can merge back without canary coverage?
,
Jul 17 2017
Approving merge to M60. +awhalley@ titzer@ all changes should be verified in canary first. Please check new merge guideline (https://chromium.googlesource.com/chromium/src.git/+/master/docs/process/merge_request.md#Merge-Requirements). There are rare scenarios and extremely urgent cases where we might have to bypass the usual process, but for normal merges, all changes must be tested in lower channels.
,
Jul 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/3c9b5b19ed1a61ebea6b0aa5fd8e532cb25629f2 commit 3c9b5b19ed1a61ebea6b0aa5fd8e532cb25629f2 Author: Clemens Hammacher <clemensh@chromium.org> Date: Tue Jul 18 06:51:47 2017 Merged: [wasm] Update signature map on indirect calls Revision: 883db26e6f43cbd027a3fda11e7c0bf9d9f3b481 NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=ahaas@chromium.org Bug: chromium:741750 Change-Id: I58cc5d341526581b369d0de864ada2c09d832e96 Reviewed-on: https://chromium-review.googlesource.com/575927 Reviewed-by: Andreas Haas <ahaas@chromium.org> Cr-Commit-Position: refs/branch-heads/6.0@{#79} Cr-Branched-From: 97dbf624a5eeffb3a8df36d24cdb2a883137385f-refs/heads/6.0.286@{#1} Cr-Branched-From: 12e6f1cb5cd9616da7b9d4a7655c088778a6d415-refs/heads/master@{#45439} [modify] https://crrev.com/3c9b5b19ed1a61ebea6b0aa5fd8e532cb25629f2/src/compiler/wasm-compiler.cc [modify] https://crrev.com/3c9b5b19ed1a61ebea6b0aa5fd8e532cb25629f2/src/wasm/wasm-module.cc [modify] https://crrev.com/3c9b5b19ed1a61ebea6b0aa5fd8e532cb25629f2/test/mjsunit/wasm/indirect-tables.js
,
Jul 18 2017
,
Jul 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/0725ff15e71ccc76b55ae82ed8d02c670b0daaaa commit 0725ff15e71ccc76b55ae82ed8d02c670b0daaaa Author: Clemens Hammacher <clemensh@chromium.org> Date: Tue Jul 18 07:20:19 2017 [wasm] Make signature map move-only Signature maps should only be updated, but never copied. We had a bug because we accidentally updated a copy of the map. This refactoring prevents any such bugs in the future, and fixes more occurences where we accidentally copied structs containing a signature map (the move-only constraint also extends to all structs containing a signature map). Drive-by: Make InstanceBuilder::NeedsWrappers const. R=titzer@chromium.org Bug: chromium:741750 Change-Id: Id919203d8c4078e608a1163e5c790c97d06a9753 Reviewed-on: https://chromium-review.googlesource.com/571791 Reviewed-by: Ben Titzer <titzer@chromium.org> Commit-Queue: Clemens Hammacher <clemensh@chromium.org> Cr-Commit-Position: refs/heads/master@{#46728} [modify] https://crrev.com/0725ff15e71ccc76b55ae82ed8d02c670b0daaaa/src/wasm/module-compiler.cc [modify] https://crrev.com/0725ff15e71ccc76b55ae82ed8d02c670b0daaaa/src/wasm/module-compiler.h [modify] https://crrev.com/0725ff15e71ccc76b55ae82ed8d02c670b0daaaa/src/wasm/module-decoder.cc [modify] https://crrev.com/0725ff15e71ccc76b55ae82ed8d02c670b0daaaa/src/wasm/signature-map.h [modify] https://crrev.com/0725ff15e71ccc76b55ae82ed8d02c670b0daaaa/src/wasm/wasm-module.h [modify] https://crrev.com/0725ff15e71ccc76b55ae82ed8d02c670b0daaaa/test/cctest/wasm/wasm-run-utils.h [modify] https://crrev.com/0725ff15e71ccc76b55ae82ed8d02c670b0daaaa/test/unittests/wasm/function-body-decoder-unittest.cc
,
Jul 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/529e3d035f775a27b406b1aff4be40d5267b9516 commit 529e3d035f775a27b406b1aff4be40d5267b9516 Author: Clemens Hammacher <clemensh@chromium.org> Date: Tue Jul 18 07:30:32 2017 Revert "Merged: [wasm] Update signature map on indirect calls" This reverts commit 3c9b5b19ed1a61ebea6b0aa5fd8e532cb25629f2. Reason for revert: New regression test fails because of non-existing method on the WasmModuleBuilder. Original change's description: > Merged: [wasm] Update signature map on indirect calls > > Revision: 883db26e6f43cbd027a3fda11e7c0bf9d9f3b481 > > NOTRY=true > NOPRESUBMIT=true > NOTREECHECKS=true > R=ahaas@chromium.org > > Bug: chromium:741750 > Change-Id: I58cc5d341526581b369d0de864ada2c09d832e96 > Reviewed-on: https://chromium-review.googlesource.com/575927 > Reviewed-by: Andreas Haas <ahaas@chromium.org> > Cr-Commit-Position: refs/branch-heads/6.0@{#79} > Cr-Branched-From: 97dbf624a5eeffb3a8df36d24cdb2a883137385f-refs/heads/6.0.286@{#1} > Cr-Branched-From: 12e6f1cb5cd9616da7b9d4a7655c088778a6d415-refs/heads/master@{#45439} TBR=ahaas@chromium.org,clemensh@chromium.org Change-Id: Ie5eb187080c2b66a889c3b8a40c37d03733526cd No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: chromium:741750 Reviewed-on: https://chromium-review.googlesource.com/575967 Reviewed-by: Clemens Hammacher <clemensh@chromium.org> Commit-Queue: Clemens Hammacher <clemensh@chromium.org> Cr-Commit-Position: refs/branch-heads/6.0@{#81} Cr-Branched-From: 97dbf624a5eeffb3a8df36d24cdb2a883137385f-refs/heads/6.0.286@{#1} Cr-Branched-From: 12e6f1cb5cd9616da7b9d4a7655c088778a6d415-refs/heads/master@{#45439} [modify] https://crrev.com/529e3d035f775a27b406b1aff4be40d5267b9516/src/compiler/wasm-compiler.cc [modify] https://crrev.com/529e3d035f775a27b406b1aff4be40d5267b9516/src/wasm/wasm-module.cc [modify] https://crrev.com/529e3d035f775a27b406b1aff4be40d5267b9516/test/mjsunit/wasm/indirect-tables.js
,
Jul 18 2017
Merge reverted, need to adapt the new mjsunit test.
,
Jul 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/2f221d6a7b7dc34f9386e8615c9a62bef4944280 commit 2f221d6a7b7dc34f9386e8615c9a62bef4944280 Author: Clemens Hammacher <clemensh@chromium.org> Date: Tue Jul 18 07:51:18 2017 Merged: [wasm] Update signature map on indirect calls Revision: 883db26e6f43cbd027a3fda11e7c0bf9d9f3b481 NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=ahaas@chromium.org Bug: chromium:741750 Change-Id: Ib63977a7aa2da786e40e517002dba50d94fb75cc Reviewed-on: https://chromium-review.googlesource.com/574911 Reviewed-by: Andreas Haas <ahaas@chromium.org> Cr-Commit-Position: refs/branch-heads/6.0@{#83} Cr-Branched-From: 97dbf624a5eeffb3a8df36d24cdb2a883137385f-refs/heads/6.0.286@{#1} Cr-Branched-From: 12e6f1cb5cd9616da7b9d4a7655c088778a6d415-refs/heads/master@{#45439} [modify] https://crrev.com/2f221d6a7b7dc34f9386e8615c9a62bef4944280/src/compiler/wasm-compiler.cc [modify] https://crrev.com/2f221d6a7b7dc34f9386e8615c9a62bef4944280/src/wasm/wasm-module.cc [modify] https://crrev.com/2f221d6a7b7dc34f9386e8615c9a62bef4944280/test/mjsunit/wasm/indirect-tables.js
,
Jul 18 2017
Second merge looks good.
,
Jul 24 2017
,
Oct 22 2017
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 raymes@chromium.org
, Jul 13 2017Owner: clemensh@chromium.org
Status: Assigned (was: Available)