refactor: remove openssl-1.0.2-fips 'allow md5' logic #5048
Conversation
lrstewart
force-pushed
the
openssl3fips_hash_1
branch
2 times, most recently
from
61a6c2a to
2e4811b
Compare
lrstewart
force-pushed
the
openssl3fips_hash_1
branch
from
2e4811b to
9bfe1fa
Compare
| S2N_ERROR_IF(!s2n_is_in_fips_mode() || (evp_digest->ctx == NULL), S2N_ERR_ALLOW_MD5_FOR_FIPS_FAILED); | ||
|
|
||
| #if !defined(OPENSSL_IS_BORINGSSL) && !defined(OPENSSL_IS_AWSLC) | ||
| EVP_MD_CTX_set_flags(evp_digest->ctx, EVP_MD_CTX_FLAG_NON_FIPS_ALLOW); | ||
| #endif | ||
| return S2N_SUCCESS; |
There was a problem hiding this comment.
This translates to "if openssl+fips, call an API to allow md5". With the removal of openssl-1.0.2-fips, we no longer support openssl+fips, and this is a no-op. I've removed it everywhere it appears.
There was a problem hiding this comment.
Does Openssl-3-fips support the use of md5? Just want to make sure we won't need to add any of these logic back when we add support for openssl-3-fips
There was a problem hiding this comment.
openssl-3.0-fips doesn't support the use of md5, or a couple other options. But openssl-3.0 also doesn't support enabling MD5 this way. There's no overlap between how openssl-3.0 does fips and how openssl-1.0.2 did fips. We can't reuse the openssl-1.0.2-fips logic, so it's cleaner to just tear it out where we find it. And this particular logic is way more complicated / deceptive than it needs to be anyway.
| *out = false; | ||
| #if !defined(OPENSSL_IS_BORINGSSL) && !defined(OPENSSL_IS_AWSLC) | ||
| if (s2n_is_in_fips_mode() && evp_digest && evp_digest->ctx && EVP_MD_CTX_test_flags(evp_digest->ctx, EVP_MD_CTX_FLAG_NON_FIPS_ALLOW)) { | ||
| /* s2n is in FIPS mode and the EVP digest allows MD5. */ | ||
| *out = true; | ||
| } | ||
| #else | ||
| if (s2n_is_in_fips_mode()) { | ||
| /* If s2n is in FIPS mode and built with AWS-LC or BoringSSL, there are no flags to check in the EVP digest to allow MD5. */ | ||
| *out = true; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
This one's a bit trickier. It roughly translates to:
if not fips: false
If openssl+fips: maybe true
If awslc+fips: true
So with the removal of openssl+fips via openssl-1.0.2-fips, this method boils down to "is fips?".
We only use this method for two purposes (see search):
- Gate calls to s2n_hash_allow_md5_for_fips: unnecessary since s2n_hash_allow_md5_for_fips is a no-op
- Decide whether or not to initialize a handshake hash: unnecessary because of refactor: remove openssl-1.0.2-fips 'allow md5' logic #5048 (comment). Basically, we check "is not fips, or is fips".
| /* | ||
| * TODO: update all CBMC proofs that depend on this file, then delete. | ||
| */ |
There was a problem hiding this comment.
If I delete this file, I have to update ALL of the s2n_hash CBMC proofs because they all declare it as a source file. It's unnecessary noise, so I'd prefer to do it in a follow-up PR: fe97ffa
| /* return false if in FIPS mode, as MD5 algs are not available in FIPS mode. */ | ||
| return !s2n_is_in_fips_mode(); |
There was a problem hiding this comment.
This was not really true even before this change. You could use md5 with fips in awslc, just not in openssl-1.0.2. So whenever we checked s2n_hash_is_available and got "false" because of fips, we'd then check s2n_digest_is_md5_allowed_for_fips and get "true", overriding the original "false". I'm just skipping straight to the final "true".
You can confirm the limited non-test usage of this method: https://github.com/search?q=repo%3Aaws%2Fs2n-tls+s2n_hash_is_available+-path%3A*tests%2Funit%2F*.c&type=code
There was a problem hiding this comment.
Since we are here, can we also update this comment to reflect the current behavior?
The handshake MD5 hash state will fail the s2n_hash_is_available() check since MD5 is not permitted in FIPS mode is no longer correct
s2n-tls/tls/s2n_handshake_transcript.c
Lines 50 to 55 in 431f014
| /* Re-initialize hashes for remaining tests */ | ||
| EXPECT_SUCCESS(s2n_hash_init(&hash_one, S2N_HASH_SHA512)); | ||
| EXPECT_SUCCESS(s2n_hash_init(&hash_two, S2N_HASH_SHA512)); |
There was a problem hiding this comment.
This test just kept using the hashes initialized to whatever hash algorithm was set last in the loop above, which just happens to be sha512 (the md5+sha1 was previously skipped). I kept the sha512 "choice", but made it explicit.
| EXPECT_FAILURE_WITH_ERRNO(sign_result, S2N_ERR_HASH_INVALID_ALGORITHM); | ||
| } |
There was a problem hiding this comment.
Instead of just skipping the md5 algorithms, I checked that they really aren't supported so that we can't lose valid coverage.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Release Summary:
Resolved issues:
related to #5045. We need to cleanup the old mess before we add a new mess.
Description of changes:
I remove the s2n_hash_allow_md5_for_fips, s2n_digest_is_md5_allowed_for_fips, and s2n_digest_allow_md5_for_fips methods because after the removal of openssl-1.0.2-fips support, they're not actually doing anything. I explain more in in-line comments, but:
Call-outs:
if you search for the removed methods, you'll still find references in the CBMC proofs. To keep this PR reviewable, I'm going to clean those up as a follow-up, since they don't affect testing.
Testing:
Existing tests continue to pass. I only needed to update one unit test, and I explain why in-line.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.