Always require OS randomness when generating secret keys #7891

pull sipa wants to merge 2 commits into bitcoin:master from sipa:betterrng changing 7 files +75 −19
  1. 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.

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

  3. sipa force-pushed on Apr 16, 2016
  4. sipa force-pushed on Apr 16, 2016
  5. 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.

  6. 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?

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

  8. laanwj added the label Wallet on Apr 18, 2016
  9. sipa force-pushed on Apr 23, 2016
  10. 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?

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

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

  13. sipa commented at 2:31 PM on April 25, 2016: member

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

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

    Anything left to do here?

  15. laanwj cross-referenced this on May 9, 2016 from issue Increase DEFAULT_KEYPOOL_SIZE to 10000. by pstratem
  16. gmaxwell commented at 1:11 AM on May 20, 2016: contributor

    ACK (but someone should test on Windows).

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

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

    RfM

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

    @paveljanik ?

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

    Ready for Merge

    Hmm, but no testing yet on Windows.

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

  22. Always require OS randomness when generating secret keys fa2637a3be
  23. Don't use assert for catching randomness failures 628cf1440a
  24. sipa force-pushed on May 28, 2016
  25. sipa merged this on May 30, 2016
  26. sipa closed this on May 30, 2016
  27. sipa referenced this in commit 950be19727 on May 30, 2016
  28. paveljanik cross-referenced this on May 31, 2016 from issue [Wallet] Add simplest BIP32/deterministic key generation implementation by jonasschnelli
  29. daira cross-referenced this on Oct 25, 2016 from issue Replace all uses of OpenSSL RNG with libsodium's RNG by daira
  30. schinzelh cross-referenced this on Dec 21, 2016 from issue V0.12.0.x [build] fix deprecated download paths by schinzelh
  31. schinzelh cross-referenced this on Dec 21, 2016 from issue bug: should use txidCollateral to calculate confirmations by UdjinM6
  32. codablock referenced this in commit 0f69b0e845 on Sep 16, 2017
  33. codablock referenced this in commit 486bdc4036 on Sep 19, 2017
  34. codablock referenced this in commit 43cbeb7fa5 on Dec 21, 2017
  35. dagurval cross-referenced this on Jan 22, 2018 from issue [wip] Use libsodium for GetRandBytes by dagurval
  36. IgorDurovic cross-referenced this on Apr 2, 2018 from issue Utils and libraries: back porting bitcoin RNG improvement by IgorDurovic
  37. andvgal referenced this in commit c70c109814 on Jan 6, 2019
  38. sickpig cross-referenced this on Oct 24, 2019 from issue [port] Update BU random number generator by sickpig
  39. barton2526 cross-referenced this on Jul 16, 2021 from issue util: Remove deprecated random number generator functions by jamescowens
  40. bitcoin locked this on Sep 8, 2021
Contributors
sipaluke-jrgmaxwellkirkalxlaanwjdcousenspaveljanik
Labels
Wallet
Linked (view graph)
#8025 Increase DEFAULT_KEYPOOL_SIZE to 10000.#8035 [Wallet] Add simplest BIP32/deterministic key generation implementation
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC