pr feedback

aws · May 26, 2023 · a6876b0 · a6876b0
1 parent 446f817
commit a6876b0
Show file tree

Hide file tree

Showing 6 changed files with 90 additions and 30 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -476,6 +476,16 @@ if(PLATFORM_SUPPORTS_KTLS)
     target_compile_options(${PROJECT_NAME} PUBLIC -DS2N_PLATFORM_SUPPORTS_KTLS)
 endif()
 
+try_compile(
+        LIBCRYPTO_SUPPORTS_TLS_PRF
+        ${CMAKE_BINARY_DIR}
+        SOURCES "${CMAKE_CURRENT_LIST_DIR}/tests/features/tls_prf.c"
+        LINK_LIBRARIES ${LINK_LIB} ${OS_LIBS}
+)
+if(LIBCRYPTO_SUPPORTS_TLS_PRF)
+    target_compile_options(${PROJECT_NAME} PUBLIC -DS2N_LIBCRYPTO_SUPPORTS_TLS_PRF)
+endif()
+
 if (NOT DEFINED CMAKE_AR)
 	message(STATUS "CMAKE_AR undefined, setting to `ar` by default")
 	SET(CMAKE_AR ar)

diff --git a/s2n.mk b/s2n.mk
@@ -261,6 +261,12 @@ ifeq ($(TRY_COMPILE_KTLS), 0)
 	DEFAULT_CFLAGS += -DS2N_PLATFORM_SUPPORTS_KTLS
 endif
 
+# Determine if libcrypto PRF implementation is available
+TRY_COMPILE_TLS_PRF := $(call try_compile,$(S2N_ROOT)/tests/features/tls_prf.c)
+ifeq ($(TRY_COMPILE_TLS_PRF), 0)
+	DEFAULT_CFLAGS += -DS2N_LIBCRYPTO_SUPPORTS_TLS_PRF
+endif
+
 CFLAGS_LLVM = ${DEFAULT_CFLAGS} -emit-llvm -c -g -O1
 
 $(BITCODE_DIR)%.bc: %.c

diff --git a/tests/features/tls_prf.c b/tests/features/tls_prf.c
@@ -0,0 +1,33 @@
+
+#include <openssl/base.h>
+#include <openssl/digest.h>
+
+#define TEST_BUFFER_SIZE 64
+
+/* BoringSSL and AWSLC define the CRYPTO_tls1_prf API in a private header. This function is
+ * forward-declared to make it accessible.
+ */
+int CRYPTO_tls1_prf(const EVP_MD *digest,
+        uint8_t *out, size_t out_len,
+        const uint8_t *secret, size_t secret_len,
+        const char *label, size_t label_len,
+        const uint8_t *seed1, size_t seed1_len,
+        const uint8_t *seed2, size_t seed2_len);
+
+int main()
+{
+//    const EVP_MD *digest = EVP_md5_sha1();
+//
+//    uint8_t out[TEST_BUFFER_SIZE] = { 0 };
+//    uint8_t secret[TEST_BUFFER_SIZE] = { 0 };
+//    const char label[TEST_BUFFER_SIZE] = { 0 };
+//    uint8_t seed1[TEST_BUFFER_SIZE] = { 0 };
+//    uint8_t seed2[TEST_BUFFER_SIZE] = { 0 };
+//
+//    if (!CRYPTO_tls1_prf(digest, out, TEST_BUFFER_SIZE, secret, TEST_BUFFER_SIZE, label,
+//                TEST_BUFFER_SIZE, seed1, TEST_BUFFER_SIZE, seed2, TEST_BUFFER_SIZE)) {
+//        return 1;
+//    }
+
+    return 0;
+}
diff --git a/tests/unit/s2n_tls_prf_test.c b/tests/unit/s2n_tls_prf_test.c
@@ -325,19 +325,25 @@ int main(int argc, char **argv)
         {
             DEFER_CLEANUP(struct s2n_connection *connection = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free);
 
-            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(NULL, &secret, &label, &seed_a, &seed_b, &seed_c, &out), S2N_ERR_NULL);
-            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(connection, NULL, &label, &seed_a, &seed_b, &seed_c, &out), S2N_ERR_NULL);
-            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(connection, &secret, NULL, &seed_a, &seed_b, &seed_c, &out), S2N_ERR_NULL);
-            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(connection, &secret, &label, &seed_a, &seed_b, &seed_c, NULL), S2N_ERR_NULL);
+            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(NULL, &secret, &label, &seed_a, &seed_b, &seed_c, &out),
+                    S2N_ERR_NULL);
+            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(connection, NULL, &label, &seed_a, &seed_b, &seed_c, &out),
+                    S2N_ERR_NULL);
+            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(connection, &secret, NULL, &seed_a, &seed_b, &seed_c, &out),
+                    S2N_ERR_NULL);
+            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(connection, &secret, &label, &seed_a, &seed_b, &seed_c, NULL),
+                    S2N_ERR_NULL);
 
             /* seed_a is required */
-            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(connection, &secret, &label, NULL, &seed_b, &seed_c, &out), S2N_ERR_PRF_INVALID_SEED);
+            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(connection, &secret, &label, NULL, &seed_b, &seed_c, &out),
+                    S2N_ERR_PRF_INVALID_SEED);
 
             /* seed_b and seed_c are optional */
             EXPECT_SUCCESS(s2n_prf(connection, &secret, &label, &seed_a, NULL, NULL, &out));
 
             /* seed_b is required if seed_c is provided */
-            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(connection, &secret, &label, &seed_a, NULL, &seed_c, &out), S2N_ERR_PRF_INVALID_SEED);
+            EXPECT_FAILURE_WITH_ERRNO(s2n_prf(connection, &secret, &label, &seed_a, NULL, &seed_c, &out),
+                    S2N_ERR_PRF_INVALID_SEED);
 
             /* seed_c is optional */
             EXPECT_SUCCESS(s2n_prf(connection, &secret, &label, &seed_a, &seed_b, NULL, &out));

diff --git a/tls/s2n_prf.c b/tls/s2n_prf.c
@@ -463,11 +463,15 @@ S2N_RESULT s2n_prf_free(struct s2n_connection *conn)
 
 static bool s2n_libcrypto_supports_tls_prf()
 {
-    return S2N_LIBCRYPTO_SUPPORTS_TLS_PRF;
+#ifdef S2N_LIBCRYPTO_SUPPORTS_TLS_PRF
+    return true;
+#else
+    return false;
+#endif
 }
 
-static S2N_RESULT s2n_custom_prf(struct s2n_connection *conn, struct s2n_blob *secret, struct s2n_blob *label, struct s2n_blob *seed_a,
-        struct s2n_blob *seed_b, struct s2n_blob *seed_c, struct s2n_blob *out)
+static S2N_RESULT s2n_custom_prf(struct s2n_connection *conn, struct s2n_blob *secret, struct s2n_blob *label,
+        struct s2n_blob *seed_a, struct s2n_blob *seed_b, struct s2n_blob *seed_c, struct s2n_blob *out)
 {
     /* We zero the out blob because p_hash works by XOR'ing with the existing
      * buffer. This is a little convoluted but means we can avoid dynamic memory
@@ -478,7 +482,8 @@ static S2N_RESULT s2n_custom_prf(struct s2n_connection *conn, struct s2n_blob *s
     RESULT_GUARD_POSIX(s2n_blob_zero(out));
 
     if (conn->actual_protocol_version == S2N_TLS12) {
-        RESULT_GUARD_POSIX(s2n_p_hash(conn->prf_space, conn->secure->cipher_suite->prf_alg, secret, label, seed_a, seed_b, seed_c, out));
+        RESULT_GUARD_POSIX(s2n_p_hash(conn->prf_space, conn->secure->cipher_suite->prf_alg, secret, label, seed_a,
+                seed_b, seed_c, out));
         return S2N_RESULT_OK;
     }
 
@@ -492,7 +497,7 @@ static S2N_RESULT s2n_custom_prf(struct s2n_connection *conn, struct s2n_blob *s
     return S2N_RESULT_OK;
 }
 
-#if S2N_LIBCRYPTO_SUPPORTS_TLS_PRF
+#ifdef S2N_LIBCRYPTO_SUPPORTS_TLS_PRF
 /* BoringSSL and AWSLC define the CRYPTO_tls1_prf API in a private header. This function is
  * forward-declared to make it accessible.
  */
@@ -503,17 +508,19 @@ int CRYPTO_tls1_prf(const EVP_MD *digest,
         const uint8_t *seed1, size_t seed1_len,
         const uint8_t *seed2, size_t seed2_len);
 
-static S2N_RESULT s2n_libcrypto_prf(struct s2n_connection *conn, struct s2n_blob *secret, struct s2n_blob *label, struct s2n_blob *seed_a,
-        struct s2n_blob *seed_b, struct s2n_blob *seed_c, struct s2n_blob *out)
+static S2N_RESULT s2n_libcrypto_prf(struct s2n_connection *conn, struct s2n_blob *secret, struct s2n_blob *label,
+        struct s2n_blob *seed_a, struct s2n_blob *seed_b, struct s2n_blob *seed_c, struct s2n_blob *out)
 {
-    /* md5_sha1 is a digest that indicates both MD5 and SHA1 should be used in the PRF calculation.
-     * This is needed for pre-TLS12 PRFs.
-     */
-    const EVP_MD *digest = EVP_md5_sha1();
-
-    if (conn->actual_protocol_version == S2N_TLS12) {
+    const EVP_MD *digest = NULL;
+    if (conn->actual_protocol_version < S2N_TLS12) {
+        /* md5_sha1 is a digest that indicates both MD5 and SHA1 should be used in the PRF calculation.
+         * This is needed for pre-TLS12 PRFs.
+         */
+        digest = EVP_md5_sha1();
+    } else {
         RESULT_GUARD(s2n_md_from_hmac_alg(conn->secure->cipher_suite->prf_alg, &digest));
     }
+    RESULT_ENSURE_REF(digest);
 
     DEFER_CLEANUP(struct s2n_stuffer seed_b_stuffer = { 0 }, s2n_stuffer_free);
     size_t seed_b_len = 0;
@@ -528,7 +535,7 @@ static S2N_RESULT s2n_libcrypto_prf(struct s2n_connection *conn, struct s2n_blob
             /* The AWSLC/BoringSSL TLS PRF implementation only provides two seed arguments. If three seeds
              * were provided, pass in the third seed by concatenating it with the second seed.
              */
-            RESULT_GUARD_POSIX(s2n_stuffer_growable_alloc(&seed_b_stuffer, seed_b->size + seed_c->size));
+            RESULT_GUARD_POSIX(s2n_stuffer_alloc(&seed_b_stuffer, seed_b->size + seed_c->size));
             RESULT_GUARD_POSIX(s2n_stuffer_write_bytes(&seed_b_stuffer, seed_b->data, seed_b->size));
             RESULT_GUARD_POSIX(s2n_stuffer_write_bytes(&seed_b_stuffer, seed_c->data, seed_c->size));
         }
@@ -538,15 +545,19 @@ static S2N_RESULT s2n_libcrypto_prf(struct s2n_connection *conn, struct s2n_blob
         RESULT_ENSURE_REF(seed_b_data);
     }
 
-    RESULT_GUARD_OSSL(CRYPTO_tls1_prf(digest, out->data, out->size, secret->data, secret->size, (const char *) label->data,
-                              label->size, seed_a->data, seed_a->size, seed_b_data, seed_b_len),
+    RESULT_GUARD_OSSL(CRYPTO_tls1_prf(digest,
+                              out->data, out->size,
+                              secret->data, secret->size,
+                              (const char *) label->data, label->size,
+                              seed_a->data, seed_a->size,
+                              seed_b_data, seed_b_len),
             S2N_ERR_PRF_DERIVE);
 
     return S2N_RESULT_OK;
 }
 #else
-static S2N_RESULT s2n_libcrypto_prf(struct s2n_connection *conn, struct s2n_blob *secret, struct s2n_blob *label, struct s2n_blob *seed_a,
-        struct s2n_blob *seed_b, struct s2n_blob *seed_c, struct s2n_blob *out)
+static S2N_RESULT s2n_libcrypto_prf(struct s2n_connection *conn, struct s2n_blob *secret, struct s2n_blob *label,
+        struct s2n_blob *seed_a, struct s2n_blob *seed_b, struct s2n_blob *seed_c, struct s2n_blob *out)
 {
     RESULT_BAIL(S2N_ERR_UNIMPLEMENTED);
 }

diff --git a/tls/s2n_prf.h b/tls/s2n_prf.h
@@ -25,12 +25,6 @@
 /* Enough to support TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384, 2*SHA384_DIGEST_LEN + 2*AES256_KEY_SIZE */
 #define S2N_MAX_KEY_BLOCK_LEN 160
 
-#if defined(OPENSSL_IS_BORINGSSL) || defined(OPENSSL_IS_AWSLC)
-    #define S2N_LIBCRYPTO_SUPPORTS_TLS_PRF 1
-#else
-    #define S2N_LIBCRYPTO_SUPPORTS_TLS_PRF 0
-#endif
-
 union p_hash_state {
     struct s2n_hmac_state s2n_hmac;
     struct s2n_evp_hmac_state evp_hmac;