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

[v10.x] deps: cherry-pick V8 changes to extend hash seed to 64-bit #23260

Closed
wants to merge 1 commit into from

Conversation

hashseed
Copy link
Member

@hashseed hashseed commented Oct 4, 2018

This serves as mitigation for the so-called HashWick vulnerability.

Original commit messages:

  commit d5686a74d56fbb6985b22663ddadd66eb7b91519
    Author: Yang Guo <yangguo@chromium.org>
    Date: Mon Jul 16 11:19:42 2018

    Extend hash seed to 64 bits

    R=bmeurer@chromium.org, ulan@chromium.org

    Bug: chromium:680662
    Change-Id: I5e1486ad2a42db2998d5485a0c4e711378678e6c
    Reviewed-on: https://chromium-review.googlesource.com/1136034
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54460}

  commit 3833fef57368c53c6170559ffa524c8c69f16ee5
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 20 11:43:13 2018

    Refactor integer hashing function names

    We now clearly differentiate between:
    - unseeded hash for 32-bit integers
    - unseeded hash for 64-bit integers
    - seeded hash for 32-bit integers
    - seeded hash for strings

    R=bmeurer@chromium.org

    Bug: chromium:680662
    Change-Id: I7459958c4158ee3501c962943dff8f33258bb5ce
    Reviewed-on: https://chromium-review.googlesource.com/1235973
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#56068}

  commit 95a979e02d7154e45b293261a6998c99d71fc238
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 20 14:34:48 2018

    Call into C++ to compute seeded integer hash

    R=bmeurer@chromium.org

    Bug: chromium:680662
    Change-Id: I8dace89d576dfcc5833fd539ce698a9ade1cb5a0
    Reviewed-on: https://chromium-review.googlesource.com/1235928
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#56091}

  commit 2c2af0022d5feb9e525a00a76cb15db9f3e38dba
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 27 16:37:57 2018

    Use 64-bit for seeded integer hashes

    R=petermarshall@chromium.org

    Bug: chromium:680662
    Change-Id: If48d1043dbe1e1bb695ec890c23e103a6cacf2d4
    Reviewed-on: https://chromium-review.googlesource.com/1244220
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#56271}

Refs: #23259

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added v10.x v8 engine Issues and PRs related to the V8 dependency. labels Oct 4, 2018
@hashseed
Copy link
Member Author

hashseed commented Oct 4, 2018

I swear I didn't plan this when I chose my github/twitter user name :)

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.

LGTM

@mcollina
Copy link
Member

mcollina commented Oct 4, 2018

@mcollina
Copy link
Member

mcollina commented Oct 4, 2018

@hashseed can you confirm this is already on master?

@hashseed
Copy link
Member Author

hashseed commented Oct 4, 2018

This is not yet on master, since the last in this series of commits landed just a week ago in upstream V8. Should I float this on master as well?

@mcollina
Copy link
Member

mcollina commented Oct 4, 2018

Yes definitely. This should be included in 11 (#23230).

cc @jasnell

@richardlau
Copy link
Member

'v8_embedder_string' in common.gypi should be bumped.

@hashseed
Copy link
Member Author

hashseed commented Oct 4, 2018

'v8_embedder_string' in common.gypi should be bumped.

Thanks. Done.

Yes definitely. This should be included in 11 (#23230).

There you go: #23264

@addaleax addaleax added the security Issues and PRs related to security. label Oct 4, 2018
@addaleax addaleax changed the title deps: cherry-pick V8 changes to extend hash seed to 64-bit [v10.x] deps: cherry-pick V8 changes to extend hash seed to 64-bit Oct 4, 2018
Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

LGTM w/ minor nit: the commit abstract can be shortened to the preferred guidelines. I suggest something like: deps: V8: cherry-pick 64-bit hash seed commits. This can happen at landing time, and the second commit squashed at that time.

This serves as mitigation for the so-called HashWick vulnerability.

Original commit messages:

  commit d5686a74d56fbb6985b22663ddadd66eb7b91519
    Author: Yang Guo <yangguo@chromium.org>
    Date: Mon Jul 16 11:19:42 2018

    Extend hash seed to 64 bits

    R=bmeurer@chromium.org, ulan@chromium.org

    Bug: chromium:680662
    Change-Id: I5e1486ad2a42db2998d5485a0c4e711378678e6c
    Reviewed-on: https://chromium-review.googlesource.com/1136034
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#54460}

  commit 3833fef57368c53c6170559ffa524c8c69f16ee5
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 20 11:43:13 2018

    Refactor integer hashing function names

    We now clearly differentiate between:
    - unseeded hash for 32-bit integers
    - unseeded hash for 64-bit integers
    - seeded hash for 32-bit integers
    - seeded hash for strings

    R=bmeurer@chromium.org

    Bug: chromium:680662
    Change-Id: I7459958c4158ee3501c962943dff8f33258bb5ce
    Reviewed-on: https://chromium-review.googlesource.com/1235973
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#56068}

  commit 95a979e02d7154e45b293261a6998c99d71fc238
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 20 14:34:48 2018

    Call into C++ to compute seeded integer hash

    R=bmeurer@chromium.org

    Bug: chromium:680662
    Change-Id: I8dace89d576dfcc5833fd539ce698a9ade1cb5a0
    Reviewed-on: https://chromium-review.googlesource.com/1235928
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#56091}

  commit 2c2af0022d5feb9e525a00a76cb15db9f3e38dba
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 27 16:37:57 2018

    Use 64-bit for seeded integer hashes

    R=petermarshall@chromium.org

    Bug: chromium:680662
    Change-Id: If48d1043dbe1e1bb695ec890c23e103a6cacf2d4
    Reviewed-on: https://chromium-review.googlesource.com/1244220
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#56271}

Refs: nodejs#23259
@hashseed
Copy link
Member Author

hashseed commented Oct 5, 2018

Squashed and reworded the commit message.

targos pushed a commit that referenced this pull request Oct 6, 2018
This serves as mitigation for the so-called HashWick vulnerability.

Original commit messages:

  commit d5686a74d56fbb6985b22663ddadd66eb7b91519
    Author: Yang Guo <yangguo@chromium.org>
    Date: Mon Jul 16 11:19:42 2018

    Extend hash seed to 64 bits

    R=bmeurer@chromium.org, ulan@chromium.org

    Bug: chromium:680662
    Change-Id: I5e1486ad2a42db2998d5485a0c4e711378678e6c
    Reviewed-on: https://chromium-review.googlesource.com/1136034
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54460}

  commit 3833fef57368c53c6170559ffa524c8c69f16ee5
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 20 11:43:13 2018

    Refactor integer hashing function names

    We now clearly differentiate between:
    - unseeded hash for 32-bit integers
    - unseeded hash for 64-bit integers
    - seeded hash for 32-bit integers
    - seeded hash for strings

    R=bmeurer@chromium.org

    Bug: chromium:680662
    Change-Id: I7459958c4158ee3501c962943dff8f33258bb5ce
    Reviewed-on: https://chromium-review.googlesource.com/1235973
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#56068}

  commit 95a979e02d7154e45b293261a6998c99d71fc238
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 20 14:34:48 2018

    Call into C++ to compute seeded integer hash

    R=bmeurer@chromium.org

    Bug: chromium:680662
    Change-Id: I8dace89d576dfcc5833fd539ce698a9ade1cb5a0
    Reviewed-on: https://chromium-review.googlesource.com/1235928
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#56091}

  commit 2c2af0022d5feb9e525a00a76cb15db9f3e38dba
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 27 16:37:57 2018

    Use 64-bit for seeded integer hashes

    R=petermarshall@chromium.org

    Bug: chromium:680662
    Change-Id: If48d1043dbe1e1bb695ec890c23e103a6cacf2d4
    Reviewed-on: https://chromium-review.googlesource.com/1244220
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#56271}

Refs: #23259

PR-URL: #23260
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@targos
Copy link
Member

targos commented Oct 6, 2018

Landed in c6a2f4a.

@targos targos closed this Oct 6, 2018
targos pushed a commit that referenced this pull request Oct 7, 2018
This serves as mitigation for the so-called HashWick vulnerability.

Original commit messages:

  commit d5686a74d56fbb6985b22663ddadd66eb7b91519
    Author: Yang Guo <yangguo@chromium.org>
    Date: Mon Jul 16 11:19:42 2018

    Extend hash seed to 64 bits

    R=bmeurer@chromium.org, ulan@chromium.org

    Bug: chromium:680662
    Change-Id: I5e1486ad2a42db2998d5485a0c4e711378678e6c
    Reviewed-on: https://chromium-review.googlesource.com/1136034
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54460}

  commit 3833fef57368c53c6170559ffa524c8c69f16ee5
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 20 11:43:13 2018

    Refactor integer hashing function names

    We now clearly differentiate between:
    - unseeded hash for 32-bit integers
    - unseeded hash for 64-bit integers
    - seeded hash for 32-bit integers
    - seeded hash for strings

    R=bmeurer@chromium.org

    Bug: chromium:680662
    Change-Id: I7459958c4158ee3501c962943dff8f33258bb5ce
    Reviewed-on: https://chromium-review.googlesource.com/1235973
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#56068}

  commit 95a979e02d7154e45b293261a6998c99d71fc238
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 20 14:34:48 2018

    Call into C++ to compute seeded integer hash

    R=bmeurer@chromium.org

    Bug: chromium:680662
    Change-Id: I8dace89d576dfcc5833fd539ce698a9ade1cb5a0
    Reviewed-on: https://chromium-review.googlesource.com/1235928
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#56091}

  commit 2c2af0022d5feb9e525a00a76cb15db9f3e38dba
    Author: Yang Guo <yangguo@chromium.org>
    Date: Thu Sep 27 16:37:57 2018

    Use 64-bit for seeded integer hashes

    R=petermarshall@chromium.org

    Bug: chromium:680662
    Change-Id: If48d1043dbe1e1bb695ec890c23e103a6cacf2d4
    Reviewed-on: https://chromium-review.googlesource.com/1244220
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#56271}

Refs: #23259

PR-URL: #23260
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@hashseed hashseed deleted the 64bithash branch November 20, 2018 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Issues and PRs related to security. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants