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

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

Closed
wants to merge 2 commits into from

Conversation

@rvagg
Copy link
Member

commented Mar 1, 2019

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 rvagg requested review from refack, indutny and hashseed Mar 1, 2019

@devsnek

devsnek approved these changes Mar 1, 2019

Copy link
Member

left a comment

nice

@richardlau

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

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

@rvagg

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

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?

@refack

refack approved these changes Mar 1, 2019

Copy link
Member

left a comment

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

@@ -130,6 +130,7 @@
'v8_enable_verify_predictable=<(v8_enable_verify_predictable)',
'v8_target_cpu=<(v8_target_arch)',
'v8_use_snapshot=<(v8_use_snapshot)',
'v8_use_siphash=<(v8_use_siphash)',

This comment has been minimized.

Copy link
@refack

refack Mar 1, 2019

Member

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

Show resolved Hide resolved deps/v8/gypfiles/v8.gyp
@rvagg

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

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

@refack

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

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.

@refack refack requested review from targos and ryzokuken Mar 1, 2019

@richardlau

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

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.

@rvagg

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

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

@targos

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

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

@hashseed

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

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

@@ -1118,6 +1123,7 @@ def configure_v8(o):
o['variables']['v8_random_seed'] = 0 # Use a random seed for hash tables.
o['variables']['v8_promise_internal_field_count'] = 1 # Add internal field to promises for async hooks.
o['variables']['v8_use_snapshot'] = 'false' if options.without_snapshot else 'true'
o['variables']['v8_use_siphash'] = 'false' if options.without_siphash else 'true'

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Mar 1, 2019

Member

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

This comment has been minimized.

Copy link
@rvagg

rvagg Mar 1, 2019

Author Member

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

This comment has been minimized.

Copy link
@refack

refack Mar 1, 2019

Member

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

./configure -- -Dv8_use_siphash=false

seems good enough

This comment has been minimized.

Copy link
@rvagg

rvagg Mar 2, 2019

Author Member

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

@rvagg

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

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.


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

To the extent possible under law, the author(s) have dedicated all

This comment has been minimized.

Copy link
@refack

refack Mar 1, 2019

Member

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

@indutny

indutny approved these changes Mar 1, 2019

Copy link
Member

left a comment

Excellent!

@refack

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

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

@rvagg

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

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.
  """
@addaleax

This comment has been minimized.

@jasnell

jasnell approved these changes Mar 2, 2019

@refack

refack approved these changes Mar 2, 2019

Copy link
Member

left a comment

:shipit:

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@rvagg should this land in a single commit or in two?

@rvagg

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2019

@refack it doesn't require it but doesn't prohibit it, in fact it's explicitly allowed. We have a practice of liberal attribution here which is why I'm going to leave it unless the author requests attribution be removed.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

Ping @rvagg about landing this in a single commit or two. I guess it was meant to be landed in two? I just ask as I am unsure if the LICENSE should really be added in a second commit.

@rvagg rvagg force-pushed the rvagg:rvagg/v8-siphash branch from 3eeeb5f to 7047468 Mar 12, 2019

rvagg added some commits Mar 1, 2019

build: enable v8's siphash for hash seed creation
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.

@rvagg rvagg force-pushed the rvagg:rvagg/v8-siphash branch from 7047468 to a155578 Mar 12, 2019

@rvagg

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

Landed in e1cd8ac...0d94c23

@rvagg rvagg closed this Mar 12, 2019

@rvagg rvagg deleted the rvagg:rvagg/v8-siphash branch Mar 12, 2019

rvagg added a commit that referenced this pull request Mar 12, 2019

build: enable v8's siphash for hash seed creation
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>

rvagg added a commit that referenced this pull request Mar 12, 2019

deps,tools: include SipHash in LICENSE
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>

BridgeAR added a commit that referenced this pull request Mar 13, 2019

build: enable v8's siphash for hash seed creation
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>

BridgeAR added a commit that referenced this pull request Mar 13, 2019

deps,tools: include SipHash in LICENSE
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>

BridgeAR added a commit that referenced this pull request Mar 13, 2019

2019-03-14, Version 11.12.0 (Current)
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

BridgeAR added a commit that referenced this pull request Mar 14, 2019

build: enable v8's siphash for hash seed creation
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>

BridgeAR added a commit that referenced this pull request Mar 14, 2019

deps,tools: include SipHash in LICENSE
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>

BridgeAR added a commit that referenced this pull request Mar 14, 2019

2019-03-14, Version 11.12.0 (Current)
Notable Changes

* 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

BridgeAR added a commit that referenced this pull request Mar 14, 2019

2019-03-14, Version 11.12.0 (Current)
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

BridgeAR added a commit that referenced this pull request Mar 14, 2019

2019-03-14, Version 11.12.0 (Current)
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

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 14, 2019

2019-03-14, Version 11.12.0 (Current)
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

BridgeAR added a commit that referenced this pull request Mar 14, 2019

2019-03-14, Version 11.12.0 (Current)
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

BridgeAR added a commit that referenced this pull request Mar 15, 2019

2019-03-15, Version 11.12.0 (Current)
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

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 15, 2019

2019-03-15, Version 11.12.0 (Current)
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

Drieger added a commit to Drieger/node that referenced this pull request Mar 22, 2019

2019-03-15, Version 11.12.0 (Current)
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

@richardlau richardlau referenced this pull request Mar 28, 2019

Open

Address HashWick #23259

BethGriggs added a commit that referenced this pull request Apr 16, 2019

deps,tools: include SipHash in LICENSE
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.