Address Feedback

crypto/s2n_libcrypto.h

@@ -17,6 +17,8 @@
 
 #include "utils/s2n_result.h"
 
+uint64_t s2n_libcrypto_awslc_api_version(void);
+bool s2n_libcrypto_is_awslc();
 S2N_RESULT s2n_libcrypto_validate_runtime(void);
 const char *s2n_libcrypto_get_version_name(void);
 bool s2n_libcrypto_supports_flag_no_check_time();

tests/features/S2N_LIBCRYPTO_SUPPORTS_MLKEM.c

@@ -0,0 +1,31 @@
+/*
+* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+*
+* Licensed under the Apache License, Version 2.0 (the "License").
+* You may not use this file except in compliance with the License.
+* A copy of the License is located at
+*
+*  http://aws.amazon.com/apache2.0
+*
+* or in the "license" file accompanying this file. This file is distributed
+* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+* express or implied. See the License for the specific language governing
+* permissions and limitations under the License.
+*/
+
+#include <openssl/evp.h>
+#include <openssl/nid.h>
+
+int main()
+{
+    EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_KEM, NULL);
+    if (ctx == NULL) {
+        return 1;
+    }
+    if (!EVP_PKEY_CTX_kem_set_params(ctx, NID_MLKEM768)) {
+        EVP_PKEY_CTX_free(ctx);
+        return 1;
+    }
+    EVP_PKEY_CTX_free(ctx);
+    return 0;
+}

tests/unit/s2n_mlkem_test.c

@@ -0,0 +1,33 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+#include "api/s2n.h"
+#include "crypto/s2n_libcrypto.h"
+#include "crypto/s2n_pq.h"
+#include "s2n_test.h"
+#include "testlib/s2n_testlib.h"
+
+int main()
+{
+    BEGIN_TEST();
+    /* MLKEM Support was added to AWSLC when AWSLC_API_VERSION == 29 */
+    if (s2n_libcrypto_is_awslc() && s2n_libcrypto_awslc_api_version() >= 30) {
+        EXPECT_TRUE(s2n_libcrypto_supports_mlkem());
+    } else if (s2n_libcrypto_is_awslc() && s2n_libcrypto_awslc_api_version() < 29) {
+        EXPECT_FALSE(s2n_libcrypto_supports_mlkem());
+    }
+
+    END_TEST();
+}

tests/unit/s2n_pq_kem_test.c

@@ -48,8 +48,6 @@ int main()
 
     for (size_t i = 0; i < s2n_array_len(test_vectors); i++) {
         const struct s2n_kem *kem = test_vectors[i];
-        bool expect_success = s2n_libcrypto_supports_evp_kem()
-                && ((kem != &s2n_mlkem_768) || s2n_libcrypto_supports_mlkem());
 
         DEFER_CLEANUP(struct s2n_blob public_key = { 0 }, s2n_free);
         EXPECT_SUCCESS(s2n_alloc(&public_key, kem->public_key_length));
@@ -66,25 +64,26 @@ int main()
         DEFER_CLEANUP(struct s2n_blob ciphertext = { 0 }, s2n_free);
         EXPECT_SUCCESS(s2n_alloc(&ciphertext, kem->ciphertext_length));
 
-        if (expect_success) {
-            /* Test a successful round-trip: keygen->enc->dec */
-            EXPECT_PQ_KEM_SUCCESS(kem->generate_keypair(kem, public_key.data, private_key.data));
-            EXPECT_PQ_KEM_SUCCESS(kem->encapsulate(kem, ciphertext.data, client_shared_secret.data, public_key.data));
-            EXPECT_PQ_KEM_SUCCESS(kem->decapsulate(kem, server_shared_secret.data, ciphertext.data, private_key.data));
-            EXPECT_BYTEARRAY_EQUAL(server_shared_secret.data, client_shared_secret.data, kem->shared_secret_key_length);
-
-            /* By design, if an invalid private key + ciphertext pair is provided to decapsulate(),
-             * the function should still succeed (return S2N_SUCCESS); however, the shared secret
-             * that was "decapsulated" will be a garbage random value. */
-            ciphertext.data[0] ^= 1; /* Flip a bit to invalidate the ciphertext */
-
-            EXPECT_PQ_KEM_SUCCESS(kem->decapsulate(kem, server_shared_secret.data, ciphertext.data, private_key.data));
-            EXPECT_BYTEARRAY_NOT_EQUAL(server_shared_secret.data, client_shared_secret.data, kem->shared_secret_key_length);
-        } else {
+        if (!s2m_kem_is_available(kem)) {
             EXPECT_FAILURE_WITH_ERRNO(kem->generate_keypair(kem, public_key.data, private_key.data), S2N_ERR_UNIMPLEMENTED);
             EXPECT_FAILURE_WITH_ERRNO(kem->encapsulate(kem, ciphertext.data, client_shared_secret.data, public_key.data), S2N_ERR_UNIMPLEMENTED);
             EXPECT_FAILURE_WITH_ERRNO(kem->decapsulate(kem, server_shared_secret.data, ciphertext.data, private_key.data), S2N_ERR_UNIMPLEMENTED);
+            continue;
         }
+
+        /* Test a successful round-trip: keygen->enc->dec */
+        EXPECT_PQ_KEM_SUCCESS(kem->generate_keypair(kem, public_key.data, private_key.data));
+        EXPECT_PQ_KEM_SUCCESS(kem->encapsulate(kem, ciphertext.data, client_shared_secret.data, public_key.data));
+        EXPECT_PQ_KEM_SUCCESS(kem->decapsulate(kem, server_shared_secret.data, ciphertext.data, private_key.data));
+        EXPECT_BYTEARRAY_EQUAL(server_shared_secret.data, client_shared_secret.data, kem->shared_secret_key_length);
+
+        /* By design, if an invalid private key + ciphertext pair is provided to decapsulate(),
+         * the function should still succeed (return S2N_SUCCESS); however, the shared secret
+         * that was "decapsulated" will be a garbage random value. */
+        ciphertext.data[0] ^= 1; /* Flip a bit to invalidate the ciphertext */
+
+        EXPECT_PQ_KEM_SUCCESS(kem->decapsulate(kem, server_shared_secret.data, ciphertext.data, private_key.data));
+        EXPECT_BYTEARRAY_NOT_EQUAL(server_shared_secret.data, client_shared_secret.data, kem->shared_secret_key_length);
     }
 
     END_TEST();

tests/unit/s2n_security_policies_test.c

@@ -198,14 +198,14 @@ int main(int argc, char **argv)
         EXPECT_EQUAL(1, security_policy->kem_preferences->kem_count);
         EXPECT_NOT_NULL(security_policy->kem_preferences->kems);
         EXPECT_EQUAL(&s2n_kyber_512_r3, security_policy->kem_preferences->kems[0]);
-        EXPECT_EQUAL(security_policy->kem_preferences->tls13_kem_groups, pq_kem_groups_2024_10_all);
+        EXPECT_EQUAL(security_policy->kem_preferences->tls13_kem_groups, ALL_SUPPORTED_KEM_GROUPS);
         /* All supported kem groups should be in the preference list, but not all of them may be available. */
-        EXPECT_EQUAL(8, security_policy->kem_preferences->tls13_kem_group_count);
+        EXPECT_EQUAL(S2N_KEM_GROUPS_COUNT, security_policy->kem_preferences->tls13_kem_group_count);
         uint32_t available_groups = 0;
         EXPECT_OK(s2n_kem_preferences_groups_available(security_policy->kem_preferences, &available_groups));
         if (s2n_libcrypto_supports_evp_kem() && s2n_is_evp_apis_supported()) {
             if (s2n_libcrypto_supports_mlkem()) {
-                EXPECT_EQUAL(8, available_groups);
+                EXPECT_EQUAL(S2N_KEM_GROUPS_COUNT, available_groups);
             } else {
                 EXPECT_EQUAL(6, available_groups);
             }

tests/unit/s2n_server_key_share_extension_test.c

@@ -885,25 +885,23 @@ int main(int argc, char **argv)
                     S2N_STUFFER_READ_EXPECT_EQUAL(&stuffer, kem_group->iana_id, uint16);
                     S2N_STUFFER_READ_EXPECT_EQUAL(&stuffer, expected_hybrid_share_size, uint16);
 
+                    uint16_t expected_first_share_size = kem_group->curve->share_size;
+                    uint16_t expected_second_share_size = kem_group->kem->ciphertext_length;
+
                     if (kem_group->send_kem_first) {
-                        if (len_prefixed) {
-                            S2N_STUFFER_READ_EXPECT_EQUAL(&stuffer, kem_group->kem->ciphertext_length, uint16);
-                        }
-                        EXPECT_SUCCESS(s2n_stuffer_skip_read(&stuffer, kem_group->kem->ciphertext_length));
-                        if (len_prefixed) {
-                            S2N_STUFFER_READ_EXPECT_EQUAL(&stuffer, kem_group->curve->share_size, uint16);
-                        }
-                        S2N_STUFFER_LENGTH_WRITTEN_EXPECT_EQUAL(&stuffer, kem_group->curve->share_size);
-                    } else {
-                        if (len_prefixed) {
-                            S2N_STUFFER_READ_EXPECT_EQUAL(&stuffer, kem_group->curve->share_size, uint16);
-                        }
-                        EXPECT_SUCCESS(s2n_stuffer_skip_read(&stuffer, kem_group->curve->share_size));
-                        if (len_prefixed) {
-                            S2N_STUFFER_READ_EXPECT_EQUAL(&stuffer, kem_group->kem->ciphertext_length, uint16);
-                        }
-                        S2N_STUFFER_LENGTH_WRITTEN_EXPECT_EQUAL(&stuffer, kem_group->kem->ciphertext_length);
+                        expected_first_share_size = kem_group->kem->ciphertext_length;
+                        expected_second_share_size = kem_group->curve->share_size;
+                    }
+
+                    if (len_prefixed) {
+                        S2N_STUFFER_READ_EXPECT_EQUAL(&stuffer, expected_first_share_size, uint16);
+                    }
+                    EXPECT_SUCCESS(s2n_stuffer_skip_read(&stuffer, expected_first_share_size));
+
+                    if (len_prefixed) {
+                        S2N_STUFFER_READ_EXPECT_EQUAL(&stuffer, expected_second_share_size, uint16);
                     }
+                    S2N_STUFFER_LENGTH_WRITTEN_EXPECT_EQUAL(&stuffer, expected_second_share_size);
 
                     EXPECT_NULL(conn->kex_params.server_ecc_evp_params.negotiated_curve);
                     EXPECT_EQUAL(server_params->kem_group, kem_group);

tests/unit/s2n_tls13_pq_handshake_test.c

@@ -675,7 +675,7 @@ int main()
                 .client_policy = &mlkem768_test_policy,
                 .server_policy = &mlkem768_test_policy,
                 .expected_kem_group = null_if_no_mlkem,
-                .expected_curve = default_curve,
+                .expected_curve = NULL,
                 .hrr_expected = false,
                 .len_prefix_expected = false,
         }

tests/unit/s2n_x509_validator_time_verification_test.c

@@ -13,12 +13,10 @@
 * permissions and limitations under the License.
 */
 
+#include "crypto/s2n_libcrypto.h"
 #include "s2n_test.h"
 #include "testlib/s2n_testlib.h"
 
-bool s2n_libcrypto_supports_flag_no_check_time();
-uint64_t s2n_libcrypto_awslc_api_version(void);
-
 static uint8_t s2n_verify_host_accept_everything(const char *host_name, size_t host_name_len, void *data)
 {
     return 1;

tls/extensions/s2n_client_key_share.c

@@ -301,7 +301,9 @@ static int s2n_client_key_share_recv_ecc(struct s2n_connection *conn, struct s2n
 
 static int s2n_client_key_share_recv_hybrid_partial_ecc(struct s2n_stuffer *key_share, struct s2n_kem_group_params *new_client_params)
 {
+    POSIX_ENSURE_REF(new_client_params);
     const struct s2n_kem_group *kem_group = new_client_params->kem_group;
+    POSIX_ENSURE_REF(kem_group);
 
     if (new_client_params->kem_params.len_prefixed) {
         uint16_t ec_share_size = 0;

tls/s2n_kem.c

@@ -172,12 +172,12 @@ const struct s2n_kem_group s2n_x25519_kyber_768_r3 = {
 const struct s2n_kem_group *ALL_SUPPORTED_KEM_GROUPS[] = {
     &s2n_x25519_mlkem_768,
     &s2n_secp256r1_mlkem_768,
-    &s2n_secp256r1_kyber_512_r3,
-    &s2n_x25519_kyber_512_r3,
     &s2n_secp256r1_kyber_768_r3,
+    &s2n_x25519_kyber_768_r3,
     &s2n_secp384r1_kyber_768_r3,
     &s2n_secp521r1_kyber_1024_r3,
-    &s2n_x25519_kyber_768_r3,
+    &s2n_secp256r1_kyber_512_r3,
+    &s2n_x25519_kyber_512_r3,
 };
 
 /* Helper safety macro to call the NIST PQ KEM functions. The NIST
@@ -461,25 +461,35 @@ int s2n_kem_recv_ciphertext(struct s2n_stuffer *in, struct s2n_kem_params *kem_p
     return S2N_SUCCESS;
 }
 
+bool s2m_kem_is_available(const struct s2n_kem *kem)
+{
+    if (kem == NULL || kem->kem_nid == NID_undef) {
+        return false;
+    }
+
+    bool available = s2n_libcrypto_supports_evp_kem();
+
+    /* Only newer versions of libcrypto have ML-KEM support. */
+    if (kem == &s2n_mlkem_768) {
+        available &= s2n_libcrypto_supports_mlkem();
+    }
+
+    return available;
+}
+
 bool s2n_kem_group_is_available(const struct s2n_kem_group *kem_group)
 {
     /* Check for values that might be undefined when compiling for older libcrypto's */
-    if (kem_group == NULL || kem_group->curve == NULL || kem_group->kem == NULL
-            || kem_group->kem->kem_nid == NID_undef) {
+    if (kem_group == NULL || kem_group->curve == NULL || kem_group->kem == NULL) {
         return false;
     }
 
-    bool available = s2n_libcrypto_supports_evp_kem();
+    bool available = s2m_kem_is_available(kem_group->kem);
 
     /* x25519 based tls13_kem_groups require EVP_APIS_SUPPORTED */
     if (kem_group->curve == &s2n_ecc_curve_x25519) {
         available &= s2n_is_evp_apis_supported();
     }
 
-    /* Only newer versions of libcrypto have ML-KEM support. */
-    if (kem_group->kem == &s2n_mlkem_768) {
-        available &= s2n_libcrypto_supports_mlkem();
-    }
-
     return available;
 }

tls/s2n_kem.h

@@ -32,21 +32,21 @@ typedef uint16_t kem_ciphertext_key_size;
 #define OUT /* Indicates a function output */
 
 #if defined(S2N_LIBCRYPTO_SUPPORTS_EVP_KEM)
-    #if defined(S2N_LIBCRYPTO_SUPPORTS_MLKEM)
-        #define S2N_NID_MLKEM768 NID_MLKEM768
-    #else
-        #define S2N_NID_MLKEM768 NID_undef
-    #endif
     #define S2N_NID_KYBER512  NID_KYBER512_R3
     #define S2N_NID_KYBER768  NID_KYBER768_R3
     #define S2N_NID_KYBER1024 NID_KYBER1024_R3
 #else
-    #define S2N_NID_MLKEM768  NID_undef
     #define S2N_NID_KYBER512  NID_undef
     #define S2N_NID_KYBER768  NID_undef
     #define S2N_NID_KYBER1024 NID_undef
 #endif
 
+#if defined(S2N_LIBCRYPTO_SUPPORTS_MLKEM)
+    #define S2N_NID_MLKEM768 NID_MLKEM768
+#else
+    #define S2N_NID_MLKEM768 NID_undef
+#endif
+
 struct s2n_kem {
     const char *name;
     int kem_nid;
@@ -132,6 +132,7 @@ int s2n_kem_send_public_key(struct s2n_stuffer *out, struct s2n_kem_params *kem_
 int s2n_kem_recv_public_key(struct s2n_stuffer *in, struct s2n_kem_params *kem_params);
 int s2n_kem_send_ciphertext(struct s2n_stuffer *out, struct s2n_kem_params *kem_params);
 int s2n_kem_recv_ciphertext(struct s2n_stuffer *in, struct s2n_kem_params *kem_params);
+bool s2m_kem_is_available(const struct s2n_kem *kem);
 bool s2n_kem_group_is_available(const struct s2n_kem_group *kem_group);
 
 /* mlkem768 */

tls/s2n_kem_preferences.c

@@ -15,6 +15,8 @@
 
 #include "tls/s2n_kem_preferences.h"
 
+#include "tls/s2n_kem.h"
+
 const struct s2n_kem *pq_kems_r3_2021_05[] = {
     /* Round 3 Algorithms */
     &s2n_kyber_512_r3,
@@ -41,18 +43,6 @@ const struct s2n_kem_group *pq_kem_groups_r3_2023_12[] = {
     &s2n_secp256r1_kyber_512_r3,
 };
 
-/* Includes both Kyber and ML-KEM KEMs. */
-const struct s2n_kem_group *pq_kem_groups_2024_10_all[] = {
-    &s2n_x25519_mlkem_768,
-    &s2n_secp256r1_mlkem_768,
-    &s2n_secp256r1_kyber_768_r3,
-    &s2n_x25519_kyber_768_r3,
-    &s2n_secp384r1_kyber_768_r3,
-    &s2n_secp521r1_kyber_1024_r3,
-    &s2n_secp256r1_kyber_512_r3,
-    &s2n_x25519_kyber_512_r3,
-};
-
 const struct s2n_kem_preferences kem_preferences_pq_tls_1_0_2021_05 = {
     .kem_count = s2n_array_len(pq_kems_r3_2021_05),
     .kems = pq_kems_r3_2021_05,
@@ -90,8 +80,8 @@ const struct s2n_kem_preferences kem_preferences_pq_tls_1_3_2023_12 = {
 const struct s2n_kem_preferences kem_preferences_all = {
     .kem_count = s2n_array_len(pq_kems_r3_2021_05),
     .kems = pq_kems_r3_2021_05,
-    .tls13_kem_group_count = s2n_array_len(pq_kem_groups_2024_10_all),
-    .tls13_kem_groups = pq_kem_groups_2024_10_all,
+    .tls13_kem_group_count = S2N_KEM_GROUPS_COUNT,
+    .tls13_kem_groups = ALL_SUPPORTED_KEM_GROUPS,
     .tls13_pq_hybrid_draft_revision = 5
 };