[ Security ] Increased entropy from OpenSSL and OS to a full SHA512 …

[ReverseControl]

…block (64 bytes) and added additional entropy from RDSEED on CPUs that support it.

To compile it a flag is needed to enable the RDSEED instruction. I compiled with

./configure CFLAGS='-mrdseed ' CXXFLAGS='-mrdseed ' CPPFLAGS='-mrdseed' --with-incompatible-bdb

As for the actual changes:

1. It is unclear to me why we only used half a SHA512 block to gather entropy from OpenSSL, and the same for OS entropy.
2. I increased that entropy gathering to a full block for each one.
3. I added entropy from the RDSEED instruction implemented on x86 by Intel and AMD.
4. The entropy from RDSEED is NIST compliant. ( See https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/details?product=11882)
5. All AMD Zen processors support this instruction and most Intel Processors launched since 2015.
6. The existence of this instruction is checked at runtime, if it is not available we default to the previous mechanism where we gather entropy from OpenSSL and the OS. Only we gather twice as much as before to get one full SHA512 block when hashing.

EDIT 1: @michilumin @rnicoll @patricklodder @edtubbs EDIT2: Fixed typo, from RNSEED to RDSEED.

[ReverseControl]

I am looking at all the errors...its a different error for each system. hahahhahahahahaha

Okay, here we go:

1. The ARM errors: I think the thing to do here is to do compile time headers and default to the previous version with double the entropy gathering (See notes on the PR).
2. For the 32-bit systems, I will default back to 32-bit registers to gather the data.
3. For 64-bit systems, the issue is the compiler flag I mentioned in the PR.
4. Apparently MacOS does not have the same cpuid.h as found in linux, which makes sense. In particular, with the new processors like the M1, I think the best course of action is the same one as for ARM. Default back to previous method with double the entropy gathering.
5. The 64-bit windows error is the same as the one I got. Solving the compiler flag will solve this one too.

@patricklodder @michilumin @edtubbs

[ReverseControl]

We are now getting the same error on all but the MacOS platform. Those errors will be fixed when the flags are added to the compiler.

As for MacOS, the fix is defining the bit_RDSEED macro as it seems #include <cpuid.h> on MacOS is not defined. I will just define it myself if it is not defined.

[ReverseControl]

@patricklodder @rnicoll Okay, ARM and MacOS are ready to go. We need to add the compiler flags to fix the others. Alternatively, we could just use the byte code implementation of RDSEED, but I'd rather fix the intrinsic flag issue now as it will come up again.

[patricklodder]

like with sse2, I'd like a ./configure flag for this unlike with sse2, we should probably give some thoughts about what the default can/should be

[ReverseControl]

@patricklodder Given this particular implementation checks for availability of this instruction at runtime, the default on the compiling side should be to always enable it. How that is done is up to discussion.

[patricklodder]

I think I can be okay with a default enable, as long as it also can be switched off, to enhance portability to architectures we currently do not test for / do not foresee. So for that to be nice, we'd just add --disable-rnseed to configure.ac, which can override a use_rnseed=yes as a configure flag. Kinda like how zmq integration is done - default on, can be disabled.

Thoughts?

[ReverseControl]

@patricklodder That's perfect.

[ReverseControl]

@patricklodder @rnicoll How do I detect ARM at the configure.ac level? That is the only way I can think of to solve this. I can think of another way, but it would probably break the Windows build.

[patricklodder]

How do I detect ARM at the configure.ac level?

You can scan the host triplet $host for arm and aarch

[ReverseControl]

@patricklodder @michilumin Could you please review?

[Mahandrafhwn]

Unsubscribe

On Mon, 30 Aug, 2021, 11:23 am RichDevX, @.***> wrote:

@.**** commented on this pull request.

In src/random.cpp https://github.com/dogecoin/dogecoin/pull/2482#discussion_r698204896:

•            int32_t status1 = _rdseed32_step( &rng_low );
  

•            int32_t status2 = _rdseed32_step( &rng_high );
  

Should place in a retry loop

https://software.intel.com/content/www/us/en/develop/articles/intel-digital-random-number-generator-drng-software-implementation-guide.html#inpage-nav-5-9

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dogecoin/dogecoin/pull/2482#pullrequestreview-741287881, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVMSUCHTZMMZYCY5N3TIVFLT7MMEJANCNFSM5CSGKWIQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[ReverseControl]

@patricklodder Could you confirm: what does uname -p return on the machine that is running the arm linux test?

[edtubbs]

https://software.intel.com/content/www/us/en/develop/blogs/changes-to-rdrand-integration-in-openssl.html According to Intel, OpenSSL is using RDSEED as an entropy source to seed the software PRNG in v1.0.2.. https://github.com/openssl/openssl/blob/1c0eede9827b0962f1d752fa4ab5d436fa039da4/providers/implementations/rands/seeding/rand_cpu_x86.c#L29

[ReverseControl]

@edtubbs Thanks for that reference. That Intel document supports the option to implement RDSEED ourselves, given OpenSSL does not gather entropy from that instruction by default.

[edtubbs]

Actually, it says OpenSSL uses the RDSEED instruction by default if available with RDRAND starting in v1.0.2, and the comments in that version are consistent.

[ReverseControl]

@edtubbs My final comment on this matter: I vote for the inclusion of this code. We maintain this code and can guarantee that sources from the hardware, if available in hardware and sufficient in bytes at running time, will always be used. We will not depend on other projects implementation/usage not even the OS kernel. Virtually no performance cost. Per NIST standard, compliant. Small enough to review carefully.

Per OpenSSL project, note this was written 3 years after the intel doc, (https://www.openssl.org/blog/blog/2017/08/12/random/):

They’re relatively well-commented, but the implementation could to change in the future. 
(referencing entropy sources.)

@michilumin @rnicoll will have the final word on this.

[patricklodder]

@ReverseControl could you please do the rebase/squash on this branch?

If you haven't the remote to this repo already:

git remote add upstream https://github.com/dogecoin/dogecoin.git

then:

git fetch upstream
git rebase -i upstream/1.14.5-dev #and squash all the commits into one
git push -f origin 1.21.5-dev-improve_entropy_for_key_generation

then this PR should show a clean commit tree and becomes mergeable.

[patricklodder]

Pushing to 1.14.6 as this is stalled on rebase.

[ReverseControl]

@patricklodder Done. Do check the rebase.