build: enable v8's SipHash for hash seed creation by rvagg · Pull Request #26367 · nodejs/node · GitHub

Triggers the V8_USE_SIPHASH to switch from the internal custom V8 hash seed generation function to an implementation of SipHash. Final step needed to clear up HashWick.

Ref: #23259
Ref: https://darksi.de/12.hashwick-v8-vulnerability/

This could arguably be semver-minor because it introduces a configure flag, but that doesn't show in --help. I'm also unsure if this has any impact on snapshots, could it be breaking for people using snapshots for fast startup? (Electron projects do, don't they?).

@rvagg build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2721/pipeline

nice

If we are enabling siphash by default, should we acknowledge its LICENSE anywhere?

Good question @richardlau, I would say that V8 needs to do that in deps/v8/LICENSE and then when we run ./tools/license-builder.sh it'll be pulled in. Feel like raising an issue against V8?

LGTM.
probably should bump patch counter at
ht tps://github.com/nodejs/node/blob/584305841d0fabee5d96ae43badfa271da99a19f/common.gypi#L40

Ohh. We've been missing this with some of the other V8 new features...
/me writes himself a TODO note

does this warrant a patch bump? I don't know the rules there

does this warrant a patch bump? I don't know the rules there

@targos ?

P.S. Up till now we did bump v8_embedder_string for gypfiles changes, even-tough we actually need not...
We (read I) should really move those out of /deps/v8.

Good question @richardlau, I would say that V8 needs to do that in deps/v8/LICENSE and then when we run ./tools/license-builder.sh it'll be pulled in. Feel like raising an issue against V8?

If I'm reading https://github.com/nodejs/node/pull/25430/files#diff-75477e7cdd271be767e5255488ca45e1 correctly, siphash is not enabled by default in V8.

@richardlau yes that's correct, I suspect because it'd be impractical, maybe impossible, to generate enough data to calculate the hash in a browser environment, especially now that they've got 64-bit seed length. It's slightly more practical when you have a server that will repeatedly answer you for hours on end.
There's may be a performance cost we're going to incur here that Chromium doesn't need/want as well. @hashseed might be able to edify us.

I'm in favor of not bumping the patch level when only gypfiles are changed

Hash flooding is not considered an vulnerability in the browser's threat model. A browser needs to deal with arbitrary, possibly malicious code, but does not care much about a tab locking up. An infinite loop is a lot simpler, with the same result.

V8 does not use it by default, and since it's a build-time flag, does not ship the code. That's why I included the v8/src/third_party/siphash/LICENSE, but have not actually pointed to it from anywhere. I can rectify this if anyone has strong opinions on this.

I got some numbers here. The current implementation is referred to as "implementation 1".

For posterity: this could be just o['variables']['v8_use_siphash'] = b(options.without_siphash) but it's fine, it's still locally consistent.

thanks for the tip, we could do a lot of simplification with that but I'll withhold this time

Do we even need a ./configure flag?
IIUC this is an obscure enough configuration that doing:

./configure -- -Dv8_use_siphash=false

seems good enough

It can be removed in a future version when someone's cleaning up. For now if it ends up causing problems, performance or backward compatibility, or whatever, at least they have an easy out

Thanks for the info @hashseed (and thanks for the hard work on this btw). I can get similar performance numbers using the benchmark in your doc with this turned on and off in Node, so at least I know it's enabled properly. I'm having a hard time finding anything in our benchmark suite that displays a meaningful difference though—there's a lot of I/O focus in our benchmarks and those that aren't I/O touch fairly deep code paths so are getting a lot of variety. Hopefully the overhead gets lost in the wash and will only hit a small subset of users.

I've included the SipHash LICENSE in ours, unfortunately the CC0 text is 3 times the size of the code we're importing.

IIUC CC0 is a "no-attribution no-copyright" license, which IMO makes explicitly including it counter productive.

Excellent!

I've included the SipHash LICENSE in ours, unfortunately the CC0 text is 3 times the size of the code we're importing.

IIUC CC0 is more of a "no copyright" waiver then a license, and as such does not require explicit attribution - https://wiki.creativecommons.org/wiki/CC0_FAQ#Does_CC0_require_others_who_use_my_work_to_give_me_attribution.3F

I think it should be removed from tools/license-builder.sh

I've removed the CC0 but left attribution, because we're good open source citizens:

- SipHash, located at deps/v8/src/third_party/siphash, is licensed as follows:
  """
    SipHash reference C implementation

    Copyright (c) 2016 Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com>

    To the extent possible under law, the author(s) have dedicated all
    copyright and related and neighboring rights to this software to the public
    domain worldwide. This software is distributed without any warranty.
  """

CI: https://ci.nodejs.org/job/node-test-pull-request/21122/

Landed in e1cd8ac...0d94c23

Triggers the V8_USE_SIPHASH to switch from the internal custom V8 hash seed generation function to an implementation of SipHash. Final step needed to clear up HashWick. PR-URL: #26367 Refs: #23259 Refs: https://darksi.de/12.hashwick-v8-vulnerability/ Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

PR-URL: #26367 Refs: #23259 Refs: https://darksi.de/12.hashwick-v8-vulnerability/ Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

Triggers the V8_USE_SIPHASH to switch from the internal custom V8 hash seed generation function to an implementation of SipHash. Final step needed to clear up HashWick. PR-URL: #26367 Refs: #23259 Refs: https://darksi.de/12.hashwick-v8-vulnerability/ Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

PR-URL: #26367 Refs: #23259 Refs: https://darksi.de/12.hashwick-v8-vulnerability/ Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

Notable Changes * build: * Enable v8's siphash for hash seed creation (Rod Vagg) #26367 * crypto: * Add `KeyObject.asymmetricKeySize` (Patrick Gansterer) #26387 * deps: * Upgrade openssl to 1.1.1b (Sam Roberts) #26327 * process: * Make `process[Symbol.toStringTag]` writable again (Ruben Bridgewater) #26488 * repl: * Add `util.inspect.replDefaults` to customize the writer (Ruben Bridgewater) #26375 * report: * Rename `triggerReport()` to `writeReport()` (Colin Ihrig) #26527

Triggers the V8_USE_SIPHASH to switch from the internal custom V8 hash seed generation function to an implementation of SipHash. Final step needed to clear up HashWick. PR-URL: #26367 Refs: #23259 Refs: https://darksi.de/12.hashwick-v8-vulnerability/ Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

PR-URL: #26367 Refs: #23259 Refs: https://darksi.de/12.hashwick-v8-vulnerability/ Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

Notable Changes * bootstrap: * Add experimental `--frozen-intrinsics` flag (Guy Bedford) #25685 * build: * Enable v8's siphash for hash seed creation (Rod Vagg) #26367 * deps: * Upgrade openssl to 1.1.1b (Sam Roberts) #26327 * process: * Make `process[Symbol.toStringTag]` writable again (Ruben Bridgewater) #26488 * repl: * Add `util.inspect.replDefaults` to customize the writer (Ruben Bridgewater) #26375 * report: * Rename `triggerReport()` to `writeReport()` (Colin Ihrig) #26527

Notable Changes * bootstrap: * Add experimental `--frozen-intrinsics` flag (Guy Bedford) nodejs#25685 * build: * Enable v8's siphash for hash seed creation (Rod Vagg) nodejs#26367 * deps: * Upgrade openssl to 1.1.1b (Sam Roberts) nodejs#26327 * process: * Make `process[Symbol.toStringTag]` writable again (Ruben Bridgewater) nodejs#26488 * repl: * Add `util.inspect.replDefaults` to customize the writer (Ruben Bridgewater) nodejs#26375 * report: * Rename `triggerReport()` to `writeReport()` (Colin Ihrig) nodejs#26527

Notable Changes * bootstrap: * Add experimental `--frozen-intrinsics` flag (Guy Bedford) #25685 * build: * Enable v8's siphash for hash seed creation (Rod Vagg) #26367 * deps: * Upgrade openssl to 1.1.1b (Sam Roberts) #26327 * process: * Make `process[Symbol.toStringTag]` writable again (Ruben Bridgewater) #26488 * repl: * Add `util.inspect.replDefaults` to customize the writer (Ruben Bridgewater) #26375 * report: * Rename `triggerReport()` to `writeReport()` (Colin Ihrig) #26527

Notable Changes * bootstrap: * Add experimental `--frozen-intrinsics` flag (Guy Bedford) nodejs#25685 * build: * Enable v8's siphash for hash seed creation (Rod Vagg) nodejs#26367 * deps: * Upgrade openssl to 1.1.1b (Sam Roberts) nodejs#26327 * process: * Make `process[Symbol.toStringTag]` writable again (Ruben Bridgewater) nodejs#26488 * repl: * Add `util.inspect.replDefaults` to customize the writer (Ruben Bridgewater) nodejs#26375 * report: * Rename `triggerReport()` to `writeReport()` (Colin Ihrig) nodejs#26527

PR-URL: #26367 Refs: #23259 Refs: https://darksi.de/12.hashwick-v8-vulnerability/ Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

Enable the v8_use_siphash flag. This flag is enabled since Node.js v11.12.0 (2019-03-15), see linked PR, release notes, and the blog post detailing the attack. Ref: nodejs/node#26367 Ref: https://nodejs.org/gl/blog/release/v11.12.0/ Ref: https://darksi.de/12.hashwick-v8-vulnerability/

rvagg requested review from hashseed, indutny and refack March 1, 2019 00:28

nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Mar 1, 2019

devsnek approved these changes Mar 1, 2019

View reviewed changes

richardlau approved these changes Mar 1, 2019

View reviewed changes

refack approved these changes Mar 1, 2019

View reviewed changes

refack requested review from ryzokuken and targos March 1, 2019 01:55

bnoordhuis approved these changes Mar 1, 2019

View reviewed changes

BridgeAR approved these changes Mar 1, 2019

View reviewed changes

refack reviewed Mar 1, 2019

View reviewed changes

indutny approved these changes Mar 1, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 2, 2019

jasnell approved these changes Mar 2, 2019

View reviewed changes

hashseed approved these changes Mar 2, 2019

View reviewed changes

rvagg force-pushed the rvagg/v8-siphash branch from 7047468 to a155578 Compare March 12, 2019 02:09

rvagg closed this Mar 12, 2019

rvagg deleted the rvagg/v8-siphash branch March 12, 2019 02:11

BridgeAR mentioned this pull request Mar 13, 2019

2019-03-14, Version 11.12.0 (Current) #26644

Merged

richardlau mentioned this pull request Mar 28, 2019

Address HashWick #23259

Closed

BethGriggs mentioned this pull request May 1, 2019

v10.16.0 proposal #27514

Merged

punkeel mentioned this pull request Jun 21, 2021

build: enable v8's siphash for hash seed creation denoland/rusty_v8#707

Merged

build: enable v8's SipHash for hash seed creation by rvagg · Pull Request #26367 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: enable v8's SipHash for hash seed creation - #26367

build: enable v8's SipHash for hash seed creation#26367rvagg wants to merge 2 commits intonodejs:masternodejs/node:masterfrom rvagg:rvagg/v8-siphashrvagg/io.js:rvagg/v8-siphashCopy head branch name to clipboard

Conversation

rvagg commented Mar 1, 2019

Uh oh!

nodejs-github-bot commented Mar 1, 2019

Uh oh!

devsnek left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau commented Mar 1, 2019

Uh oh!

rvagg commented Mar 1, 2019

Uh oh!

refack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rvagg commented Mar 1, 2019

Uh oh!

refack commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Mar 1, 2019

Uh oh!

rvagg commented Mar 1, 2019

Uh oh!

targos commented Mar 1, 2019

Uh oh!

hashseed commented Mar 1, 2019

Uh oh!

bnoordhuis Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

rvagg Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

refack Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvagg Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

rvagg commented Mar 1, 2019

Uh oh!

refack Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Mar 1, 2019

Uh oh!

rvagg commented Mar 2, 2019

Uh oh!

addaleax commented Mar 2, 2019

Uh oh!

rvagg commented Mar 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

build: enable v8's SipHash for hash seed creation#26367
rvagg wants to merge 2 commits into
nodejs:masterfrom
rvagg:rvagg/v8-siphash

refack left a comment •
edited
Loading

refack commented Mar 1, 2019 •
edited
Loading

refack Mar 1, 2019 •
edited
Loading