refactor: Use strong libcrypto randomness instead of custom random#5726
refactor: Use strong libcrypto randomness instead of custom random#5726kaukabrizvi wants to merge 20 commits intoaws:mainfrom
Conversation
utils/s2n_random.c
Outdated
| return false; | ||
| #else | ||
| return s2n_libcrypto_is_openssl() && !s2n_is_in_fips_mode(); | ||
| return s2n_libcrypto_is_openssl() && !s2n_is_in_fips_mode() && !s2n_use_libcrypto_rand(); |
There was a problem hiding this comment.
Nit: I think s2n_use_libcrypto_rand already includes a fips mode check no?
Actually, do you think that this s2n_supports_custom_rand() function can be cleaned up at all? It seems to be testing libcrypto capabilities, e.g. if this engine support exists and hasn't been explicitly disabled by users.
It's possible it can't be cleaned up. But it looks weird to me.
There was a problem hiding this comment.
I took a look and don’t think this can be simplified much. There are 4 checks needed before delegating crypto layer randomness to the custom rand:
- Libcrypto supports engine (OpenSSL)
- Exclude OpenSSL 1.0.2-FIPS
- User disabled rand override
- s2n is using libcrypto rand
I suppose this means that removing s2n_libcrypto_is_openssl() could clean it up slightly. To my knowledge, no other libcryptos that we support also support engine, so this check is implictly included above. I'm hoping to remove this logic altogether in a follow-up PR which deals with OpenSSL 1.0.2 randomness; that should clean things up a lot😅
6b3f106 to
65239a0
Compare
|
@maddeleine @CarolYeh910 thank you for the feedback. After reviewing this more closely, I determined that the chunking logic is unnecessary because libcrypto already handles large request sizes internally. I’ve removed that logic. With chunking removed, we’re also no longer passing RAND_*bytes through a generic function pointer, so the adapter logic is no longer needed either. |
Goal
This change brings us one step closer to deprecating s2n's custom DRBG by sourcing entropy from strong backend libcrypto's randomness implementation when available.
Why
Over the years, maintaining our own custom DBRG implementation has led to several issues, most of which relate to portability concerns. See #4348 and docs/design/DRBG.md in this PR for more.
How
We currently gate libcrypto backend delegation for randomness based on wether or not s2n is built in FIPS mode. This PR refactors the logic to delegate randomness based on libcrypto feature detection.
With these changes, we delegate randomness to the libcrypto if it supports a seperate public/private module for randomness. This is required to uphold our security guarantee that public entropy which is visible on the wire cannot be used to leak information about the private entropy.
As part of this PR, I modified tests which previously exercised the "backend randomness in use" path by observing FIPS mode to now gate based on the
random_uses_libcryptoindicator instead. I also added fixes to typos in two randomness tests as part of this PR which I noticed when looking over the files.Callouts
This PR does not completely remove the DRBG module in s2n-tls because there are still two libcrypto cases that do not meet our standard: OpenSSL 1.0.2 and versions of AWS-LC prior to aws/aws-lc#2963 do not support public/private random. These cases will be handled in a follow-up PR alongside full deprecation of the DRBG module.
Testing
Since this PR re-factors existing randomness branching, I updated tests that relied on previous assertions as to when the backend libcrypto randomness was being utilized. I also added testing logic in
s2n_random_implementation_testto ensure that, when the libcrypto random is used outside of FIPS mode, the sources of randomness for public/private are distinct symbols.Related
#4348
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.