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

Use RdSeed when available, and reduce RdRand load #15250

Merged
merged 1 commit into from Feb 18, 2019
Merged

Conversation

@sipa
Copy link
Member

@sipa sipa commented Jan 25, 2019

This introduces support for autodetecting and using the RdSeed instruction on x86/x86_64 systems.

In addition:

  • In SeedFast, only 64 bits of entropy are generated through RdRand (256 was relatively slow).
  • In SeedStartup, 256 bits of entropy are generated, using RdSeed (preferably) or RdRand (otherwise).
@sipa sipa force-pushed the 201901_rdseed branch 2 times, most recently from 4dddd07 to 83ebab1 Jan 25, 2019
@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jan 25, 2019

While you're changing random.cpp: isn't the std::move on L562 without any effect? Could be removed?

@sipa
Copy link
Member Author

@sipa sipa commented Jan 25, 2019

@practicalswift MixExtract intentionally takes an rvalue reference, so that callers must either pass in a temporary, or use std::move explicitly (as the hasher object becomes useless after the call).

src/random.cpp Outdated Show resolved Hide resolved
src/random.cpp Outdated Show resolved Hide resolved
src/random.cpp Outdated Show resolved Hide resolved
@sipa sipa force-pushed the 201901_rdseed branch from 83ebab1 to 6739643 Jan 26, 2019
@sipa
Copy link
Member Author

@sipa sipa commented Jan 26, 2019

@gmaxwell Wow, I didn't know about those failure modes of RdRand and RdSeed. Rewrote things a bit to take that into account. RdRand is now retried up to 10 times (and ignored after that). RdSeed is retried indefinitely (with a pause in between). When using RdRand for 256-bit seeding, it's invoked 1024 times and XORed to produce each 64 bit group.

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Jan 28, 2019

utACK

Copy link
Member

@Empact Empact left a comment

Concept ACK. Apologies if any of the above is particularly naive. :P

src/random.cpp Outdated Show resolved Hide resolved
src/random.cpp Show resolved Hide resolved
src/random.cpp Show resolved Hide resolved
src/random.cpp Outdated Show resolved Hide resolved
src/random.cpp Show resolved Hide resolved
src/random.cpp Show resolved Hide resolved
src/random.cpp Outdated Show resolved Hide resolved
src/random.cpp Outdated Show resolved Hide resolved
@sipa sipa force-pushed the 201901_rdseed branch from 6739643 to eab3817 Feb 3, 2019
This introduces support for autodetecting and using the RdSeed instruction.

In addition:
* In SeedFast, only 64 bits of entropy are generated through RdRand (256 was relatively slow).
* In SeedStartup, 256 bits of entropy are generated, using RdSeed (preferably) or RdRand (otherwise).
@sipa sipa force-pushed the 201901_rdseed branch from eab3817 to 1435fab Feb 4, 2019
@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Feb 7, 2019

ACK

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Feb 7, 2019

Aside, we might want to consider this one a bug fix, since technically we weren't using rdrand quite correctly before. OTOH, the use of rdrand was entirely non-critical... sooo...

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Feb 7, 2019
@laanwj
Copy link
Member

@laanwj laanwj commented Feb 12, 2019

So to be clear: this doesn't result in a CPU with broken and/or backdoored RDRAND or RDSEED instruction to generate vulberable private keys?

@sipa
Copy link
Member Author

@sipa sipa commented Feb 12, 2019

@laanwj Not trivially, at least. The output of the rdrand/rdseed is mixed with other entropy using SHA512 to produce output and the new state.

In theory a backdoored CPU could try to infer what variable is going to store entropy, and try to control that directly.

@laanwj
Copy link
Member

@laanwj laanwj commented Feb 12, 2019

Thanks!

In theory a backdoored CPU could try to infer what variable is going to store entropy, and try to control that directly.

Sure—I meant when say, only those instructions were made to return a fixed value, not anything more advanced/complex. Like a bugdoor with plausible deniability.

@laanwj
Copy link
Member

@laanwj laanwj commented Feb 13, 2019

Code review utACK 1435fab
I have no hardware to be able to test this on.
(if you do, /proc/cpuinfo will show the rdrand and/or rdseed under flags)

@JustinTArthur
Copy link
Contributor

@JustinTArthur JustinTArthur commented Feb 13, 2019

ACK 1435fab on macOS x86_64 on Intel Core i7-7920HQ CPU @ 3.10GHz (has RDRAND & RDSEED).

2019-02-13T19:22:30Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
2019-02-13T19:22:30Z Using RdSeed as additional entropy source
2019-02-13T19:22:30Z Using RdRand as an additional entropy source

Functional and unit tests pass.

@sipa
Copy link
Member Author

@sipa sipa commented Feb 13, 2019

Tested on a system with RdRand but no RdSeed; tests pass, bitcoind runs fine, and reports only "Using RdRand as an additional entropy source".

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Feb 14, 2019

Tested on a system with no rdrand/rdseed, and it didn't catch fire. does @luke-jr maybe have a 32-bit system with rdrand/rdseed?

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Feb 14, 2019

@laanwj To expand on Pieter's response. Our mixing construction is designed to be secure against adversarial inputs generated by an attacker that can also inspect the entire process state unless the attacker can attack SHA512 in a very serious way (e.g. construct a suffix to a message being hashed that causes a specific hash value).

There are various standards for combining entropy sources that just xor the sources. The construction we're using is obviously stronger than those. If we had just xored the state then a malicious rdrand could inspect and return the current state (perhaps xored with a constant) in order to override it, and even plausibly do so as a bugdoor (e.g. oops we accidentally failed to update the register). I just mention this to point out that what we're doing is stronger than something other people think is acceptable...

@pstratem
Copy link
Contributor

@pstratem pstratem commented Feb 14, 2019

@gmaxwell
Linux debian 4.9.0-8-686-pae #1 SMP Debian 4.9.130-2 (2018-10-27) i686 GNU/Linux 2019-02-14T21:30:29Z Using RdSeed as additional entropy source 2019-02-14T21:30:29Z Using RdRand as an additional entropy source

@laanwj laanwj merged commit 1435fab into bitcoin:master Feb 18, 2019
2 checks passed
laanwj added a commit that referenced this issue Feb 18, 2019
1435fab Use RdSeed when available, and reduce RdRand load (Pieter Wuille)

Pull request description:

  This introduces support for autodetecting and using the RdSeed instruction on x86/x86_64 systems.

  In addition:
  * In SeedFast, only 64 bits of entropy are generated through RdRand (256 was relatively slow).
  * In SeedStartup, 256 bits of entropy are generated, using RdSeed (preferably) or RdRand (otherwise).

Tree-SHA512: fb7d3e22e93e14592f4b07282aa79d7c3cc4e9debdd9978580b8d2562bbad345e289bf3f80de2c50c9b50b8bac2aa9b838f9f272f7f8d43f1efc0913aa8acce3
random-zebra added a commit to PIVX-Project/PIVX that referenced this issue Apr 14, 2021
cecbf6c Use secure.h header for secure allocators (Fuzzbawls)
d9f67da net: add ifaddrs.h include (fanquake)
e906436 build: check if -lsocket is required with *ifaddrs (fanquake)
414f405 rand: only try and use freeifaddrs if available (fanquake)
3a039d6 build: avoid getifaddrs when unavailable (Cory Fields)
77bddd7 Use GetStrongRandBytes in gmp bignum initialization (Fuzzbawls)
b70b26f Fix typo in comment in randomenv.cpp (Fuzzbawls)
fec460c Put bounds on the number of CPUID leaves explored (Pieter Wuille)
41ab1ff Fix CPUID subleaf iteration (Pieter Wuille)
8a9bbb1 Move events_hasher into RNGState() (Pieter Wuille)
88c2ae5 random: mark RandAddPeriodic and SeedPeriodic as noexcept (fanquake)
81d382f doc: correct random.h docs after bitcoin#17270 (fanquake)
f363ea9 Seed RNG with precision timestamps on receipt of net messages. (Matt Corallo)
7d6ddcb Run background seeding periodically instead of unpredictably (Pieter Wuille)
4679181 Add information gathered through getauxval() (Pieter Wuille)
88d97d0 Feed CPUID data into RNG (Pieter Wuille)
8f5b9c9 Use sysctl for seeding on MacOS/BSD (Pieter Wuille)
67de246 Gather additional entropy from the environment (Pieter Wuille)
6142e1f Seed randomness with process id / thread id / various clocks (Pieter Wuille)
7bde8b7 [MOVEONLY] Move cpuid code from random to compat/cpuid (Fuzzbawls)
52b5336 [MOVEONLY] Move perfmon data gathering to new randomenv module (Pieter Wuille)
27cf995 doc: minor corrections in random.cpp (fanquake)
fccd2b8 doc: correct function name in ReportHardwareRand() (fanquake)
909473e Fix FreeBSD build by including utilstrencodings.h (Fuzzbawls)
630931f break circular dependency: random/sync -> util -> random/sync (Fuzzbawls)
5eed08c random: remove call to RAND_screen() (Windows only) (fanquake)
ada9868 gui: remove OpenSSL PRNG seeding (Windows, Qt only) (fanquake)
22a7121 Fix non-deterministic coverage of test DoS_mapOrphans (Fuzzbawls)
79e7fd3 Add ChaCha20 bench (Jonas Schnelli)
6966aa9 Add ChaCha20 encryption option (XOR) (Jonas Schnelli)
28c9cdb tests: Add script checking for deterministic line coverage (practicalswift)
c82e359 test: Make bloom tests deterministic (MarcoFalke)
7b33223 Document strenghtening (Pieter Wuille)
0190dec Add hash strengthening to the RNG (Pieter Wuille)
67e336d Use RdSeed when available, and reduce RdRand load (Pieter Wuille)
4ffda1f Document RNG design in random.h (Pieter Wuille)
2b6381e Use secure allocator for RNG state (Pieter Wuille)
080deb3 Encapsulate RNGState better (Pieter Wuille)
787d72f DRY: Implement GetRand using FastRandomContext::randrange (Pieter Wuille)
5bc2583 Sprinkle some sweet noexcepts over the RNG code (Pieter Wuille)
774899f Remove hwrand_initialized. (Pieter Wuille)
698d133 Switch all RNG code to the built-in PRNG. (Pieter Wuille)
038a45a Integrate util/system's CInit into RNGState (Fuzzbawls)
5f20e62 Abstract out seeding/extracting entropy into RNGState::MixExtract (Pieter Wuille)
298f97c Add thread safety annotations to RNG state (Pieter Wuille)
2326535 Rename some hardware RNG related functions (Pieter Wuille)
d76ee83 Automatically initialize RNG on first use. (Pieter Wuille)
1a5dbc5 Don't log RandAddSeedPerfmon details (Pieter Wuille)
32e6c42 Simplify testing RNG code (Fuzzbawls)
972effa Make unit tests use the insecure_rand_ctx exclusively (Fuzzbawls)
af52bf5 Use a FastRandomContext in LimitOrphanTxSize (Fuzzbawls)
746d466 Introduce a Shuffle for FastRandomContext and use it in wallet (Fuzzbawls)
1cdf124 Use a local FastRandomContext in a few more places in net (Fuzzbawls)
e862564 Make addrman use its local RNG exclusively (Fuzzbawls)
94b2ead Make FastRandomContext support standard C++11 RNG interface (Pieter Wuille)

Pull request description:

  This is a collection of upstream PRs that have been backported to bring our RNG (`src/random`) code more up-to-date. The following upstream PRs have been included here:

  - bitcoin#12742
  - bitcoin#14624
    - some of this had already been merged previously
  - bitcoin#14955
  - bitcoin#15250
  - bitcoin#15224
  - bitcoin#15324
  - bitcoin#15296
  - bitcoin#15512
  - bitcoin#16878
  - bitcoin#17151
  - bitcoin#17191
  - bitcoin#13236
  - bitcoin#13314
  - bitcoin#17169
  - bitcoin#17270
    -  omitted last commit as our testing framework doesn't support it currently
    - omitted bitcoin@64e1e02, to be pulled in after our time utility is updated in a separate PR
  - bitcoin#17573
  - bitcoin#17507
  - bitcoin#17670
  - bitcoin#17527
  - bitcoin#14127
  - bitcoin#21486

ACKs for top commit:
  furszy:
    ACK cecbf6c with a minor nit that can be easily tackled later.
  random-zebra:
    rebase utACK cecbf6c and merging...

Tree-SHA512: 3463b693cc9bddc1ec15228d264a794f5c2f159073fafa2ccf6e2563abfeb4369e49505f97ca84f2478ca792bd07b66d2cd83c58044d6a0cae6af42d22f5784b
ogabrielides added a commit to ogabrielides/dash that referenced this issue Sep 16, 2021
1435fab Use RdSeed when available, and reduce RdRand load (Pieter Wuille)

Pull request description:

  This introduces support for autodetecting and using the RdSeed instruction on x86/x86_64 systems.

  In addition:
  * In SeedFast, only 64 bits of entropy are generated through RdRand (256 was relatively slow).
  * In SeedStartup, 256 bits of entropy are generated, using RdSeed (preferably) or RdRand (otherwise).

Tree-SHA512: fb7d3e22e93e14592f4b07282aa79d7c3cc4e9debdd9978580b8d2562bbad345e289bf3f80de2c50c9b50b8bac2aa9b838f9f272f7f8d43f1efc0913aa8acce3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants