Skip to content
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

Remove OpenSSL #10299

Closed
wants to merge 7 commits into from
Closed

Remove OpenSSL #10299

wants to merge 7 commits into from

Conversation

sipa
Copy link
Member

@sipa sipa commented Apr 29, 2017

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.

@gmaxwell
Copy link
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.

@sipa
Copy link
Member Author

sipa commented Apr 29, 2017

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

@gmaxwell
Copy link
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.

@sipa
Copy link
Member Author

sipa commented Apr 30, 2017

@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 Clean up Windows RNG openssl/openssl#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?

@gmaxwell
Copy link
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)

@sipa
Copy link
Member Author

sipa commented Apr 30, 2017

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

@gmaxwell
Copy link
Contributor

gmaxwell commented Apr 30, 2017

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.

@gmaxwell
Copy link
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.

@sipa
Copy link
Member Author

sipa commented Apr 30, 2017

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

@gmaxwell
Copy link
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.

@sipa
Copy link
Member Author

sipa commented May 2, 2017

Going to close this and propose some other RNG changes first.

@Sjors
Copy link
Member

Sjors commented Feb 24, 2018

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.

Sjors added a commit to Sjors/bitcoin that referenced this pull request Feb 24, 2018
Based on @sipa's bitcoin#10299

UNSAFE AND POORLY TESTED - DO NOT USE THIS
@sipa sipa mentioned this pull request Oct 26, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants