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

refactor: remove openssl-1.0.2-fips 'allow md5' logic #5048

Merged
merged 5 commits into from
Jan 23, 2025
Merged

refactor: remove openssl-1.0.2-fips 'allow md5' logic #5048

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9bfe1fa
refactor: remove openssl-1.0.2-fips 'allow md5' logic
lrstewart Jan 17, 2025
3e01156
cleanup / consolidate transcript hash comment
lrstewart Jan 22, 2025
c60e3fd
Add back non-fips md5 to ecdsa test
lrstewart Jan 22, 2025
01745fb
Merge branch 'main' into openssl3fips_hash_1
lrstewart Jan 22, 2025
20684fa
Merge branch 'main' into openssl3fips_hash_1
lrstewart Jan 23, 2025
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
39 changes: 3 additions & 36 deletions crypto/s2n_evp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,39 +15,6 @@

#include "crypto/s2n_evp.h"

#include "crypto/s2n_fips.h"
#include "error/s2n_errno.h"
#include "utils/s2n_safety.h"

int s2n_digest_allow_md5_for_fips(struct s2n_evp_digest *evp_digest)
{
POSIX_ENSURE_REF(evp_digest);
/* This is only to be used for EVP digests that will require MD5 to be used
* to comply with the TLS 1.0 and 1.1 RFC's for the PRF. MD5 cannot be used
* outside of the TLS 1.0 and 1.1 PRF when in FIPS mode.
*/
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;
Comment on lines -29 to -34
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

S2N_RESULT s2n_digest_is_md5_allowed_for_fips(struct s2n_evp_digest *evp_digest, bool *out)
{
RESULT_ENSURE_REF(out);
*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
Comment on lines -40 to -51
Copy link
Contributor Author

@lrstewart lrstewart Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

return S2N_RESULT_OK;
}
/*
* TODO: update all CBMC proofs that depend on this file, then delete.
*/
Comment on lines +18 to +20
Copy link
Contributor Author

@lrstewart lrstewart Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

3 changes: 0 additions & 3 deletions crypto/s2n_evp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,3 @@ struct s2n_evp_hmac_state {
*/
#define S2N_EVP_PKEY_CTX_set_signature_md(ctx, md) \
EVP_PKEY_CTX_set_signature_md(ctx, (EVP_MD *) (uintptr_t) md)

int s2n_digest_allow_md5_for_fips(struct s2n_evp_digest *evp_digest);
S2N_RESULT s2n_digest_is_md5_allowed_for_fips(struct s2n_evp_digest *evp_digest, bool *out);
56 changes: 1 addition & 55 deletions crypto/s2n_hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,6 @@ bool s2n_hash_is_available(s2n_hash_algorithm alg)
switch (alg) {
case S2N_HASH_MD5:
case S2N_HASH_MD5_SHA1:
/* return false if in FIPS mode, as MD5 algs are not available in FIPS mode. */
return !s2n_is_in_fips_mode();
Comment on lines -116 to -117
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jouho jouho Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/* The handshake MD5 hash state will fail the s2n_hash_is_available() check
* since MD5 is not permitted in FIPS mode. This check will not be used as
* the handshake MD5 hash state is specifically used by the TLS 1.0 and TLS 1.1
* PRF, which is required to comply with the TLS 1.0 and 1.1 RFCs and is approved
* as per NIST Special Publication 800-52 Revision 1.
*/

case S2N_HASH_NONE:
case S2N_HASH_SHA1:
case S2N_HASH_SHA224:
Expand Down Expand Up @@ -301,20 +299,6 @@ static int s2n_evp_hash_new(struct s2n_hash_state *state)
return S2N_SUCCESS;
}

static int s2n_evp_hash_allow_md5_for_fips(struct s2n_hash_state *state)
{
/* This is only to be used for s2n_hash_states that will require MD5 to be used
* to comply with the TLS 1.0 and 1.1 RFC's for the PRF. MD5 cannot be used
* outside of the TLS 1.0 and 1.1 PRF when in FIPS mode. When needed, this must
* be called prior to s2n_hash_init().
*/
POSIX_GUARD(s2n_digest_allow_md5_for_fips(&state->digest.high_level.evp));
if (s2n_use_custom_md5_sha1()) {
POSIX_GUARD(s2n_digest_allow_md5_for_fips(&state->digest.high_level.evp_md5_secondary));
}
return S2N_SUCCESS;
}

static int s2n_evp_hash_init(struct s2n_hash_state *state, s2n_hash_algorithm alg)
{
POSIX_ENSURE_REF(state->digest.high_level.evp.ctx);
Expand Down Expand Up @@ -419,32 +403,16 @@ static int s2n_evp_hash_copy(struct s2n_hash_state *to, struct s2n_hash_state *f
POSIX_GUARD_OSSL(EVP_MD_CTX_copy_ex(to->digest.high_level.evp_md5_secondary.ctx, from->digest.high_level.evp_md5_secondary.ctx), S2N_ERR_HASH_COPY_FAILED);
}

bool is_md5_allowed_for_fips = false;
POSIX_GUARD_RESULT(s2n_digest_is_md5_allowed_for_fips(&from->digest.high_level.evp, &is_md5_allowed_for_fips));
if (is_md5_allowed_for_fips && (from->alg == S2N_HASH_MD5 || from->alg == S2N_HASH_MD5_SHA1)) {
POSIX_GUARD(s2n_hash_allow_md5_for_fips(to));
}
return S2N_SUCCESS;
}

static int s2n_evp_hash_reset(struct s2n_hash_state *state)
{
int reset_md5_for_fips = 0;
bool is_md5_allowed_for_fips = false;
POSIX_GUARD_RESULT(s2n_digest_is_md5_allowed_for_fips(&state->digest.high_level.evp, &is_md5_allowed_for_fips));
if ((state->alg == S2N_HASH_MD5 || state->alg == S2N_HASH_MD5_SHA1) && is_md5_allowed_for_fips) {
reset_md5_for_fips = 1;
}

POSIX_GUARD_OSSL(S2N_EVP_MD_CTX_RESET(state->digest.high_level.evp.ctx), S2N_ERR_HASH_WIPE_FAILED);
if (state->alg == S2N_HASH_MD5_SHA1 && s2n_use_custom_md5_sha1()) {
POSIX_GUARD_OSSL(S2N_EVP_MD_CTX_RESET(state->digest.high_level.evp_md5_secondary.ctx), S2N_ERR_HASH_WIPE_FAILED);
}

if (reset_md5_for_fips) {
POSIX_GUARD(s2n_hash_allow_md5_for_fips(state));
}

/* hash_init resets the ready_for_input and currently_in_hash fields. */
return s2n_evp_hash_init(state, state->alg);
}
Expand All @@ -465,7 +433,6 @@ static int s2n_evp_hash_free(struct s2n_hash_state *state)

static const struct s2n_hash s2n_low_level_hash = {
.alloc = &s2n_low_level_hash_new,
.allow_md5_for_fips = NULL,
.init = &s2n_low_level_hash_init,
.update = &s2n_low_level_hash_update,
.digest = &s2n_low_level_hash_digest,
Expand All @@ -476,7 +443,6 @@ static const struct s2n_hash s2n_low_level_hash = {

static const struct s2n_hash s2n_evp_hash = {
.alloc = &s2n_evp_hash_new,
.allow_md5_for_fips = &s2n_evp_hash_allow_md5_for_fips,
.init = &s2n_evp_hash_init,
.update = &s2n_evp_hash_update,
.digest = &s2n_evp_hash_digest,
Expand Down Expand Up @@ -514,19 +480,6 @@ S2N_RESULT s2n_hash_state_validate(struct s2n_hash_state *state)
return S2N_RESULT_OK;
}

int s2n_hash_allow_md5_for_fips(struct s2n_hash_state *state)
{
POSIX_ENSURE_REF(state);
/* Ensure that hash_impl is set, as it may have been reset for s2n_hash_state on s2n_connection_wipe.
* When in FIPS mode, the EVP API's must be used for hashes.
*/
POSIX_GUARD(s2n_hash_set_impl(state));
goatgoose marked this conversation as resolved.
Show resolved Hide resolved

POSIX_ENSURE_REF(state->hash_impl->allow_md5_for_fips);

return state->hash_impl->allow_md5_for_fips(state);
}

int s2n_hash_init(struct s2n_hash_state *state, s2n_hash_algorithm alg)
{
POSIX_ENSURE_REF(state);
Expand All @@ -535,15 +488,8 @@ int s2n_hash_init(struct s2n_hash_state *state, s2n_hash_algorithm alg)
*/
POSIX_GUARD(s2n_hash_set_impl(state));

bool is_md5_allowed_for_fips = false;
POSIX_GUARD_RESULT(s2n_digest_is_md5_allowed_for_fips(&state->digest.high_level.evp, &is_md5_allowed_for_fips));

if (s2n_hash_is_available(alg) || ((alg == S2N_HASH_MD5 || alg == S2N_HASH_MD5_SHA1) && is_md5_allowed_for_fips)) {
/* s2n will continue to initialize an "unavailable" hash when s2n is in FIPS mode and
* FIPS is forcing the hash to be made available.
*/
if (s2n_hash_is_available(alg)) {
POSIX_ENSURE_REF(state->hash_impl->init);

return state->hash_impl->init(state, alg);
} else {
POSIX_BAIL(S2N_ERR_HASH_INVALID_ALGORITHM);
Expand Down
2 changes: 0 additions & 2 deletions crypto/s2n_hash.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ struct s2n_hash_state {
*/
struct s2n_hash {
int (*alloc)(struct s2n_hash_state *state);
int (*allow_md5_for_fips)(struct s2n_hash_state *state);
int (*init)(struct s2n_hash_state *state, s2n_hash_algorithm alg);
int (*update)(struct s2n_hash_state *state, const void *data, uint32_t size);
int (*digest)(struct s2n_hash_state *state, void *out, uint32_t size);
Expand All @@ -94,7 +93,6 @@ bool s2n_hash_is_available(s2n_hash_algorithm alg);
int s2n_hash_is_ready_for_input(struct s2n_hash_state *state);
int s2n_hash_new(struct s2n_hash_state *state);
S2N_RESULT s2n_hash_state_validate(struct s2n_hash_state *state);
int s2n_hash_allow_md5_for_fips(struct s2n_hash_state *state);
int s2n_hash_init(struct s2n_hash_state *state, s2n_hash_algorithm alg);
int s2n_hash_update(struct s2n_hash_state *state, const void *data, uint32_t size);
int s2n_hash_digest(struct s2n_hash_state *state, void *out, uint32_t size);
Expand Down
32 changes: 0 additions & 32 deletions tests/cbmc/proofs/s2n_digest_allow_md5_for_fips/Makefile
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion tests/cbmc/proofs/s2n_digest_allow_md5_for_fips/cbmc-proof.txt
View file
Open in desktop

This file was deleted.

40 changes: 0 additions & 40 deletions tests/cbmc/proofs/s2n_digest_allow_md5_for_fips/s2n_digest_allow_md5_for_fips_harness.c
View file
Open in desktop

This file was deleted.

35 changes: 0 additions & 35 deletions tests/cbmc/proofs/s2n_digest_allow_md5_for_fips_boringssl_awslc/Makefile
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion tests/cbmc/proofs/s2n_digest_allow_md5_for_fips_boringssl_awslc/cbmc-proof.txt
View file
Open in desktop

This file was deleted.

40 changes: 0 additions & 40 deletions ...llow_md5_for_fips_boringssl_awslc/s2n_digest_allow_md5_for_fips_boringssl_awslc_harness.c
View file
Open in desktop

This file was deleted.

32 changes: 0 additions & 32 deletions tests/cbmc/proofs/s2n_digest_is_md5_allowed_for_fips/Makefile
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion tests/cbmc/proofs/s2n_digest_is_md5_allowed_for_fips/cbmc-proof.txt
View file
Open in desktop

This file was deleted.

Loading
Loading