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

[WIP] [Crypto] Use stronger rand for key generation #643

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Warrows
Collaborator

Warrows commented Jun 29, 2018

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

@wafflebot wafflebot bot added the review label Jun 29, 2018

@Mrs-X

Mrs-X requested changes Jun 29, 2018 edited

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 Warrows changed the title from [Crypto] Use stronger rand for key generation to [WIP] [Crypto] Use stronger rand for key generation Aug 7, 2018

theuni and others added some commits Jan 21, 2015

openssl: abstract out OPENSSL_cleanse
This makes it easier for us to replace it if desired, since it's now only in
one spot. Also, it avoids the openssl include from allocators.h, which
essentially forced openssl to be included from every compilation unit.
Switch memory_cleanse implementation to BoringSSL's to ensure memory …
…clearing even with link-time optimization.

The implementation we currently use from OpenSSL prevents the compiler from optimizing away clensing operations on blocks of memory that are about to be released, but this protection is not extended to link-time optimization. This commit copies the solution cooked up by Google compiler engineers which uses inline assembly directives to instruct the compiler not to optimize out the call under any circumstances. As the code is in-lined, this has the added advantage of removing one more OpenSSL dependency.

Regarding license compatibility, Google's contributions to BoringSSL library, including this code, is made available under the ISC license, which is MIT compatible.

BoringSSL git commit: ad1907fe73334d6c696c8539646c21b11178f20f
@Warrows

This comment has been minimized.

Collaborator

Warrows commented Oct 28, 2018

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.

sipa added some commits Apr 16, 2016

@Warrows Warrows added this to the Future milestone Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment