util: Remove deprecated random number generator functions

[jamescowens]

This commit changes CInit to check for proper RNG seeding and use RAND_poll() if necessary. It removes RandAddSeed() RandAddSeedPerfmon() and also the use of RAND_screen. [Update - RandAddSeed() and RandAddSeedPerfmon() have been retained, but a macro conditional changes what they do.]

OpenSSL in 1.1.0+ has fixed entropy source issues that obviate the kludges that were in the code to collect additional entropy from Performance Counters/screen etc. In 1.1.0+, OpenSSL automatically seeds (and reseeds) the RNG with entropy from trusted sources without intervention. Obviously the super simple approach to this is something like what I have done in this PR.

There are several big issues remaining however:

1. Some operating systems that are still being supported are using 1.0.2 such as Ubuntu Xenial (16.04), which has problems with the RNG that were fixed in 1.1.0+.
2. The documentation for RAND in OpenSSL states explicity that the status (error return value ) of RAND_bytes() should be checked at each call, because a transient failure of the entropy sources could cause the RNG to not have enough entropy and then degrade into not providing valid random numbers. As far as I can tell this is not done in ANY of the code. RTFM. We should at a minimum put a wrapper around the vanilla RAND_bytes() with a reseed try using RAND_poll() if the call to RAND_bytes() gives an unsuccessful status.
3. A short review of Bitcoin's current RNG code in random.cpp/h shows a far more advanced approach where they essentially largely wrote their own RNG. I am not sure we are ready to drop-ship that in yet.

If people are nervous about removing the RandAddSeed / RandAddSeedPerfmon in places that originally were critical points where more entropy was desired, we could simply call RAND_poll() there, or put RandAddSeed() back in place as a simple wrapper.

Comments?

[barton2526]

Upstream solutions (in no particular order):

• add GetRandBytes() as wrapper for RAND_bytes() bitcoin/bitcoin#4400
• gui: remove OpenSSL PRNG seeding (Windows, Qt only) bitcoin/bitcoin#17151
• random: remove call to RAND_screen() (Windows only) bitcoin/bitcoin#17191
• Always require OS randomness when generating secret keys bitcoin/bitcoin#7891
• Add RNG strengthening (10ms once every minute) bitcoin/bitcoin#15224

and then their current implementation:

• Switch all RNG code to the built-in PRNG bitcoin/bitcoin#14955

[jamescowens]

I have elected a very minimal approach to this PR after thinking about what we need to do. I am using a macro definition conditional to only use the performance counter key information on Windows and OpenSSL < 1.1, since that is the only situation based on the documentation where the entropy source may not be adequate. RandAddSeedPerfmon() now reduces to RandAddSeed only effectively on all but Win32 and OpenSSL < 1.1.

I have left the calls to RandAddSeedPerfmon() in the code, even though they are probably not necessary in OpenSSL 1.1 and greater.

This should be adequate for our transition from OpenSSL 1.0+ to 1.1+.

The RAND_event and RAND_screen have been removed, because they are only relevant on Win32 for OpenSSL < 1.1, and we compile the Windows production releases with depends which uses OpenSSL >= 1.1.

[jamescowens]

Then we may not be able to do it in the constructor, as that is too early I think. I will consider it if it is not too difficult to wire up.

…

Sent from my iPhone

On Aug 14, 2021, at 10:47 AM, div72 ***@***.***> wrote:  @div72 commented on this pull request. In src/util.cpp: > - // Seed OpenSSL PRNG with performance counter - RandAddSeed(); + if (seed_successful) break; + + MilliSleep(100); + } + + if (!seed_successful) std::abort(); This is an unlikely condition to reach but I would prefer if the user could be alerted that the RNG is not able to be seeded, rather than just aborting. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

[div72]

@jamescowens Outputting to stdout would be enough, just silently exiting bothers me.

[jamescowens]

I think stderr is fine. I will put up the change.

[div72]

utACK.