Fix all tests under S2N_NO_PQ

aws · Sep 29, 2023 · f668698 · f668698
1 parent cc80471
commit f668698
Show file tree

Hide file tree

Showing 6 changed files with 20 additions and 12 deletions.
diff --git a/tests/unit/s2n_client_key_share_extension_pq_test.c b/tests/unit/s2n_client_key_share_extension_pq_test.c
@@ -479,7 +479,7 @@ int main()
                          * iteration of the outer loop of this test (index i), we populate test_kem_groups[] with a
                          * different permutation of all_kem_groups[] to ensure we handle each kem_group key share
                          * correctly. */
-                        const struct s2n_kem_group *test_kem_groups[S2N_KEM_GROUPS_COUNT];
+                        struct s2n_kem_group *test_kem_groups[S2N_KEM_GROUPS_COUNT];
                         for (size_t j = 0; j < S2N_KEM_GROUPS_COUNT; j++) {
                             /* cppcheck-suppress moduloofone */
                             test_kem_groups[j] = ALL_SUPPORTED_KEM_GROUPS[(j + i) % S2N_KEM_GROUPS_COUNT];

diff --git a/tests/unit/s2n_server_key_share_extension_test.c b/tests/unit/s2n_server_key_share_extension_test.c
@@ -577,6 +577,9 @@ int main(int argc, char **argv)
                 {
                     for (size_t i = 0; i < s2n_array_len(test_kem_groups); i++) {
                         const struct s2n_kem_group *kem_group = test_kem_groups[i];
+                        if (!kem_group->available) {
+                            continue;
+                        }
                         struct s2n_connection *client_conn = NULL;
                         EXPECT_NOT_NULL(client_conn = s2n_connection_new(S2N_CLIENT));
                         client_conn->security_policy_override = &test_security_policy;
@@ -836,10 +839,10 @@ int main(int argc, char **argv)
                     conn->kex_params.server_ecc_evp_params.negotiated_curve = NULL;
 
                     struct s2n_kem_group_params *server_params = &conn->kex_params.server_kem_group_params;
-                    if (i >= kem_pref->tls13_kem_group_count || !kem_pref->tls13_kem_groups[i]->available) {
+                    const struct s2n_kem_group *kem_group = kem_pref->tls13_kem_groups[i];
+                    if (!kem_group->available) {
                         continue;
                     }
-                    const struct s2n_kem_group *kem_group = kem_pref->tls13_kem_groups[i];
                     server_params->kem_group = kem_group;
                     server_params->kem_params.kem = kem_group->kem;
                     server_params->ecc_params.negotiated_curve = kem_group->curve;

diff --git a/tests/unit/s2n_tls13_hybrid_shared_secret_test.c b/tests/unit/s2n_tls13_hybrid_shared_secret_test.c
@@ -414,14 +414,11 @@ int main(int argc, char **argv)
             EXPECT_NOT_NULL(client_conn = s2n_connection_new(S2N_CLIENT));
             EXPECT_NOT_NULL(server_conn = s2n_connection_new(S2N_SERVER));
 
-            if (kem_group->available) {
-                EXPECT_SUCCESS(set_up_conns(client_conn, server_conn, test_vector->client_ecc_key,
-                        test_vector->server_ecc_key, kem_group, test_vector->pq_secret));
-            } else {
-                EXPECT_FAILURE(set_up_conns(client_conn, server_conn, test_vector->client_ecc_key,
-                        test_vector->server_ecc_key, kem_group, test_vector->pq_secret));
-                continue;
-            }
+            // Expect success here regardless of whether a given |kem_group| is
+            // available because we will fall back on ECDH if no KEMs are
+            // available.
+            EXPECT_SUCCESS(set_up_conns(client_conn, server_conn, test_vector->client_ecc_key,
+                    test_vector->server_ecc_key, kem_group, test_vector->pq_secret));
 
             /* Calculate the hybrid shared secret */
             DEFER_CLEANUP(struct s2n_blob client_calculated_shared_secret = { 0 }, s2n_free);

diff --git a/tls/extensions/s2n_server_key_share.c b/tls/extensions/s2n_server_key_share.c
@@ -333,6 +333,12 @@ static int s2n_server_key_share_recv(struct s2n_connection *conn, struct s2n_stu
         POSIX_GUARD(s2n_server_key_share_recv_ecc(conn, negotiated_named_group_iana, extension));
     } else if (s2n_kem_preferences_includes_tls13_kem_group(kem_pref, negotiated_named_group_iana)) {
         POSIX_GUARD(s2n_server_key_share_recv_pq_hybrid(conn, negotiated_named_group_iana, extension));
+    } else if (!s2n_pq_is_enabled() && negotiated_named_group_iana >= TLS_PQ_KEM_GROUP_ID_START) {
+        // |s2n_kem_preferences_includes_tls13_kem_group| will return false if
+        // PQ is disabled and thus no KEM groups are supported, so check
+        // whether the IANA name we've recieved indicates PQ KEM and return an
+        // appropriate error.
+        POSIX_BAIL(S2N_ERR_PQ_DISABLED);
     } else {
         POSIX_BAIL(S2N_ERR_ECDHE_UNSUPPORTED_CURVE);
     }

diff --git a/tls/s2n_kem_preferences.c b/tls/s2n_kem_preferences.c
@@ -84,7 +84,8 @@ bool s2n_kem_preferences_includes_tls13_kem_group(const struct s2n_kem_preferenc
     }
 
     for (size_t i = 0; i < kem_preferences->tls13_kem_group_count; i++) {
-        if (query_iana_id == kem_preferences->tls13_kem_groups[i]->iana_id && kem_preferences->tls13_kem_groups[i]->available) {
+        if (query_iana_id == kem_preferences->tls13_kem_groups[i]->iana_id
+                && kem_preferences->tls13_kem_groups[i]->available) {
             return true;
         }
     }

diff --git a/tls/s2n_tls_parameters.h b/tls/s2n_tls_parameters.h
@@ -69,6 +69,7 @@
  * https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml
  */
 #define TLS_PQ_KEM_GROUP_ID_X25519_KYBER_512_R3     0x2F39
+#define TLS_PQ_KEM_GROUP_ID_START TLS_PQ_KEM_GROUP_ID_X25519_KYBER_512_R3
 #define TLS_PQ_KEM_GROUP_ID_SECP256R1_KYBER_512_R3  0x2F3A
 #define TLS_PQ_KEM_GROUP_ID_SECP384R1_KYBER_768_R3  0x2F3C
 #define TLS_PQ_KEM_GROUP_ID_SECP521R1_KYBER_1024_R3 0x2F3D