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.

Sign up for GitHub

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

Merged
merged 19 commits into from
Jul 31, 2024
Merged

fix: SSLv3 handshake with openssl-1.0.2-fips fails #4644

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
9c5cd8d
test removing the gating logic
jouho Jul 8, 2024
a5ccfac
remove comments
jouho Jul 8, 2024
105d849
throw error if fips + sslv3 + ossl-1.0.2 tries to handshake
jouho Jul 16, 2024
b71c053
modify gating logic
jouho Jul 16, 2024
f5ee8f8
cleanup
jouho Jul 16, 2024
3818d08
fix unit test
jouho Jul 16, 2024
2d7c895
move gating logic location
jouho Jul 17, 2024
8f309a3
allow sslv3 handshake for ossl-1.0.2-fips
jouho Jul 25, 2024
8297f40
fix cbmc harness
jouho Jul 25, 2024
f2c1129
Merge branch 'main' into sslv3-ossl-1-0-2-handshake-fix
jouho Jul 26, 2024
be3630d
test: allow md5 for fips
jouho Jul 26, 2024
bfb7f2c
enable md5 for hmac init
jouho Jul 26, 2024
2174758
fix hmac test
jouho Jul 26, 2024
0afd9d1
address PR feedback
jouho Jul 27, 2024
9b8647b
address PR feedback
jouho Jul 27, 2024
67a41fa
remove duplicate flag activation
jouho Jul 29, 2024
7b3584b
address PR feedback
jouho Jul 30, 2024
2ce2a34
address PR feedback
jouho Jul 30, 2024
93489a4
Merge branch 'main' into sslv3-ossl-1-0-2-handshake-fix
jouho Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions crypto/s2n_hmac.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,6 @@ bool s2n_hmac_is_available(s2n_hmac_algorithm hmac_alg)
case S2N_HMAC_MD5:
case S2N_HMAC_SSLv3_MD5:
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();
case S2N_HMAC_NONE:
case S2N_HMAC_SHA1:
case S2N_HMAC_SHA224:
Expand Down
1 change: 0 additions & 1 deletion tests/cbmc/proofs/s2n_hmac_is_available/s2n_hmac_is_available_harness.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ void s2n_hmac_is_available_harness()
case S2N_HASH_MD5:
case S2N_HMAC_SSLv3_MD5:
case S2N_HMAC_SSLv3_SHA1:
assert(is_available == !s2n_is_in_fips_mode() || s2n_libcrypto_is_awslc()); break;
case S2N_HASH_NONE:
case S2N_HASH_SHA1:
case S2N_HASH_SHA224:
Expand Down
8 changes: 0 additions & 8 deletions tests/unit/s2n_crypto_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,6 @@ int main()
for (size_t i = 0; i < s2n_array_len(supported_versions); i++) {
const uint8_t version = supported_versions[i];

/* See https://github.com/aws/s2n-tls/issues/4476
* Retrieving the master secret won't vary between FIPS and non-FIPS,
* so this testing limitation is not a concern.
*/
if (s2n_is_in_fips_mode() && version == S2N_SSLv3) {
continue;
}

DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, ecdsa_chain_and_key));
Expand Down
12 changes: 12 additions & 0 deletions tests/unit/s2n_hmac_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@
#include "testlib/s2n_testlib.h"
#include "utils/s2n_safety.h"

static S2N_RESULT s2n_allow_md5_for_fips(struct s2n_hmac_state *hmac)
{
if (s2n_is_in_fips_mode()) {
RESULT_GUARD_POSIX(s2n_hash_allow_md5_for_fips(&hmac->inner));
RESULT_GUARD_POSIX(s2n_hash_allow_md5_for_fips(&hmac->inner_just_key));
RESULT_GUARD_POSIX(s2n_hash_allow_md5_for_fips(&hmac->outer));
RESULT_GUARD_POSIX(s2n_hash_allow_md5_for_fips(&hmac->outer_just_key));
}
return S2N_RESULT_OK;
}

int main(int argc, char **argv)
{
uint8_t digest_pad[256];
Expand Down Expand Up @@ -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));
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@jouho jouho Jul 30, 2024 •
edited
Loading

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

Copy link
Contributor Author

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.

EXPECT_SUCCESS(s2n_hmac_init(&hmac, S2N_HMAC_SSLv3_MD5, sekrit, strlen((char *) sekrit)));
EXPECT_SUCCESS(s2n_hmac_update(&hmac, hello, strlen((char *) hello)));
EXPECT_SUCCESS(s2n_hmac_digest(&hmac, digest_pad, 16));
Expand Down
9 changes: 4 additions & 5 deletions tls/s2n_prf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,6 @@ static int s2n_sslv3_prf(struct s2n_connection *conn, struct s2n_blob *secret, s
POSIX_ENSURE_REF(conn->handshake.hashes);
struct s2n_hash_state *workspace = &conn->handshake.hashes->hash_workspace;

/* FIPS specifically allows MD5 for the legacy PRF */
if (s2n_is_in_fips_mode() && conn->actual_protocol_version < S2N_TLS12) {
POSIX_GUARD(s2n_hash_allow_md5_for_fips(workspace));
}

uint32_t outputlen = out->size;
uint8_t *output = out->data;
uint8_t iteration = 1;
Expand Down Expand Up @@ -157,6 +152,10 @@ static int s2n_sslv3_prf(struct s2n_connection *conn, struct s2n_blob *secret, s

struct s2n_hash_state *md5 = workspace;
POSIX_GUARD(s2n_hash_reset(md5));
/* FIPS specifically allows MD5 for the legacy PRF */
if (s2n_is_in_fips_mode() && conn->actual_protocol_version < S2N_TLS12) {
POSIX_GUARD(s2n_hash_allow_md5_for_fips(workspace));
}
POSIX_GUARD(s2n_hash_init(md5, S2N_HASH_MD5));
POSIX_GUARD(s2n_hash_update(md5, secret->data, secret->size));
POSIX_GUARD(s2n_hash_update(md5, sha_digest, sizeof(sha_digest)));
Expand Down
Loading