Security: A bug in JSFunction::GetDerivedMap |
||||||||||||||||||||
Issue descriptionFrom https://bugs.chromium.org/p/chromium/issues/detail?id=800032 There's a comment saying "Fast case, new.target is a subclass of constructor." in the JSFunction::GetDerivedMap method, but it doesn't check that "new_target" is actually a subclass of "constructor". So if "new_target" is not a subclass of "constructor", "pre_allocated" can be higher than "in_object_properties". To exploit this, it required to be able to change the value of "constructor_initial_map->UnusedPropertyFields()", but I couldn't find any way. So I'm not so sure about this part. PoC: function gc() { for (let i = 0; i < 20; i++) new ArrayBuffer(0x2000000); } class Derived extends Array { constructor(a) { const a = 1; } } // Derived is not a subclass of RegExp let o = Reflect.construct(RegExp, [], Derived); o.lastIndex = 0x1234; gc();
,
Jan 29 2018
Assigning to runtime folks, Igor PTAL.
,
Jan 30 2018
So, potential type confusion, leading to OOB write? Do I have that right? If confirmed, that would be High.
,
Jan 30 2018
Having a look at this right now.
,
Jan 30 2018
I tentatively landed https://crrev.com/c/866931 earlier today which turns a couple of DCHECKs on Map into hard CHECKS. This does catch this bug and crashes hard. Now it's time to turn this into a proper runtime error and address the underlying issue. [runtime] Harden some Map setters Convert certain DCHECKS into CHECKS for some Map setters. This should have minimal performance impact at the same time getting us better coverage out there in the wild. Change-Id: I9a12f43e1baca15d9bf8b1aed86bb6b0dc13921d Reviewed-on: https://chromium-review.googlesource.com/866931 Commit-Queue: Camillo Bruni <cbruni@chromium.org> Reviewed-by: Igor Sheludko <ishell@chromium.org> Cr-Commit-Position: refs/heads/master@{#50958}
,
Jan 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/8361fa5896450e099539668cd7c637d01923d4ef commit 8361fa5896450e099539668cd7c637d01923d4ef Author: Camillo Bruni <cbruni@chromium.org> Date: Wed Jan 31 12:07:56 2018 [runtime] Fix derived class instantiation Bug: chromium:806388 Change-Id: Ieb343f0d532c16b6102e85222b77713f23bacf8c Reviewed-on: https://chromium-review.googlesource.com/894942 Reviewed-by: Igor Sheludko <ishell@chromium.org> Commit-Queue: Camillo Bruni <cbruni@chromium.org> Cr-Commit-Position: refs/heads/master@{#50990} [modify] https://crrev.com/8361fa5896450e099539668cd7c637d01923d4ef/src/objects.cc [modify] https://crrev.com/8361fa5896450e099539668cd7c637d01923d4ef/src/objects.h [add] https://crrev.com/8361fa5896450e099539668cd7c637d01923d4ef/test/mjsunit/regress/regress-crbug-806388.js
,
Jan 31 2018
#6: Is this a complete fix, or is there more work to be done?
,
Jan 31 2018
,
Jan 31 2018
The commit mentioned in #5 essentially prevents any exploit. #6 should fix the underlying issue. I'm travelling tomorrow and don't have time to properly look into this until monday. I'd like to properly reason about this to see whether the issue is fully resolved (maybe ishell@ has some time left on THU+FRI). As per separate discussions, ishell will do the backmerge while I'm OOO, most probably on friday.
,
Feb 5 2018
Going to backmerge CL from #5 to both M65 / M64 today. #6 is up for discussion, should be fine though.
,
Feb 5 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), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 5 2018
,
Feb 5 2018
Pls merge your change to M65 before 1:00 PM PT today, Monday (02/05/18) so we can pick it up for dev release on Tuesday. Thank you.
,
Feb 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/8f6a5ac6ec08583138821648aacc184767e29faa commit 8f6a5ac6ec08583138821648aacc184767e29faa Author: Camillo Bruni <cbruni@chromium.org> Date: Mon Feb 05 18:04:08 2018 Merged: Squashed multiple commits. Merged: [runtime] Harden some Map setters Revision: 36d3ec Merged: [runtime] Fix derived class instantiation Revision: 8361fa BUG= chromium:806388 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=ishell@chromium.org Change-Id: I7b75565023c447e0e1ed505b8df478972f2a2b27 Reviewed-on: https://chromium-review.googlesource.com/902202 Reviewed-by: Igor Sheludko <ishell@chromium.org> Cr-Commit-Position: refs/branch-heads/6.5@{#27} Cr-Branched-From: 73c55f57fe8506011ff854b15026ca765b669700-refs/heads/6.5.254@{#1} Cr-Branched-From: 594a1a0b6e551397cfdf50870f6230da34db2dc8-refs/heads/master@{#50664} [modify] https://crrev.com/8f6a5ac6ec08583138821648aacc184767e29faa/src/objects.cc [modify] https://crrev.com/8f6a5ac6ec08583138821648aacc184767e29faa/src/objects.h [modify] https://crrev.com/8f6a5ac6ec08583138821648aacc184767e29faa/src/objects/map-inl.h [add] https://crrev.com/8f6a5ac6ec08583138821648aacc184767e29faa/test/mjsunit/regress/regress-crbug-806388.js
,
Feb 5 2018
Per comment #14, this is already merge to M65. If nothing is pending for M65, pls remove "Merge-Approved-65" label. Thank you.
,
Feb 5 2018
,
Feb 8 2018
Looks fixed. cbruni@, please reopen if there are more fixes intended here.
,
Feb 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/1534290a2974b856d713b8ef9f6c063e279e48db commit 1534290a2974b856d713b8ef9f6c063e279e48db Author: Camillo Bruni <cbruni@chromium.org> Date: Thu Feb 08 07:48:35 2018 Merged: Squashed multiple commits. Merged: [runtime] Harden some Map setters Revision: 36d3ec Merged: [runtime] Fix derived class instantiation Revision: 8361fa BUG= chromium:806388 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=ishell@chromium.org Change-Id: I1a8932993f6454c1d7e3380d435aa9138505beba Reviewed-on: https://chromium-review.googlesource.com/902243 Reviewed-by: Igor Sheludko <ishell@chromium.org> Commit-Queue: Igor Sheludko <ishell@chromium.org> Cr-Commit-Position: refs/branch-heads/6.4@{#90} Cr-Branched-From: 0407506af3d9d7e2718be1d8759296165b218fcf-refs/heads/6.4.388@{#1} Cr-Branched-From: a5fc4e085ee543cb608eb11034bc8f147ba388e1-refs/heads/master@{#49724} [modify] https://crrev.com/1534290a2974b856d713b8ef9f6c063e279e48db/src/objects-inl.h [modify] https://crrev.com/1534290a2974b856d713b8ef9f6c063e279e48db/src/objects.cc [modify] https://crrev.com/1534290a2974b856d713b8ef9f6c063e279e48db/src/objects.h [add] https://crrev.com/1534290a2974b856d713b8ef9f6c063e279e48db/test/mjsunit/regress/regress-crbug-806388.js
,
Feb 8 2018
,
Feb 8 2018
,
Feb 13 2018
,
Feb 13 2018
,
Apr 25 2018
,
May 17 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
,
Jun 12 2018
Needed for Node 8 and Node 9. I am handling the backport.
,
Jun 12 2018
Node backports * 8.x: https://github.com/nodejs/node/pull/21294 * 9.x: https://github.com/nodejs/node/pull/21293
,
Jan 4
|
||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||
Comment 1 by mea...@chromium.org
, Jan 26 2018Components: Blink>JavaScript
Owner: jgruber@chromium.org
Status: Assigned (was: Unconfirmed)