Skip to content

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

Closed
hashseed wants to merge 1 commit into
nodejs:v10.x-stagingfrom
hashseed:64bithash
Closed

[v10.x] deps: cherry-pick V8 changes to extend hash seed to 64-bit#23260
hashseed wants to merge 1 commit into
nodejs:v10.x-stagingfrom
hashseed:64bithash

Conversation

@hashseed

@hashseed hashseed commented Oct 4, 2018

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Collaborator

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

hashseed commented Oct 4, 2018

Copy link
Copy Markdown
Member Author

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

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@mcollina

mcollina commented Oct 4, 2018

Copy link
Copy Markdown
Member

@mcollina

mcollina commented Oct 4, 2018

Copy link
Copy Markdown
Member

@hashseed can you confirm this is already on master?

@hashseed

hashseed commented Oct 4, 2018

Copy link
Copy Markdown
Member Author

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

mcollina commented Oct 4, 2018

Copy link
Copy Markdown
Member

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

cc @jasnell

@richardlau

Copy link
Copy Markdown
Member

'v8_embedder_string' in common.gypi should be bumped.

@hashseed

hashseed commented Oct 4, 2018

Copy link
Copy Markdown
Member Author

'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

@ofrobots ofrobots left a comment

Copy link
Copy Markdown
Contributor

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

hashseed commented Oct 5, 2018

Copy link
Copy Markdown
Member Author

Squashed and reworded the commit message.

@targos

targos commented Oct 5, 2018

Copy link
Copy Markdown
Member

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

targos commented Oct 6, 2018

Copy link
Copy Markdown
Member

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>
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