New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 806388 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: A bug in JSFunction::GetDerivedMap

Project Member Reported by lokihardt@google.com, Jan 26 2018

Issue description

From 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();
 

Comment 1 by mea...@chromium.org, Jan 26 2018

Cc: -jgruber@chromium.org
Components: Blink>JavaScript
Owner: jgruber@chromium.org
Status: Assigned (was: Unconfirmed)
Cc: verwa...@chromium.org cbruni@chromium.org jgruber@chromium.org
Owner: ishell@chromium.org
Assigning to runtime folks, Igor PTAL.

Comment 3 by palmer@chromium.org, Jan 30 2018

Cc: hpayer@chromium.org
Labels: M-65 Security_Impact-Stable OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
So, potential type confusion, leading to OOB write? Do I have that right? If confirmed, that would be High.

Comment 4 by cbruni@chromium.org, Jan 30 2018

Having a look at this right now.

Comment 5 by cbruni@chromium.org, 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}
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Comment 7 by palmer@chromium.org, Jan 31 2018

#6: Is this a complete fix, or is there more work to be done?

Comment 8 by palmer@chromium.org, Jan 31 2018

Labels: Security_Severity-High

Comment 9 by cbruni@chromium.org, 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.

Labels: Merge-Request-65 Merge-Request-64
Going to backmerge CL from #5 to both M65 / M64 today. #6 is up for discussion, should be fine though.
Project Member

Comment 11 by sheriffbot@chromium.org, Feb 5 2018

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
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
Labels: -Merge-Review-64 -Merge-Request-65 Merge-Approved-65 Merge-Approved-64
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.

Project Member

Comment 14 by bugdroid1@chromium.org, Feb 5 2018

Labels: merge-merged-6.5
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

Per comment #14, this is already merge to M65. If nothing is pending for M65, pls remove "Merge-Approved-65" label. Thank you.
Labels: -Merge-Approved-65
Status: Fixed (was: Assigned)
Looks fixed. cbruni@, please reopen if there are more fixes intended here.
Project Member

Comment 18 by bugdroid1@chromium.org, Feb 8 2018

Labels: merge-merged-6.4
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

Labels: -Merge-Approved-64
Project Member

Comment 20 by sheriffbot@chromium.org, Feb 8 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: CVE-2018-6056
Labels: Release-2-M64
Labels: CVE_description-missing
Project Member

Comment 24 by sheriffbot@chromium.org, May 17 2018

Labels: -Restrict-View-SecurityNotify allpublic
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
Labels: NodeJS-Backport-Approved
Needed for Node 8 and Node 9. I am handling the backport.
Labels: -NodeJS-Backport-Approved NodeJS-Backport-Done
Node backports
* 8.x: https://github.com/nodejs/node/pull/21294
* 9.x: https://github.com/nodejs/node/pull/21293
Labels: -CVE_description-missing CVE_description-submitted

Sign in to add a comment