Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: V8: backport f320600cd1f4 (V18.x CVE-2024-4761) #54597

Closed

Conversation

giancorderoortiz
Copy link

V8 Backport of v8/v8@f320600

Fixes CVE-2024-4761, which has been tagged by CISA as KEV.

Original commit message:

    [wasm-gc] Only normalize JSObject targets in SetOrCopyDataProperties

    Bug: 339458194
    Change-Id: I4d6eebdd921971fa28d7c474535d978900ba633f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5527397
    Reviewed-by: Rezvan Mahdavi Hezaveh <[email protected]>
    Commit-Queue: Shu-yu Guo <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#93811}

Refs: v8/v8@f320600
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. v8 engine Issues and PRs related to the V8 dependency. labels Aug 27, 2024
@targos
Copy link
Member

targos commented Aug 27, 2024

Any reason to believe this is needed? wasm-gc has not been enabled in V8 until very recently (end of 2023)

@RedYetiDev RedYetiDev added the security Issues and PRs related to security. label Aug 27, 2024
@giancorderoortiz
Copy link
Author

Latest Node 18 minor version has V8 10.2.154.26. See https://github.com/nodejs/node/blob/v18.20.4/deps/v8/include/v8-version.h
CVE-2024-4761 fixed in V8 version 12.6.213 by v8/v8@f320600
Hence the need for a backport.

@targos
Copy link
Member

targos commented Aug 27, 2024

My point is that wasm-gc (the feature that the commit targets), was not enabled in V8 10.2. So that version cannot be affected.

@giancorderoortiz giancorderoortiz changed the title deps: V8: backport f320600cd1f4 (CVE-2024-4761) deps: V8: backport f320600cd1f4 (V18.x CVE-2024-4761) Aug 27, 2024
@RedYetiDev RedYetiDev added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 28, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unfortunately needed to please the vulnerability scanners. We stated previously that while this is not a vulnerability for Node.js, we would accept a backport if somebody would do it. (As stated, this is work for the sake of work created by the vulnerability scanners and company policies).

lgtm

@targos
Copy link
Member

targos commented Aug 28, 2024

@mcollina this doesn't even build. I'm sorry but I refuse to spend voluntary time fixing stuff for broken payed security tools.

@targos targos closed this Aug 28, 2024
@targos targos reopened this Aug 28, 2024
@targos
Copy link
Member

targos commented Aug 28, 2024

@giancorderoortiz I appreciate the effort you made, this is not your fault

@targos
Copy link
Member

targos commented Aug 28, 2024

I'd also like to understand something: let's say that we successfully (and meaningfully) backport a V8 CVE fix into v18.x. What mechanism will make the security tools stop flagging Node.js as vulnerable?

@mcollina
Copy link
Member

I'm sorry but I refuse to spend voluntary time fixing stuff for broken payed security tools.

As said, this is up to @giancorderoortiz and their employer to fix if they want to, not us.

(So no, don't spend any time on it @targos).

@mcollina
Copy link
Member

What mechanism will make the security tools stop flagging Node.js as vulnerable?

I actually have no clue. Likely they can put a checkmark in a spreadsheet that this was mitigated. It is the unfortunate reality of the enterprise world.

If any of my current or past employers would have needed something like this under a support contract, I would have obliged, to the point of sending a PR or creating a build of Node just for them.

Frankly, dealing with these kind of things is why we are enrolling in HeroDevs.

@targos
Copy link
Member

targos commented Aug 29, 2024

I'm one of the few active releasers. Creating a release is a lot of time.

@targos
Copy link
Member

targos commented Aug 29, 2024

We should also take into account the risk of backporting V8 fixes. This very pull request shows that it is non trivial and while here it's obvious because the build is broken, another change might build fine but break the system in unexpected ways. I don't know if anyone in the active collaborator base knows V8 good enough to properly review these changes

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 29, 2024
@mcollina
Copy link
Member

I can't find the issue atm, but I recall that @RafaelGSS mentioned he would assemble a release.

Original commit message:

    Merged: [parser] Using FunctionParsingScope for parsing class static blocks

    Class static blocks contain statements, don't inherit the
    ExpressionScope stack.

    (cherry picked from commit 3e037e195e508dea045f5626862412e8f64fc919)

    Bug: 341663589
    Change-Id: Ice9a710293b028e5d9fd30d5d85c4842f970b152
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5558360
    Reviewed-by: Adam Klein <[email protected]>
    Reviewed-by: Shu-yu Guo <[email protected]>
    Commit-Queue: Adam Klein <[email protected]>
    Cr-Commit-Position: refs/branch-heads/12.4@{nodejs#38}
    Cr-Branched-From: 309640da62fae0485c7e4f64829627c92d53b35d-refs/heads/12.4.254@{nodejs#1}
    Cr-Branched-From: 5dc24701432278556a9829d27c532f974643e6df-refs/heads/main@{#92862}

Refs: v8/v8@6e5e105
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mhdawson mhdawson removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 11, 2024
@mcollina
Copy link
Member

The outcome of the TSC meeting is that, unfortunately, we are not planning to accept this backport due to compatibility risk as this will need to go to maintenance release.

Thanks anyway.

@mcollina mcollina closed this Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. security Issues and PRs related to security. v8 engine Issues and PRs related to the V8 dependency. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants