[Crypto] Use stronger rand for key generation #643

Warrows · 2018-06-29T09:30:40Z

Since #576 hasn't changed in over a month, here is a reworked version of it.
So in this PR:
-We add the memory_cleanse function from upstream, to remove a number of OpenSSL calls.
-We use OS randomness in addition to OpenSSL randomness (see #576 for why it's needed).

Mrs-X

NACK.

If you look upstream at Bitcoin, static void GetOSRand(unsigned char *ent32) (and probably others) uses a much more sophisticated code for the different OS platforms. (see https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp)

If we go their approach, we have to completely copy their implementation and double check whether it's properly used by our code.

Remember that lots of SSL attacks (and security attacks in general) are based on flawed random number generation.

Until then we should stick with what we have now.

Warrows · 2018-10-28T18:25:02Z

I moved the memory_cleanse stuff into its own PR (#761) and rebased this on top of it. I'm still looking at bringing this up to date with current Bitcoin Core code.

Fuzzbawls · 2019-07-01T04:29:27Z

Unless you're also going to pick bitcoin@124d13a, need to include test_random.h in Makefile.test.include

Warrows · 2019-07-09T20:48:42Z

@Mrs-X @Fuzzbawls This branch is caught up with Bitcoin from december, 2018 (pre bitcoin#14955)
I intend to pursue the effort but I think it's a good point to examine and merge. My preferred course of action is to first merge this PR, and then to open an other to backport bitcoin#14955 and the few following changes. If you guys are comfortable with that, I'll change the status here from WIP to reviewable. If not, then the other course of action is to keep adding everything here I guess. Let me know what you think.

These are available in sandboxes without access to files or devices. Also [they are safer and more straightforward](https://en.wikipedia.org/wiki/Entropy-supplying_system_calls) to use than `/dev/urandom` as reading from a file has quite a few edge cases: - Linux: `getrandom(buf, buflen, 0)`. [getrandom(2)](http://man7.org/linux/man-pages/man2/getrandom.2.html) was introduced in version 3.17 of the Linux kernel. - OpenBSD: `getentropy(buf, buflen)`. The [getentropy(2)](http://man.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man2/getentropy.2) function appeared in OpenBSD 5.6. - FreeBSD and NetBSD: `sysctl(KERN_ARND)`. Not sure when this was added but it has existed for quite a while. Alternatives: - Linux has sysctl `CTL_KERN` / `KERN_RANDOM` / `RANDOM_UUID` which gives 16 bytes of randomness. This may be available on older kernels, however [sysctl is deprecated on Linux](https://lwn.net/Articles/605392/) and even removed in some distros so we shouldn't use it. Add tests for `GetOSRand()`: - Test that no error happens (otherwise `RandFailure()` which aborts) - Test that all 32 bytes are overwritten (initialize with zeros, try multiple times) Discussion: - When to use these? Currently they are always used when available. Another option would be to use them only when `/dev/urandom` is not available. But this would mean these code paths receive less testing, and I'm not sure there is any reason to prefer `/dev/urandom`. Closes: bitcoin#9676

-BEGIN VERIFY SCRIPT- sed -i 's/insecure_randbits(1)/insecure_randbool()/g' src/test/*_tests.cpp sed -i 's/insecure_randrange(2)/insecure_randbool()/g' src/test/*_tests.cpp sed -i 's/insecure_randrange(4)/insecure_randbits(2)/g' src/test/*_tests.cpp sed -i 's/insecure_randrange(32)/insecure_randbits(5)/g' src/test/*_tests.cpp sed -i 's/insecure_randrange(256)/insecure_randbits(8)/g' src/test/*_tests.cpp -END VERIFY SCRIPT-

functions -BEGIN VERIFY SCRIPT- sed -i 's/\<insecure_randbits(/InsecureRandBits(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<insecure_randbool(/InsecureRandBool(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<insecure_randrange(/InsecureRandRange(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<insecure_randbytes(/InsecureRandBytes(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<insecure_rand256(/InsecureRand256(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<insecure_rand(/InsecureRand32(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<seed_insecure_rand(/SeedInsecureRand(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp -END VERIFY SCRIPT-

Some mistakes where done while backporting bitcoin#10321 Compilation is fixed here

rbx needs to be stashed in a 64bit register on 64bit platforms. With this crash in particular, it was holding a stack canary which was not properly restored after the cpuid. Split out the x86+PIC case so that x86_64 doesn't have to worry about it.

Rationale: * Reduce the number of false positives from static analyzers * Potentially enable additional compiler optimizations

random-zebra

Looks good from visual inspection. Will run some test.

After this is merged, these changes can also be used to take a stab at solving the problem left in #875 (get better randomness distribution in GMP bignum rand funcs).

src/crypto/chacha20.cpp

src/net.cpp

src/crypto/chacha20.cpp

Makes the code shorter and more concise. Might allow for some compiler optimisations.

random-zebra

ACK b7dda92

stale

furszy

ACK b7dda92

b7dda92 [Log] Replace a string by the function name in a log (warrows) 977f089 [Refactor] Use arrays instead of unic vars in Chacha20 (warrows) d8abe32 [Random] Add a missing include (warrows) 27663b8 Do not permit copying FastRandomContexts (Pieter Wuille) 64e03e6 Bugfix: randbytes should seed when needed (non reachable issue) (Pieter Wuille) e8f12aa Check if sys/random.h is required for getentropy on OSX. (James Hilliard) de85c7a Add attribute [[noreturn]] (C++11) to functions that will not return (practicalswift) df46c7f Fix resource leak (Dag Robole) d426d85 Clarify entropy source (Pieter Wuille) 30a320b Use cpuid intrinsics instead of asm code (Pieter Wuille) 0c21204 random: fix crash on some 64bit platforms (Cory Fields) b8bbb9c Use rdrand as entropy source on supported platforms (Pieter Wuille) 8e19443 [Tests] Fix compilation (warrows) f53edec [Rand/test] scripted-diff: Use new naming style for insecure_rand* functions (warrows) 272f3a5 [Random / tests] scripted-diff: Use randbits/bool instead of randrange (warrows) 0173ee3 Replace rand() & ((1 << N) - 1) with randbits(N) (Pieter Wuille) 250de74 Replace more rand() % NUM by randranges (Pieter Wuille) d690413 [Random / tests] scripted-diff: use insecure_rand256/randrange more (warrows) 4a811ff Merge test_random.h into test_bitcoin.h (Pieter Wuille) f275e63 Add various insecure_rand wrappers for tests (Pieter Wuille) 602af4f Add FastRandomContext::rand256() and ::randbytes() (Pieter Wuille) 9054978 Add perf counter data to GetStrongRandBytes state in scheduler (Matt Corallo) 17dd13e Add internal method to add new random data to our internal RNG state (Matt Corallo) c7a1602 Use sanity check timestamps as entropy (Pieter Wuille) f671fe9 Test that GetPerformanceCounter() increments (Pieter Wuille) dcb536f Use hardware timestamps in RNG seeding (Pieter Wuille) 7c3f290 [Random] Fix compilation (warrows) 22b7895 random: only use getentropy on openbsd (Cory Fields) e5750e5 Add a FastRandomContext::randrange and use it (Pieter Wuille) 2a0f6cd Switch FastRandomContext to ChaCha20 (Pieter Wuille) 401ca7d Introduce FastRandomContext::randbool() (Pieter Wuille) 3d056d6 Add ChaCha20 (Pieter Wuille) 3c97f3f Kill insecure_random and associated global state (Wladimir J. van der Laan) 68ba16c Maintain state across GetStrongRandBytes calls (Pieter Wuille) c3c399e random: Add fallback if getrandom syscall not available (Wladimir J. van der Laan) 7a8111f sanity: Move OS random to sanity check function (Wladimir J. van der Laan) 387c2e9 squashme: comment that NUM_OS_RANDOM_BYTES should not be changed lightly (Wladimir J. van der Laan) 9e8c266 util: Specific GetOSRandom for Linux/FreeBSD/OpenBSD (Wladimir J. van der Laan) f989b86 Don't use assert for catching randomness failures (Pieter Wuille) a15419e Always require OS randomness when generating secret keys (Pieter Wuille) Pull request description: Since #576 hasn't changed in over a month, here is a reworked version of it. So in this PR: -We add the memory_cleanse function from upstream, to remove a number of OpenSSL calls. -We use OS randomness in addition to OpenSSL randomness (see #576 for why it's needed). ACKs for top commit: random-zebra: ACK b7dda92 furszy: ACK [`b7dda92`](b7dda92) Tree-SHA512: d92cbc14d844263ced753248e646f5cd4f03ec37546f50ff1b558fc3076b6d777c7efcb899c0400bc510e21311b5bd93d9aca26d811033fedb370f8457204035

3f3edde [Bench] Use PIVX address in Base58Decode test (random-zebra) 5a1be90 [Travis] Disable benchmark framework for trusty test (random-zebra) 1bd89ac Initialize recently introduced non-static class member lastCycles to zero in constructor (random-zebra) ec60671 Require a steady clock for bench with at least micro precision (random-zebra) 84069ce bench: prefer a steady clock if the resolution is no worse (random-zebra) 38367b1 bench: switch to std::chrono for time measurements (random-zebra) a24633a Remove countMaskInv caching in bench framework (random-zebra) 9e9bc22 Restore default format state of cout after printing with std::fixed/setprecision (random-zebra) 3dd559d Avoid static analyzer warnings regarding uninitialized arguments (random-zebra) e85f224 Replace boost::function with std::function (C++11) (random-zebra) 98c0857 Prevent warning: variable 'x' is uninitialized (random-zebra) 7f0d4b3 FastRandom benchmark (random-zebra) d9fa0c6 Add prevector destructor benchmark (random-zebra) e1527ba Assert that what might look like a possible division by zero is actually unreachable (random-zebra) e94cf15 bench: Fix initialization order in registration (random-zebra) 151c25f Basic CCheckQueue Benchmarks (random-zebra) 51aedbc Use std:thread:hardware_concurrency, instead of Boost, to determine available cores (random-zebra) d447613 Use real number of cores for default -par, ignore virtual cores (random-zebra) 9162a56 [Refactoring] Removed using namespace <xxx> from bench/ sources (random-zebra) 5c07f67 bench: Add support for measuring CPU cycles (random-zebra) 41ce1ed bench: Fix subtle counting issue when rescaling iteration count (random-zebra) 68ea794 Avoid integer division in the benchmark inner-most loop. (random-zebra) 3fa4f27 bench: Added base58 encoding/decoding benchmarks (random-zebra) 4442118 bench: Add crypto hash benchmarks (random-zebra) a5179b6 [Trivial] ensure minimal header conventions (random-zebra) 8607d6b Support very-fast-running benchmarks (random-zebra) 4aebb60 Simple benchmarking framework (random-zebra) Pull request description: Introduces the benchmarking framework, loosely based on google's micro-benchmarking library (https://github.com/google/benchmark), ported from Bitcoin, up to 0.16. The benchmark framework is hard-coded to run each benchmark for one wall-clock second, and then spits out .csv-format timing information to stdout. Backported PR: - bitcoin#6733 - bitcoin#6770 - bitcoin#6892 - bitcoin#8039 - bitcoin#8107 - bitcoin#8115 - bitcoin#9200 - bitcoin#9202 - bitcoin#9281 - bitcoin#6361 - bitcoin#10271 - bitcoin#9498 - bitcoin#9712 - bitcoin#9547 - bitcoin#9505 (benchmark only. Rest was in #1557) - bitcoin#9792 (benchmark only. Rest was in #643) - bitcoin#10272 - bitcoin#10395 (base58 only) - bitcoin#10963 - bitcoin#11303 (first commit) - bitcoin#11562 - bitcoin#11646 - bitcoin#11654 Current output of `src/bench/bench_pivx`: ``` #Benchmark,count,min(ns),max(ns),average(ns),min_cycles,max_cycles,average_cycles Base58CheckEncode,131072,7697,8065,7785,20015,20971,20242 Base58Decode,294912,3305,3537,3454,8595,9198,8981 Base58Encode,180224,5498,6020,5767,14297,15652,14994 CCheckQueueSpeed,320,3159960,3535173,3352787,8216030,9191602,8717388 CCheckQueueSpeedPrevectorJob,96,9184484,11410840,10823070,23880046,29668680,28140445 FastRandom_1bit,320,3143690,4838162,3199156,8173726,12579373,8317941 FastRandom_32bit,60,17097612,17923669,17367440,44454504,46602306,45156079 PrevectorClear,3072,334741,366618,346731,870340,953224,901516 PrevectorDestructor,2816,344233,368912,357281,895022,959187,928948 RIPEMD160,288,3404503,3693917,3577774,8851850,9604334,9302363 SHA1,384,2718128,2891558,2802513,7067238,7518184,7286652 SHA256,176,6133760,6580005,6239866,15948035,17108376,16223916 SHA512,240,4251468,4358706,4313463,11054006,11332826,11215186 Sleep100ms,10,100221470,100302411,100239073,260580075,260790726,260625870 ``` NOTE: Not all the tests have been pulled yet (as we might not have the code being tested, or it would require rewrites to work with our different code base), but the framework is updated to December 2017. ACKs for top commit: Fuzzbawls: ACK 3f3edde Tree-SHA512: c283311a9accf6d2feeb93b185afa08589ebef3f18b6e86980dbc3647b9845f75ac9ecce2f1b08738d25ceac36596a2c89d41e4dbf3b463502aa695611aa1f8e

ghost assigned Warrows Jun 29, 2018

ghost added the review label Jun 29, 2018

Mrs-X previously requested changes Jun 29, 2018

View reviewed changes

Warrows mentioned this pull request Aug 4, 2018

Utils and libraries: back porting bitcoin RNG improvement #576

Closed

Warrows force-pushed the remove_openssl_random branch from 3cd9eaa to 2fe3bff Compare August 4, 2018 09:02

Warrows changed the title ~~[Crypto] Use stronger rand for key generation~~ [WIP] [Crypto] Use stronger rand for key generation Aug 7, 2018

Warrows force-pushed the remove_openssl_random branch from 2fe3bff to f1f6869 Compare August 15, 2018 13:04

Warrows force-pushed the remove_openssl_random branch from f1f6869 to bd0d53f Compare October 28, 2018 18:16

Warrows force-pushed the remove_openssl_random branch from bd0d53f to c022e59 Compare October 28, 2018 18:44

Warrows added this to the Future milestone Nov 16, 2018

Warrows force-pushed the remove_openssl_random branch from c022e59 to 2039b15 Compare December 26, 2018 11:41

Warrows mentioned this pull request Jan 20, 2019

Bring PIVX random on par with bitcoin #799

Closed

42 tasks

Warrows force-pushed the remove_openssl_random branch from 2039b15 to 1a46b99 Compare January 24, 2019 17:29

Warrows force-pushed the remove_openssl_random branch from f2ff2e5 to 7d22de6 Compare June 30, 2019 12:19

Warrows force-pushed the remove_openssl_random branch 2 times, most recently from 7f97397 to 50a6293 Compare July 9, 2019 20:43

Warrows changed the title ~~[WIP] [Crypto] Use stronger rand for key generation~~ [Crypto] Use stronger rand for key generation Jul 14, 2019

Warrows requested a review from Mrs-X July 14, 2019 21:47

Warrows modified the milestones: Future, 4.0.0 Jul 17, 2019

Warrows requested review from Fuzzbawls, furszy and random-zebra July 17, 2019 12:44

Warrows added the Backport label Jul 17, 2019

sipa and others added 3 commits July 31, 2019 11:50

Always require OS randomness when generating secret keys

a15419e

Don't use assert for catching randomness failures

f989b86

sipa and others added 14 commits July 31, 2019 11:52

Replace rand() & ((1 << N) - 1) with randbits(N)

0173ee3

[Tests] Fix compilation

8e19443

Some mistakes where done while backporting bitcoin#10321 Compilation is fixed here

Use rdrand as entropy source on supported platforms

b8bbb9c

Use cpuid intrinsics instead of asm code

30a320b

Clarify entropy source

d426d85

Fix resource leak

df46c7f

Add attribute [[noreturn]] (C++11) to functions that will not return

de85c7a

Rationale: * Reduce the number of false positives from static analyzers * Potentially enable additional compiler optimizations

Check if sys/random.h is required for getentropy on OSX.

e8f12aa

Bugfix: randbytes should seed when needed (non reachable issue)

64e03e6

Do not permit copying FastRandomContexts

27663b8

[Random] Add a missing include

d8abe32

Warrows force-pushed the remove_openssl_random branch from 50a6293 to d8abe32 Compare July 31, 2019 11:05

random-zebra reviewed Jul 31, 2019

View reviewed changes

src/crypto/chacha20.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

random-zebra self-requested a review July 31, 2019 12:41

furszy reviewed Jul 31, 2019

View reviewed changes

src/crypto/chacha20.cpp Outdated Show resolved Hide resolved

Warrows added 2 commits August 1, 2019 09:54

[Refactor] Use arrays instead of unic vars in Chacha20

977f089

Makes the code shorter and more concise. Might allow for some compiler optimisations.

[Log] Replace a string by the function name in a log

b7dda92

random-zebra approved these changes Aug 3, 2019

View reviewed changes

furszy self-requested a review September 20, 2019 01:25

furszy approved these changes Sep 20, 2019

View reviewed changes

random-zebra merged commit b7dda92 into PIVX-Project:master Sep 20, 2019

Warrows deleted the remove_openssl_random branch September 20, 2019 20:26

random-zebra mentioned this pull request May 27, 2020

[Tests] Benchmarking Framework #1650

Merged

PRCYDev mentioned this pull request Feb 2, 2021

[Crypto] Use stronger rand for key generation [rpcserver] PRCYCoin/PRCYCoin#14

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Crypto] Use stronger rand for key generation #643

[Crypto] Use stronger rand for key generation #643

Uh oh!

Warrows commented Jun 29, 2018

Uh oh!

Mrs-X left a comment •

edited

Loading

Uh oh!

Warrows commented Oct 28, 2018

Uh oh!

Fuzzbawls commented Jul 1, 2019

Uh oh!

Warrows commented Jul 9, 2019

Uh oh!

random-zebra left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

random-zebra left a comment

Uh oh!

furszy left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

[Crypto] Use stronger rand for key generation #643

[Crypto] Use stronger rand for key generation #643

Uh oh!

Conversation

Warrows commented Jun 29, 2018

Uh oh!

Mrs-X 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!

Warrows commented Oct 28, 2018

Uh oh!

Fuzzbawls commented Jul 1, 2019

Uh oh!

Warrows commented Jul 9, 2019

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Mrs-X left a comment •

edited

Loading