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

Always require OS randomness when generating secret keys #7891

Merged
merged 2 commits into from May 30, 2016

Conversation

Projects
None yet
7 participants
@sipa
Member

sipa commented Apr 16, 2016

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

View changes

Show outdated Hide outdated src/random.cpp
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Apr 16, 2016

Member

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.

Member

gmaxwell commented Apr 16, 2016

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

This comment has been minimized.

Show comment
Hide comment
@kirkalx

kirkalx Apr 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?

Contributor

kirkalx commented Apr 17, 2016

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

View changes

Show outdated Hide outdated src/random.cpp

@laanwj laanwj added the Wallet label Apr 18, 2016

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 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?

Member

sipa commented Apr 23, 2016

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

This comment has been minimized.

Show comment
Hide comment
@kirkalx

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

Contributor

kirkalx commented Apr 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

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

Member

laanwj commented Apr 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 25, 2016

Member

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

Member

sipa commented Apr 25, 2016

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 5, 2016

Member

Anything left to do here?

Member

sipa commented May 5, 2016

Anything left to do here?

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 20, 2016

Member

ACK (but someone should test on Windows).

Member

gmaxwell commented May 20, 2016

ACK (but someone should test on Windows).

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 20, 2016

Contributor

ACK ecc7110

Can't test on Windows though.

Contributor

paveljanik commented May 20, 2016

ACK ecc7110

Can't test on Windows though.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 24, 2016

Contributor

RfM

Contributor

paveljanik commented May 24, 2016

RfM

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa
Member

sipa commented May 24, 2016

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 24, 2016

Contributor

Ready for Merge

Hmm, but no testing yet on Windows.

Contributor

paveljanik commented May 24, 2016

Ready for Merge

Hmm, but no testing yet on Windows.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

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

Member

sipa commented May 28, 2016

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.

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request May 30, 2016

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

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

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

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

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

codablock added a commit to codablock/dash that referenced this pull request Dec 21, 2017

Merge #7891: Always require OS randomness when generating secret keys
628cf14 Don't use assert for catching randomness failures (Pieter Wuille)
fa2637a Always require OS randomness when generating secret keys (Pieter Wuille)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment