Always require OS randomness when generating secret keys #7891

sipa commented at 10:51 AM on April 16, 2016: member

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.

in src/random.cpp:None in e5ef17c5b1 outdated

  96 | +    CryptReleaseContext(hProvider, 0);
  97 | +#else
  98 | +    int f = open("/dev/urandom", O_RDONLY);
  99 | +    assert(f != -1);
 100 | +    int n = read(f, ent32, 32);
 101 | +    assert(n == 32);

luke-jr commented at 10:56 AM on April 16, 2016:

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

sipa commented at 11:06 AM on April 16, 2016:

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.

sipa force-pushed on Apr 16, 2016

gmaxwell commented at 1:01 PM on April 16, 2016: contributor

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 commented at 4:30 AM on April 17, 2016: contributor

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?

in src/random.cpp:None in 3a68c3afb2 outdated

  96 | +    assert(ret);
  97 | +    CryptReleaseContext(hProvider, 0);
  98 | +#else
  99 | +    int f = open("/dev/urandom", O_RDONLY);
 100 | +    assert(f != -1);
 101 | +    int have = 0;

gmaxwell commented at 7:32 PM on April 17, 2016:

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.

sipa commented at 6:09 PM on April 19, 2016:

Agree, will fix.

sipa commented at 6:16 PM on April 19, 2016:

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

laanwj commented at 12:48 PM on April 20, 2016:

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.

laanwj commented at 12:49 PM on April 20, 2016:

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.

dcousens commented at 3:47 AM on April 21, 2016:

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?

sipa commented at 4:39 AM on April 21, 2016:

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.

laanwj commented at 6:30 AM on April 22, 2016:

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.

laanwj added the label Wallet on Apr 18, 2016

sipa force-pushed on Apr 23, 2016

sipa commented at 4:10 PM on April 23, 2016: member

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 commented at 3:33 AM on April 24, 2016: contributor

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 commented at 10:53 AM on April 25, 2016: member

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 commented at 2:31 PM on April 25, 2016: member

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

sipa commented at 5:07 PM on May 5, 2016: member

Anything left to do here?

gmaxwell commented at 1:11 AM on May 20, 2016: contributor

ACK (but someone should test on Windows).

paveljanik commented at 5:58 AM on May 20, 2016: contributor

ACK https://github.com/bitcoin/bitcoin/pull/7891/commits/ecc71103fe9266ae42cb12853a258150c9e36249

Can't test on Windows though.

paveljanik commented at 9:17 AM on May 24, 2016: contributor

RfM

sipa commented at 2:14 PM on May 24, 2016: member

@paveljanik ?

paveljanik commented at 2:17 PM on May 24, 2016: contributor

Ready for Merge

Hmm, but no testing yet on Windows.

sipa commented at 11:41 PM on May 28, 2016: member

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.

Always require OS randomness when generating secret keys fa2637a3be

Don't use assert for catching randomness failures 628cf1440a

sipa force-pushed on May 28, 2016

sipa merged this on May 30, 2016

sipa closed this on May 30, 2016

sipa referenced this in commit 950be19727 on May 30, 2016

codablock referenced this in commit 0f69b0e845 on Sep 16, 2017

codablock referenced this in commit 486bdc4036 on Sep 19, 2017

codablock referenced this in commit 43cbeb7fa5 on Dec 21, 2017

andvgal referenced this in commit c70c109814 on Jan 6, 2019

MarcoFalke locked this on Sep 8, 2021