Remove OpenSSL #10299

1. 
   sipa commented at 5:58 pm on April 29, 2017: member

   This PR removes the need for OpenSSL in bitcoind, and the direct uses of it in the GUI.

   It introduces a simple PRNG wrapper around GetOSRand() that protects against some cases of VM duplication:

   • tmp = SHA512(time() || stack_pointer || os_random() || state)
   • seed = tmp[0:32]
   • state = tmp[32:64]
   • Use seed as key for ChaCha20 to produce desired randomness.

   Then that wrapper is used to implement GetRandBytes, and GetStrongRandBytes is merged with it. This is overkill for some use cases, and they can later be replaced with FastRandomContext-based solutions (which is pretty strong now, since #9792).

   Our cleanse function is also replaced with a self-implemented best-effort explicit zeroing.

   It does not remove OpenSSL from existing unit tests or build system.

2. sipa force-pushed on Apr 29, 2017
3. 
   gmaxwell commented at 6:18 pm on April 29, 2017: contributor

   I’d like to see us pull most seeder stuff out of the earlier randomness PR to set that initial state to protect against cases like we saw in netbsd where the OS rng was producing no randomness of use.
4. 
   sipa commented at 6:31 pm on April 29, 2017: member

   @gmaxwell As that’s trying to protect against a scenario that the current OpenSSL-based approach is unlikely to be protecting against, I think that’s something that can easily be done in a later PR.
5. 
   gmaxwell commented at 7:25 pm on April 29, 2017: contributor

   @sipa OpenSSL used a collection of system pointers and timestamps, and our use on windows also adds a screenshot. I agree that they can be considered separately, but we should do the improved seeding first so that we’re not potentially introducing a vulnerability on systems where the os entropy isn’t helpful.
6. fanquake added the label Refactoring on Apr 29, 2017
7. 
   sipa commented at 6:58 pm on April 30, 2017: member

   @gmaxwell I had a look at what OpenSSL 1.1.0 uses as RNG seeding (latest stable release):

   • On UNIX systems
     • read /dev/urandom / EGD / arc4random bytes (through a poll loop, which does not gather timing information)
     • The (internally cached!) current pid
     • getuid()
     • time() (seconds resolution)
   • On Windows:
     • CryptGenRandom (OS entropy, like we do)
     • Intel HW rng (through OS call)
     • Timing information (tsc if available, GetPerformanceCounter like we do, GetTickCount)
     • Memory usage statistics
     • Current pid
     • Since https://github.com/openssl/openssl/pull/1079, RAND_screen does not actually read the screen anymore, but is an alias for RAND_poll (which uses the above steps as source).

   If we include the missing sources (pid/uid/rdrand/tsc/memusage) in our GetRandBytes call, do you agree it is a strict improvement?

8. 
   gmaxwell commented at 8:43 pm on April 30, 2017: contributor

   Yes, I would… though I don’t see why we shouldn’t just take the entropy collection function we wrote before, which (at least on linux) was pretty comprehensive and really not complex at all and use that as the seed. We could do it in a commit merged before this one too, it would just feed openssl.

   I think what we should be doing is in the realtime call we would read things which are relatively cheap (e.g. TSC) and will change, and an initial call that sets the initial randomness, includes things that don’t change during execution and/or are more expensive to do (e.g. dumping /etc/passwd into it, running a cpu-expensive KDF on the result)

9. 
   sipa commented at 8:49 pm on April 30, 2017: member

   @gmaxwell Again, if everyone is fine with just doing that, great. But if all of that needs to become a separately-maintained library first, then I don’t think it should be a blocker for this PR.

   I do not believe that protecting against broken OSes should be a priority for us. In general, OSses are in a much better position to provide an RNG than we are. If the OS RNG is broken, generally all bets are off, including remote access to your system. It’s a worthwhile goal to provide a best effort security in that case, but not if it comes at the cost of extra maintenance work that this project isn’t willing to take on.

10. 
    gmaxwell commented at 9:00 pm on April 30, 2017: contributor

    We have seen more instances of OS RNGs being broken than OpenSSL’s RNG being broken in the last 5 years.

    To me it sounds like you are proposing increasing exposure to real issues that we have actually seen in order to eliminate crufty code which is time tested. I don’t believe that this is a good trade-off.

    Doubly so because our process for generating long lived secrets is already combined with the OS rng in a way that should basically make it impossible for an OpenSSL fault to screw our long term key generation. In my view 95% of the purpose of the OpenSSL randomness pool is protecting against the OS’s behavior.

11. 
    gmaxwell commented at 9:08 pm on April 30, 2017: contributor

    Unrelated to this debate: I think you should considering adding a counter to the reseeder. e.g. static unsigned uint32_t entropy_count = 0; and in the lock append it to the hash and update it. It’s another source of real-time entropy which is effectively free, as it’s protected by the same lock as the entropy pool.
12. 
    sipa commented at 9:19 pm on April 30, 2017: member

    @gmaxwell I considered doing that, but given that it’s effectively a hash chain, unless the hash function is horribly broken, a counter does not add anything. In fact, increasing the state to 36 bytes should be a better use of 4 bytes of memory than a counter.
13. 
    gmaxwell commented at 10:35 pm on April 30, 2017: contributor

    I thought about that too, though I believe the counter would allow us to make stronger proofs about the cycle length of the function while relying on much less strong assumptions about the behavior of the hash– similar to how fortuna uses a permutation in order to make very strong proofs about the cycle length. I suppose we could always change it later if someone wants to do the formal work to verify it.

    The fact that it has compromise resistance (by new entropy constantly being added in the form of time/stack pointer/os entropy) means the cycle length is probably not much of a concern. Without that, then there would be weak seeds, though they’d be cryptographically hard to find, where the RNG output would have short cycles (the frequency of short cycles in random functions is surprisingly high)– and these would be eliminated by a counter unless you allowed a really implausible hash function.

14. Drop OpenSSL PRNG, always use OS entropy a7d7a77c7c
15. Drop GetStrongRandBytes, use GetRandBytes everywhere 27c183253e
16. Use explicit zeroing instead of OpenSSL cleanse 783d01d341
17. Remove OpenSSL initialization ef53113b2e
18. Add FastRandomContext::rand256() ace4f211d8
19. Add FastRandomContext::randbytes() 810926850d
20. Switch all tests to FastRandomContext

    No need for real OS randomness in general in tests.

    365f352e49
21. sipa force-pushed on May 2, 2017
22. 
    sipa commented at 11:01 pm on May 2, 2017: member

    Going to close this and propose some other RNG changes first.
23. sipa closed this on May 2, 2017

    ---

24. 
    Sjors commented at 4:59 pm on February 24, 2018: member

    Don’t forget to move openssl to qt_packages in depends/packages/packages.mk if you try this again. Assuming that’s the last piece.
25. Sjors referenced this in commit dbec6836eb on Feb 24, 2018
26. Sjors referenced this in commit a76a633758 on Feb 24, 2018
27. DrahtBot locked this on Sep 8, 2021

Contributors
sipa gmaxwell Sjors

Labels
Refactoring