refactor: remove unused evp support for md5+sha1 (#5106)

Author: lrstewart

crypto/s2n_evp_signing.c

@@ -16,6 +16,7 @@
 #include "crypto/s2n_evp_signing.h"
 
 #include "crypto/s2n_evp.h"
+#include "crypto/s2n_fips.h"
 #include "crypto/s2n_pkey.h"
 #include "crypto/s2n_rsa_pss.h"
 #include "error/s2n_errno.h"
@@ -50,18 +51,58 @@ static S2N_RESULT s2n_evp_pkey_set_rsa_pss_saltlen(EVP_PKEY_CTX *pctx)
 #endif
 }
 
-bool s2n_evp_signing_supported()
+static bool s2n_evp_md5_sha1_is_supported()
+{
+#if defined(S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH)
+    return true;
+#else
+    return false;
+#endif
+}
+
+static bool s2n_evp_md_ctx_set_pkey_ctx_is_supported()
 {
 #ifdef S2N_LIBCRYPTO_SUPPORTS_EVP_MD_CTX_SET_PKEY_CTX
-    /* We can only use EVP signing if the hash state has an EVP_MD_CTX
-     * that we can pass to the EVP signing methods.
-     */
-    return s2n_hash_evp_fully_supported();
+    return true;
 #else
     return false;
 #endif
 }
 
+bool s2n_evp_signing_supported()
+{
+    /* We must use the FIPS-approved EVP APIs in FIPS mode,
+     * but we could also use the EVP APIs outside of FIPS mode.
+     * Only using the EVP APIs in FIPS mode was a choice made to reduce
+     * the impact of adding support for the EVP APIs.
+     * We should consider instead making the EVP APIs the default.
+     */
+    if (!s2n_is_in_fips_mode()) {
+        return false;
+    }
+
+    /* Our EVP signing logic is intended to support FIPS 140-3.
+     * FIPS 140-3 does not allow externally calculated digests (except for
+     * signing, but not verifying, with ECDSA).
+     * See https://csrc.nist.gov/Projects/Cryptographic-Algorithm-Validation-Program/Digital-Signatures,
+     * and note that "component" tests only exist for ECDSA sign.
+     *
+     * We currently work around that restriction by calling EVP_MD_CTX_set_pkey_ctx,
+     * which lets us set a key on an existing hash state. This is important
+     * when we need to handle signing the TLS1.2 client cert verify message,
+     * which requires signing the entire message transcript. If EVP_MD_CTX_set_pkey_ctx
+     * is unavailable (true for openssl-1.0.2), our current EVP logic will not work.
+     *
+     * FIPS 140-3 is also not possible if EVP_md5_sha1() isn't available
+     * (again true for openssl-1.0.2). In that case, we use two separate hash
+     * states to track the md5 and sha1 parts of the hash separately. That means
+     * that we also have to calculate the digests separately, then combine the
+     * result. We therefore only have an externally calculated digest available
+     * for signing or verifying.
+     */
+    return s2n_evp_md_ctx_set_pkey_ctx_is_supported() && s2n_evp_md5_sha1_is_supported();
+}
+
 /* If using EVP signing, override the sign and verify pkey methods.
  * The EVP methods can handle all pkey types / signature algorithms.
  */

crypto/s2n_hash.c

@@ -15,29 +15,22 @@
 
 #include "crypto/s2n_hash.h"
 
-#include "crypto/s2n_fips.h"
+#include "crypto/s2n_evp_signing.h"
 #include "crypto/s2n_hmac.h"
 #include "crypto/s2n_openssl.h"
 #include "error/s2n_errno.h"
 #include "utils/s2n_safety.h"
 
-static bool s2n_use_custom_md5_sha1()
-{
-#if defined(S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH)
-    return false;
-#else
-    return true;
-#endif
-}
-
 static bool s2n_use_evp_impl()
 {
-    return s2n_is_in_fips_mode();
-}
-
-bool s2n_hash_evp_fully_supported()
-{
-    return s2n_use_evp_impl() && !s2n_use_custom_md5_sha1();
+    /* Our current EVP signing implementation requires EVP hashing.
+     *
+     * We could use the EVP hashing impl with legacy signing, but that would
+     * unnecessarily complicate the logic. The only known libcrypto that
+     * doesn't support EVP signing is openssl-1.0.2. Just let legacy
+     * libcryptos use legacy methods.
+     */
+    return s2n_evp_signing_supported();
 }
 
 const EVP_MD *s2n_hash_alg_to_evp_md(s2n_hash_algorithm alg)
@@ -289,13 +282,8 @@ static int s2n_low_level_hash_free(struct s2n_hash_state *state)
 static int s2n_evp_hash_new(struct s2n_hash_state *state)
 {
     POSIX_ENSURE_REF(state->digest.high_level.evp.ctx = S2N_EVP_MD_CTX_NEW());
-    if (s2n_use_custom_md5_sha1()) {
-        POSIX_ENSURE_REF(state->digest.high_level.evp_md5_secondary.ctx = S2N_EVP_MD_CTX_NEW());
-    }
-
     state->is_ready_for_input = 0;
     state->currently_in_hash = 0;
-
     return S2N_SUCCESS;
 }
 
@@ -311,13 +299,6 @@ static int s2n_evp_hash_init(struct s2n_hash_state *state, s2n_hash_algorithm al
         return S2N_SUCCESS;
     }
 
-    if (alg == S2N_HASH_MD5_SHA1 && s2n_use_custom_md5_sha1()) {
-        POSIX_ENSURE_REF(state->digest.high_level.evp_md5_secondary.ctx);
-        POSIX_GUARD_OSSL(EVP_DigestInit_ex(state->digest.high_level.evp.ctx, EVP_sha1(), NULL), S2N_ERR_HASH_INIT_FAILED);
-        POSIX_GUARD_OSSL(EVP_DigestInit_ex(state->digest.high_level.evp_md5_secondary.ctx, EVP_md5(), NULL), S2N_ERR_HASH_INIT_FAILED);
-        return S2N_SUCCESS;
-    }
-
     POSIX_ENSURE_REF(s2n_hash_alg_to_evp_md(alg));
     POSIX_GUARD_OSSL(EVP_DigestInit_ex(state->digest.high_level.evp.ctx, s2n_hash_alg_to_evp_md(alg), NULL), S2N_ERR_HASH_INIT_FAILED);
     return S2N_SUCCESS;
@@ -335,12 +316,6 @@ static int s2n_evp_hash_update(struct s2n_hash_state *state, const void *data, u
 
     POSIX_ENSURE_REF(EVP_MD_CTX_md(state->digest.high_level.evp.ctx));
     POSIX_GUARD_OSSL(EVP_DigestUpdate(state->digest.high_level.evp.ctx, data, size), S2N_ERR_HASH_UPDATE_FAILED);
-
-    if (state->alg == S2N_HASH_MD5_SHA1 && s2n_use_custom_md5_sha1()) {
-        POSIX_ENSURE_REF(EVP_MD_CTX_md(state->digest.high_level.evp_md5_secondary.ctx));
-        POSIX_GUARD_OSSL(EVP_DigestUpdate(state->digest.high_level.evp_md5_secondary.ctx, data, size), S2N_ERR_HASH_UPDATE_FAILED);
-    }
-
     return S2N_SUCCESS;
 }
 
@@ -362,23 +337,6 @@ static int s2n_evp_hash_digest(struct s2n_hash_state *state, void *out, uint32_t
 
     POSIX_ENSURE_REF(EVP_MD_CTX_md(state->digest.high_level.evp.ctx));
 
-    if (state->alg == S2N_HASH_MD5_SHA1 && s2n_use_custom_md5_sha1()) {
-        POSIX_ENSURE_REF(EVP_MD_CTX_md(state->digest.high_level.evp_md5_secondary.ctx));
-
-        uint8_t sha1_digest_size = 0;
-        POSIX_GUARD(s2n_hash_digest_size(S2N_HASH_SHA1, &sha1_digest_size));
-
-        unsigned int sha1_primary_digest_size = sha1_digest_size;
-        unsigned int md5_secondary_digest_size = digest_size - sha1_primary_digest_size;
-
-        POSIX_ENSURE(EVP_MD_CTX_size(state->digest.high_level.evp.ctx) <= sha1_digest_size, S2N_ERR_HASH_DIGEST_FAILED);
-        POSIX_ENSURE((size_t) EVP_MD_CTX_size(state->digest.high_level.evp_md5_secondary.ctx) <= md5_secondary_digest_size, S2N_ERR_HASH_DIGEST_FAILED);
-
-        POSIX_GUARD_OSSL(EVP_DigestFinal_ex(state->digest.high_level.evp.ctx, ((uint8_t *) out) + MD5_DIGEST_LENGTH, &sha1_primary_digest_size), S2N_ERR_HASH_DIGEST_FAILED);
-        POSIX_GUARD_OSSL(EVP_DigestFinal_ex(state->digest.high_level.evp_md5_secondary.ctx, out, &md5_secondary_digest_size), S2N_ERR_HASH_DIGEST_FAILED);
-        return S2N_SUCCESS;
-    }
-
     POSIX_ENSURE((size_t) EVP_MD_CTX_size(state->digest.high_level.evp.ctx) <= digest_size, S2N_ERR_HASH_DIGEST_FAILED);
     POSIX_GUARD_OSSL(EVP_DigestFinal_ex(state->digest.high_level.evp.ctx, out, &digest_size), S2N_ERR_HASH_DIGEST_FAILED);
     return S2N_SUCCESS;
@@ -397,21 +355,12 @@ static int s2n_evp_hash_copy(struct s2n_hash_state *to, struct s2n_hash_state *f
 
     POSIX_ENSURE_REF(to->digest.high_level.evp.ctx);
     POSIX_GUARD_OSSL(EVP_MD_CTX_copy_ex(to->digest.high_level.evp.ctx, from->digest.high_level.evp.ctx), S2N_ERR_HASH_COPY_FAILED);
-
-    if (from->alg == S2N_HASH_MD5_SHA1 && s2n_use_custom_md5_sha1()) {
-        POSIX_ENSURE_REF(to->digest.high_level.evp_md5_secondary.ctx);
-        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);
-    }
-
     return S2N_SUCCESS;
 }
 
 static int s2n_evp_hash_reset(struct s2n_hash_state *state)
 {
     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);
-    }
 
     /* hash_init resets the ready_for_input and currently_in_hash fields. */
     return s2n_evp_hash_init(state, state->alg);
@@ -421,12 +370,6 @@ static int s2n_evp_hash_free(struct s2n_hash_state *state)
 {
     S2N_EVP_MD_CTX_FREE(state->digest.high_level.evp.ctx);
     state->digest.high_level.evp.ctx = NULL;
-
-    if (s2n_use_custom_md5_sha1()) {
-        S2N_EVP_MD_CTX_FREE(state->digest.high_level.evp_md5_secondary.ctx);
-        state->digest.high_level.evp_md5_secondary.ctx = NULL;
-    }
-
     state->is_ready_for_input = 0;
     return S2N_SUCCESS;
 }

crypto/s2n_hash.h

@@ -54,8 +54,6 @@ union s2n_hash_low_level_digest {
 /* The evp_digest stores all OpenSSL structs to be used with OpenSSL's EVP hash API's. */
 struct s2n_hash_evp_digest {
     struct s2n_evp_digest evp;
-    /* Always store a secondary evp_digest to allow resetting a hash_state to MD5_SHA1 from another alg. */
-    struct s2n_evp_digest evp_md5_secondary;
 };
 
 /* s2n_hash_state stores the s2n_hash implementation being used (low-level or EVP),

tests/cbmc/include/cbmc_proof/cbmc_utils.h

@@ -47,13 +47,8 @@ struct rc_keys_from_evp_pkey_ctx {
     int pkey_eckey_refs;
 };
 
-/**
- * In the `rc_keys_from_hash_state`, we store two `rc_keys_from_evp_pkey_ctx` objects:
- * one for the `evp` field and another for `evp_md5_secondary` field.
- */
 struct rc_keys_from_hash_state {
     struct rc_keys_from_evp_pkey_ctx evp;
-    struct rc_keys_from_evp_pkey_ctx evp_md5;
 };
 
 /**

tests/cbmc/proofs/s2n_hash_const_time_get_currently_in_hash_block/Makefile

@@ -22,7 +22,7 @@ HARNESS_FILE = $(HARNESS_ENTRY).c
 PROOF_SOURCES += $(PROOFDIR)/$(HARNESS_FILE)
 PROOF_SOURCES += $(PROOF_SOURCE)/make_common_datastructures.c
 PROOF_SOURCES += $(PROOF_STUB)/s2n_calculate_stacktrace.c
-PROOF_SOURCES += $(PROOF_STUB)/s2n_is_in_fips_mode.c
+PROOF_SOURCES += $(PROOF_STUB)/s2n_evp_signing_supported.c
 
 PROJECT_SOURCES += $(SRCDIR)/crypto/s2n_hash.c
 

tests/cbmc/proofs/s2n_hash_copy/Makefile

@@ -25,7 +25,7 @@ PROOF_SOURCES += $(OPENSSL_SOURCE)/ec_override.c
 PROOF_SOURCES += $(OPENSSL_SOURCE)/evp_override.c
 PROOF_SOURCES += $(PROOF_SOURCE)/make_common_datastructures.c
 PROOF_SOURCES += $(PROOF_STUB)/s2n_calculate_stacktrace.c
-PROOF_SOURCES += $(PROOF_STUB)/s2n_is_in_fips_mode.c
+PROOF_SOURCES += $(PROOF_STUB)/s2n_evp_signing_supported.c
 
 PROJECT_SOURCES += $(SRCDIR)/crypto/s2n_hash.c
 PROJECT_SOURCES += $(SRCDIR)/utils/s2n_ensure.c

tests/cbmc/proofs/s2n_hash_digest/Makefile

@@ -27,7 +27,7 @@ PROOF_SOURCES += $(OPENSSL_SOURCE)/md5_override.c
 PROOF_SOURCES += $(OPENSSL_SOURCE)/sha_override.c
 PROOF_SOURCES += $(PROOF_SOURCE)/make_common_datastructures.c
 PROOF_SOURCES += $(PROOF_STUB)/s2n_calculate_stacktrace.c
-PROOF_SOURCES += $(PROOF_STUB)/s2n_is_in_fips_mode.c
+PROOF_SOURCES += $(PROOF_STUB)/s2n_evp_signing_supported.c
 
 PROJECT_SOURCES += $(SRCDIR)/crypto/s2n_hash.c
 PROJECT_SOURCES += $(SRCDIR)/utils/s2n_safety.c

tests/cbmc/proofs/s2n_hash_free/Makefile

@@ -24,7 +24,7 @@ PROOF_SOURCES += $(OPENSSL_SOURCE)/ec_override.c
 PROOF_SOURCES += $(OPENSSL_SOURCE)/evp_override.c
 PROOF_SOURCES += $(PROOF_SOURCE)/cbmc_utils.c
 PROOF_SOURCES += $(PROOF_SOURCE)/make_common_datastructures.c
-PROOF_SOURCES += $(PROOF_STUB)/s2n_is_in_fips_mode.c
+PROOF_SOURCES += $(PROOF_STUB)/s2n_evp_signing_supported.c
 PROOF_SOURCES += $(PROOFDIR)/$(HARNESS_FILE)
 
 PROJECT_SOURCES += $(SRCDIR)/crypto/s2n_hash.c

tests/cbmc/proofs/s2n_hash_free/s2n_hash_free_harness.c

@@ -13,7 +13,7 @@
  * permissions and limitations under the License.
  */
 
-#include "crypto/s2n_fips.h"
+#include "crypto/s2n_evp_signing.h"
 #include "crypto/s2n_hash.h"
 
 #include <cbmc_proof/make_common_datastructures.h>
@@ -35,9 +35,8 @@ void s2n_hash_free_harness()
     assert(s2n_hash_free(state) == S2N_SUCCESS);
     if (state != NULL) {
         assert(state->hash_impl->free != NULL);
-        if (s2n_is_in_fips_mode()) {
+        if (s2n_evp_signing_supported()) {
             assert(state->digest.high_level.evp.ctx == NULL);
-            assert(state->digest.high_level.evp_md5_secondary.ctx == NULL);
             assert_rc_decrement_on_hash_state(&saved_hash_state);
         } else {
             assert_rc_unchanged_on_hash_state(&saved_hash_state);
@@ -47,11 +46,10 @@ void s2n_hash_free_harness()
 
     /* Cleanup after expected error cases, for memory leak check. */
     if (state != NULL) {
-        /* 1. `free` leftover EVP_MD_CTX objects if `s2n_is_in_fips_mode`,
+        /* 1. `free` leftover EVP_MD_CTX objects if `s2n_evp_signing_supported`,
               since `s2n_hash_free` is a NO-OP in that case. */
-        if (!s2n_is_in_fips_mode()) {
+        if (!s2n_evp_signing_supported()) {
             S2N_EVP_MD_CTX_FREE(state->digest.high_level.evp.ctx);
-            S2N_EVP_MD_CTX_FREE(state->digest.high_level.evp_md5_secondary.ctx);
         }
 
         /* 2. `free` leftover reference-counted keys (i.e. those with non-zero ref-count),

tests/cbmc/proofs/s2n_hash_get_currently_in_hash_total/Makefile

@@ -22,7 +22,7 @@ HARNESS_FILE = $(HARNESS_ENTRY).c
 PROOF_SOURCES += $(PROOFDIR)/$(HARNESS_FILE)
 PROOF_SOURCES += $(PROOF_SOURCE)/make_common_datastructures.c
 PROOF_SOURCES += $(PROOF_STUB)/s2n_calculate_stacktrace.c
-PROOF_SOURCES += $(PROOF_STUB)/s2n_is_in_fips_mode.c
+PROOF_SOURCES += $(PROOF_STUB)/s2n_evp_signing_supported.c
 
 PROJECT_SOURCES += $(SRCDIR)/crypto/s2n_hash.c