Always require OS randomness when generating secret keys #7891

sipa · 2016-04-16T10:51:00Z

With concerns about OpenSSL's RNG increasing, we should just always require OS randomness in addition to our normal randomness source when generating keys. This is an infrequent operation (especially since signing was switched to using deterministic nonces), so this should not hurt performance at all.

In addition, get rid of the random calls to RandAddPerfMonData, which were generally correlated with places where keys or signatures were generated. Better just do it whenever we actually need that kind of assurance.

This does add a dependency from random on crypto, which makes bitcoin-cli now link in crypto. That's unfortunate, and the randomness utilities should probably moved to a different lib, but I'm not doing that now.

luke-jr · 2016-04-16T10:56:55Z

src/random.cpp

This doesn't look safe without a retry. read() doesn't guarantee the entire buffer gets filled ever, afaik.

I believe there are some guarantees for reading from /dev/urandom always providing at least up to some number of bytes when requested, but it's better to be safe. I've replaced it with a loop.

gmaxwell · 2016-04-16T13:01:56Z

meta-concept-ack. A reasonable separation of concerns in the migration off of openssl is time of use addition of OS entropy, a replacement CSPRNG, and replacement seeding. Each of these can be done independently Taking OS entropy at time of use for long term keys is a basic, sensible thing to do and protects users against flaws in the either the OS rng or the process CSPRNG.

The specific combiner used here seems reasonable to me.

kirkalx · 2016-04-17T04:30:32Z

Concept ACK. Minor point though: should use of /dev/urandom be protected by a HAVE_URANDOM or similar? Or is support for it universal on supported non-Windows systems?

gmaxwell · 2016-04-17T19:32:01Z

src/random.cpp

We should be doing something stronger than assert here. If the code is compiled with assertions disabled, this code would be incorrect. Assertions should be used for invariants, not error handling.

Agree, will fix.

Hmm... abort() or raise a C++ exception or depend on ui_interface.h (yuck) to call ThreadSafeMessageBox?

There's not really a good way to do a global abort at the moment from this place.
AbortNode() is the closest, it shows a message and does a more graceful shutdown, but that would introduce a dependency on main.cpp.

To avoid the main dependency, and if you want to structure this as a library, maybe add a function to register a fatal error handler? (which we would point to AbortNode, then raise an exception to kill the current thread). It could still call abort() if nothing registered.

Raise an exception is the most appropriate action here.
If it isn't thorough enough, maybe catch it at a higher level and then call AbortNode?

My worry about exceptions is that it may be caught somewhere without causing a shutdown, and it is hard to verify this for all call sites.

Exceptions should be for reasonably-normal problems which the application could, in principle, handle (it may still AbortNode of them, obviously). This is not one of them.
Really this is more territory for a fatal error handler routine.

sipa · 2016-04-23T16:10:36Z

Added a commit that uses LogPrintf + abort() in case of randomness failure. Adding the ability to install an error handler seems reasonable, but doing it correctly would require locking primitives~~, which would cause libconsensus to end up depending on boost again~~. After the C++11 switchover this will be much easier, and I'd prefer to do that in a separate PR then.

EDIT: I'm wrong, libconsensus does not depend on random. Still, can we keep error handling for another PR?

kirkalx · 2016-04-24T03:33:22Z

Perhaps RandFailure() could take a reason param (which would cover my concern above about relying on /dev/urandom to be present), but as you say, error handling for another PR...

laanwj · 2016-04-25T10:53:34Z

Adding the ability to install an error handler seems reasonable, but doing it correctly would require locking primitives

Couldn't you just require that the error handler is set from initialization before use of any of the functions? (e.g. AppInit2).
You don't have to support the scenario where the error handler is changed while your function is being called.
For example http and httprpc also have setup that has to be done in a single-threaded fashion before the module is used.

In any case I'm fine with doing that in another PR.

should use of /dev/urandom be protected by a HAVE_URANDOM or similar? Or is support for it universal on supported non-Windows systems?

I think /dev/random is universal on UN*X, urandom is less common but most BSDs seem to have it, some only as a link to /dev/random.

sipa · 2016-04-25T14:31:38Z

On OSX /dev/urandom exists but does the same as /dev/random

sipa · 2016-05-05T17:07:53Z

Anything left to do here?

gmaxwell · 2016-05-20T01:11:46Z

ACK (but someone should test on Windows).

paveljanik · 2016-05-20T05:58:09Z

ACK ecc7110

Can't test on Windows though.

paveljanik · 2016-05-24T09:17:32Z

RfM

sipa · 2016-05-24T14:14:02Z

@paveljanik ?

paveljanik · 2016-05-24T14:17:17Z

Ready for Merge

Hmm, but no testing yet on Windows.

sipa · 2016-05-28T23:41:50Z

Tested by compiling using depends/ for win64, and then running bitcoin-qt.exe on native Windows 7 64-bit, and typing getnewaddress few times in the debug console. The resulting addresses were all different.

628cf14 Don't use assert for catching randomness failures (Pieter Wuille) fa2637a Always require OS randomness when generating secret keys (Pieter Wuille)

…et keys 628cf14 Don't use assert for catching randomness failures (Pieter Wuille) fa2637a Always require OS randomness when generating secret keys (Pieter Wuille)

luke-jr reviewed Apr 16, 2016
View reviewed changes

sipa force-pushed the betterrng branch 2 times, most recently from cb5aede to 3a68c3a Compare April 16, 2016 11:08

gmaxwell reviewed Apr 17, 2016
View reviewed changes

laanwj added the Wallet label Apr 18, 2016

sipa force-pushed the betterrng branch from 3a68c3a to ecc7110 Compare April 23, 2016 16:08

laanwj mentioned this pull request May 9, 2016

Increase DEFAULT_KEYPOOL_SIZE to 10000. #8025

Closed

sipa added 2 commits May 29, 2016 01:52

Always require OS randomness when generating secret keys

fa2637a

Don't use assert for catching randomness failures

628cf14

sipa force-pushed the betterrng branch from ecc7110 to 628cf14 Compare May 28, 2016 23:52

sipa merged commit 628cf14 into bitcoin:master May 30, 2016

paveljanik mentioned this pull request May 31, 2016

[Wallet] Add simplest BIP32/deterministic key generation implementation #8035

Merged

daira mentioned this pull request Oct 25, 2016

Replace all uses of OpenSSL RNG with libsodium's RNG zcash/zcash#1632

Closed

This was referenced Dec 21, 2016

V0.12.0.x [build] fix deprecated download paths dashpay/dash#1219

Merged

bug: should use txidCollateral to calculate confirmations dashpay/dash#1188

Merged

dagurval mentioned this pull request Jan 22, 2018

[wip] Use libsodium for GetRandBytes bitcoinxt/bitcoinxt#307

Closed

IgorDurovic mentioned this pull request Apr 2, 2018

Utils and libraries: back porting bitcoin RNG improvement PIVX-Project/PIVX#576

Closed

sickpig mentioned this pull request Oct 24, 2019

[port] Update BU random number generator BitcoinUnlimited/BitcoinUnlimited#1997

Merged

barton2526 mentioned this pull request Jul 16, 2021

util: Remove deprecated random number generator functions gridcoin-community/Gridcoin-Research#2236

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Always require OS randomness when generating secret keys #7891

Always require OS randomness when generating secret keys #7891

Uh oh!

Conversation

sipa commented Apr 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr Apr 16, 2016

Choose a reason for hiding this comment

Uh oh!

sipa Apr 16, 2016

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented Apr 16, 2016

Uh oh!

kirkalx commented Apr 17, 2016

Uh oh!

gmaxwell Apr 17, 2016

Choose a reason for hiding this comment

Uh oh!

sipa Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

sipa Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj Apr 20, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj Apr 20, 2016 • 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!

dcousens Apr 21, 2016 • 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!

sipa Apr 21, 2016 via email

Choose a reason for hiding this comment

Uh oh!

laanwj Apr 22, 2016 • 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!

sipa commented Apr 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kirkalx commented Apr 24, 2016

Uh oh!

laanwj commented Apr 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Apr 25, 2016

Uh oh!

sipa commented May 5, 2016

Uh oh!

gmaxwell commented May 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paveljanik commented May 20, 2016

Uh oh!

paveljanik commented May 24, 2016

Uh oh!

sipa commented May 24, 2016

Uh oh!

paveljanik commented May 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented May 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sipa commented Apr 16, 2016 •

edited

Loading

laanwj Apr 20, 2016 •

edited

Loading

dcousens Apr 21, 2016 •

edited

Loading

laanwj Apr 22, 2016 •

edited

Loading

sipa commented Apr 23, 2016 •

edited

Loading

laanwj commented Apr 25, 2016 •

edited

Loading

gmaxwell commented May 20, 2016 •

edited

Loading

paveljanik commented May 24, 2016 •

edited

Loading