-
Notifications
You must be signed in to change notification settings - Fork 722
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
fix: SSLv3 handshake with openssl-1.0.2-fips fails #4644
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case S2N_HMAC_SSLv3_SHA1:
/* Some libcryptos, such as OpenSSL, disable MD5 by default when in FIPS mode, which is
* required in order to negotiate SSLv3. However, this is supported in AWS-LC.
*/
return !s2n_is_in_fips_mode() || s2n_libcrypto_is_awslc();
Have you checked to see if that is true? What happens if you remove that check? Where does the error occur?
I tried removing |
tests/unit/s2n_hmac_test.c
Outdated
@@ -52,6 +63,7 @@ int main(int argc, char **argv) | |||
uint8_t hmac_sslv3_md5_size = 0; | |||
POSIX_GUARD(s2n_hmac_digest_size(S2N_HMAC_SSLv3_MD5, &hmac_sslv3_md5_size)); | |||
EXPECT_EQUAL(hmac_sslv3_md5_size, 16); | |||
EXPECT_OK(s2n_allow_md5_for_fips(&hmac)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right to me. This code is gated on "s2n_hmac_is_available(S2N_HMAC_SSLv3_MD5)". How can S2N_HMAC_SSLv3_MD5 be considered available if you have to call a method that doesn't exist outside of this test (and really shouldn't) in order to use S2N_HMAC_SSLv3_MD5? That sounds like S2N_HMAC_SSLv3_MD5 being unavailable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s2n_hmac_is_available() doesn't really check if the algorithm is compatible with certain libcrypto and simply checks if we can recognize the given input as an HMAC algorithm (especially after my change in s2n_hmac.c). So although this test is gated by that, FIPS mode still requires a specific allowance for MD5 to be used in FIPS mode. s2n_allow_md5_for_fips() usually gets called during the handshake to enable MD5 for FIPS, but in this test we are only testing the hash, so it needs to be manually called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If s2n_hmac_is_available() doesn't check if the algorithm is compatible with the current libcrypto, that would be different behavior from all the other s2n-tls is_available() methods. Why would it be called "is_available" instead of something like "is_valid" if the intended behavior was only to check whether the input is valid?
If s2n_hmac_is_available() reports "true", I should be able to use that hmac algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that was partially the intended behavior originally since it was specifically blocking the use of hmac in fips mode. This logic was removed in this PR because it doesn't seem like OpenSSL fips module prevents the use of MD5 in any ways, which does not align with the comment:
/* Some libcryptos, such as OpenSSL, disable MD5 by default when in FIPS mode, which is
* required in order to negotiate SSLv3. However, this is supported in AWS-LC.
*/
I will go ahead and change the function name from s2n_hmac_is_available
to s2n_hmac_is_valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failing case was actually caused by attempting to use SHA1 for hmac. I applied the suggested change so that s2n_hmac_is_available() still gate the use of MD5 in fips, but will allow SHA1. s2n_hmac_test confirms that it is able to initialize hmac with SHA1 without manually enabling the flag. We also have s2n_sslv3_test which tests SSLv3 handshake and can confirm that openssl-1.0.2-fips is able to do SSLv3 handshake.
- fix s2n_hmac_is_available conditions
- remove check for s2n_hmac_is_available(S2N_HMAC_SSLv3_SHA1)
Resolved issues:
Resolves #4476
Description of changes:
We cannot perform SSLv3 handshake when built with OpenSSL-1.0.2-fips. The reason is documented below.
This change will allow the use of MD5 algorithm in FIPS mode, which fixes handshake failures we were observing in #4476, as well as allowing SHA1 to be used for HMAC in SSLv3.
Call-outs:
Root cause
MD5 is generally not FIPS compliant and the client tries to initialize hash with MD5
s2n-tls/crypto/s2n_hash.c
Lines 111 to 117 in a99299a
However we allow MD5 for FIPS if the digest has the correct flag to indicate that it is available for FIPS.
aws-lc is able to use MD5 because in the function call to check if MD5 is allowed for FIPS, we always enable MD5 if s2n-tls is built with aws-lc:
s2n-tls/crypto/s2n_evp.c
Lines 37 to 49 in a99299a
In the failing case,
EVP_MD_CTX_test_flags()
returns false, which results in throwingS2N_ERR_HASH_INVALID_ALGORITHM
Solution
We can fix this issue by inserting the following block of code
right before this line to enable MD5:
s2n-tls/tls/s2n_prf.c
Line 160 in 7a7fae2
As well as allowing the use of HMAC in FIPS mode when using SHA1:
s2n-tls/crypto/s2n_hmac.c
Lines 79 to 88 in 7a7fae2
Testing:
Confirmed all unit tests and existing failure case is now passing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.