S2N client negotation of un-offered group fix (#3422)

Fix s2n client negotiation of un-offered group + unit tests

Author: franklee26

crypto/s2n_ecc_evp.c

@@ -23,6 +23,8 @@
 
 #include <stdint.h>
 
+#include "tls/s2n_connection.h"
+#include "tls/s2n_ecc_preferences.h"
 #include "tls/s2n_tls_parameters.h"
 #include "utils/s2n_mem.h"
 #include "utils/s2n_safety.h"
@@ -478,21 +480,26 @@ int s2n_ecc_evp_parse_params_point(struct s2n_blob *point_blob, struct s2n_ecc_e
     return 0;
 }
 
-int s2n_ecc_evp_parse_params(struct s2n_ecdhe_raw_server_params *raw_server_ecc_params,
-                             struct s2n_ecc_evp_params *ecc_evp_params) {
-    S2N_ERROR_IF(
-        s2n_ecc_evp_find_supported_curve(&raw_server_ecc_params->curve_blob, &ecc_evp_params->negotiated_curve) != 0,
-        S2N_ERR_ECDHE_UNSUPPORTED_CURVE);
+int s2n_ecc_evp_parse_params(struct s2n_connection* conn,
+                             struct s2n_ecdhe_raw_server_params* raw_server_ecc_params,
+                             struct s2n_ecc_evp_params* ecc_evp_params) {
+    POSIX_ENSURE(
+            s2n_ecc_evp_find_supported_curve(conn, &raw_server_ecc_params->curve_blob, &ecc_evp_params->negotiated_curve) == 0,
+            S2N_ERR_ECDHE_UNSUPPORTED_CURVE);
     return s2n_ecc_evp_parse_params_point(&raw_server_ecc_params->point_blob, ecc_evp_params);
 }
 
-int s2n_ecc_evp_find_supported_curve(struct s2n_blob *iana_ids, const struct s2n_ecc_named_curve **found) {
+int s2n_ecc_evp_find_supported_curve(struct s2n_connection* conn, struct s2n_blob *iana_ids, const struct s2n_ecc_named_curve **found) {
+    const struct s2n_ecc_preferences* ecc_prefs = NULL;
+    POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_prefs));
+    POSIX_ENSURE_REF(ecc_prefs);
+
     struct s2n_stuffer iana_ids_in = {0};
 
     POSIX_GUARD(s2n_stuffer_init(&iana_ids_in, iana_ids));
     POSIX_GUARD(s2n_stuffer_write(&iana_ids_in, iana_ids));
-    for (size_t i = 0; i < s2n_all_supported_curves_list_len; i++) {
-        const struct s2n_ecc_named_curve *supported_curve = s2n_all_supported_curves_list[i];
+    for (size_t i = 0; i < ecc_prefs->count; i++) {
+        const struct s2n_ecc_named_curve *supported_curve = ecc_prefs->ecc_curves[i];
         for (uint32_t j = 0; j < iana_ids->size / 2; j++) {
             uint16_t iana_id;
             POSIX_GUARD(s2n_stuffer_read_uint16(&iana_ids_in, &iana_id));

crypto/s2n_ecc_evp.h

@@ -70,10 +70,7 @@ int s2n_ecc_evp_compute_shared_secret_from_params(struct s2n_ecc_evp_params *pri
                                                   struct s2n_blob *shared_key);
 int s2n_ecc_evp_write_params_point(struct s2n_ecc_evp_params *ecc_evp_params, struct s2n_stuffer *out);
 int s2n_ecc_evp_read_params_point(struct s2n_stuffer *in, int point_size, struct s2n_blob *point_blob);
-int s2n_ecc_evp_compute_shared_secret_from_params(struct s2n_ecc_evp_params *private_ecc_evp_params,
-                                                  struct s2n_ecc_evp_params *public_ecc_evp_params,
-                                                  struct s2n_blob *shared_key);
-int s2n_ecc_evp_compute_shared_secret_as_server(struct s2n_ecc_evp_params *server_ecc_evp_params, 
+int s2n_ecc_evp_compute_shared_secret_as_server(struct s2n_ecc_evp_params *server_ecc_evp_params,
                                             struct s2n_stuffer *Yc_in, struct s2n_blob *shared_key);
 int s2n_ecc_evp_compute_shared_secret_as_client(struct s2n_ecc_evp_params *server_ecc_evp_params, 
                                             struct s2n_stuffer *Yc_out, struct s2n_blob *shared_key); 
@@ -82,8 +79,9 @@ int s2n_ecc_evp_write_params(struct s2n_ecc_evp_params *ecc_evp_params, struct s
                              struct s2n_blob *written);
 int s2n_ecc_evp_read_params(struct s2n_stuffer *in, struct s2n_blob *data_to_verify,
                             struct s2n_ecdhe_raw_server_params *raw_server_ecc_params);
-int s2n_ecc_evp_parse_params(struct s2n_ecdhe_raw_server_params *raw_server_ecc_params,
-                             struct s2n_ecc_evp_params *ecc_evp_params);
-int s2n_ecc_evp_find_supported_curve(struct s2n_blob *iana_ids, const struct s2n_ecc_named_curve **found);
+int s2n_ecc_evp_parse_params(struct s2n_connection *conn,
+                             struct s2n_ecdhe_raw_server_params *raw_server_ecc_params,
+                             struct s2n_ecc_evp_params* ecc_evp_params);
+int s2n_ecc_evp_find_supported_curve(struct s2n_connection* conn, struct s2n_blob *iana_ids, const struct s2n_ecc_named_curve **found);
 int s2n_ecc_evp_params_free(struct s2n_ecc_evp_params *ecc_evp_params);
 int s2n_is_evp_apis_supported();

tests/unit/s2n_ecc_evp_test.c

@@ -20,10 +20,14 @@
 #include "crypto/s2n_ecc_evp.h"
 #include "stuffer/s2n_stuffer.h"
 #include "testlib/s2n_testlib.h"
+#include "tls/s2n_connection.h"
+#include "tls/s2n_security_policies.h"
 #include "utils/s2n_mem.h"
 
 #define ECDHE_PARAMS_LEGACY_FORM 4
 
+extern const struct s2n_ecc_named_curve s2n_unsupported_curve;
+
 int main(int argc, char **argv) {
     BEGIN_TEST();
     EXPECT_SUCCESS(s2n_disable_tls13_in_test());
@@ -89,7 +93,7 @@ int main(int argc, char **argv) {
     }
     {
         /* Test failure case for computing shared key for all supported curves when the server
-        and client curves donot match */
+        and client curves do not match */
         for (int i = 0; i < s2n_all_supported_curves_list_len; i++) {
             for (int j = 0; j < s2n_all_supported_curves_list_len; j++) {
                 struct s2n_ecc_evp_params server_params = {0};
@@ -216,6 +220,10 @@ int main(int argc, char **argv) {
         }
     }
     {
+        DEFER_CLEANUP(struct s2n_connection* conn = s2n_connection_new(S2N_CLIENT),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(conn);
+        EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(conn, "test_all"));
         /* Test read/write/parse params for all supported curves */
         for (int i = 0; i < s2n_all_supported_curves_list_len; i++) {
             struct s2n_ecc_evp_params write_params = {0};
@@ -238,23 +246,26 @@ int main(int argc, char **argv) {
 
              /* Read params points from the wire */
             EXPECT_SUCCESS(s2n_ecc_evp_read_params(&wire, &ecdh_params_received, &ecdhe_data));
-            EXPECT_SUCCESS(s2n_ecc_evp_parse_params(&ecdhe_data, &read_params));
+            EXPECT_SUCCESS(s2n_ecc_evp_parse_params(conn, &ecdhe_data, &read_params));
 
             /* Check that the point we read is the same we wrote */
             EXPECT_TRUE(EVP_PKEY_cmp(write_params.evp_pkey, read_params.evp_pkey));
 
-            /* Clean up */ 
+            /* Clean up */
             EXPECT_SUCCESS(s2n_stuffer_free(&wire));
             EXPECT_SUCCESS(s2n_ecc_evp_params_free(&write_params));
             EXPECT_SUCCESS(s2n_ecc_evp_params_free(&read_params));
         }
     }
     {
+        DEFER_CLEANUP(struct s2n_connection* conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(conn);
+        EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(conn, "test_all"));
         /* Test generate/read/write/parse and compute shared secrets for all supported curves */
         for (int i = 0; i < s2n_all_supported_curves_list_len; i++) {
             struct s2n_ecc_evp_params server_params = {0};
             struct s2n_ecc_evp_params read_params = {0};
-            struct s2n_ecc_evp_params client_params = {0}; 
+            struct s2n_ecc_evp_params client_params = {0};
             struct s2n_stuffer wire;
             struct s2n_blob ecdh_params_sent, ecdh_params_received;
             struct s2n_blob server_shared_secret, client_shared_secret;
@@ -274,7 +285,7 @@ int main(int argc, char **argv) {
             /* Client reads the public */
             struct s2n_ecdhe_raw_server_params ecdhe_data = {0};
             EXPECT_SUCCESS(s2n_ecc_evp_read_params(&wire, &ecdh_params_received, &ecdhe_data));
-            EXPECT_SUCCESS(s2n_ecc_evp_parse_params(&ecdhe_data, &read_params));
+            EXPECT_SUCCESS(s2n_ecc_evp_parse_params(conn, &ecdhe_data, &read_params));
 
             /* Verify if the client correctly read the server public */
             EXPECT_TRUE(EVP_PKEY_cmp(server_params.evp_pkey, read_params.evp_pkey));
@@ -283,11 +294,11 @@ int main(int argc, char **argv) {
             client_params.negotiated_curve =s2n_all_supported_curves_list[i];
             EXPECT_SUCCESS(s2n_ecc_evp_generate_ephemeral_key(&client_params));
             EXPECT_NOT_NULL(client_params.evp_pkey);
-        
+
             /* Compute shared secret for the server */
             EXPECT_SUCCESS(
                 s2n_ecc_evp_compute_shared_secret_from_params(&server_params, &client_params, &server_shared_secret));
-            
+
             /* Compute shared secret for the client */
             EXPECT_SUCCESS(
                 s2n_ecc_evp_compute_shared_secret_from_params(&client_params, &read_params, &client_shared_secret));
@@ -299,15 +310,18 @@ int main(int argc, char **argv) {
             /* Clean up */
             EXPECT_SUCCESS(s2n_stuffer_free(&wire));
             EXPECT_SUCCESS(s2n_free(&server_shared_secret));
-            EXPECT_SUCCESS(s2n_free(&client_shared_secret)); 
+            EXPECT_SUCCESS(s2n_free(&client_shared_secret));
             EXPECT_SUCCESS(s2n_ecc_evp_params_free(&server_params));
             EXPECT_SUCCESS(s2n_ecc_evp_params_free(&read_params));
             EXPECT_SUCCESS(s2n_ecc_evp_params_free(&client_params));
         }
     }
     {
+        DEFER_CLEANUP(struct s2n_connection* conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(conn);
+        EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(conn, "test_all"));
         /* Test generate->write->read->compute_shared with all supported curves */
-    for (int i = 0; i < s2n_all_supported_curves_list_len; i++) {
+        for (int i = 0; i < s2n_all_supported_curves_list_len; i++) {
             struct s2n_ecc_evp_params server_params = {0}, client_params = {0};
             struct s2n_stuffer wire;
             struct s2n_blob server_shared, client_shared, ecdh_params_sent, ecdh_params_received;
@@ -323,7 +337,7 @@ int main(int argc, char **argv) {
             /* Client reads the public */
             struct s2n_ecdhe_raw_server_params ecdhe_data = {0};
             EXPECT_SUCCESS(s2n_ecc_evp_read_params(&wire, &ecdh_params_received, &ecdhe_data));
-            EXPECT_SUCCESS(s2n_ecc_evp_parse_params(&ecdhe_data, &client_params));
+            EXPECT_SUCCESS(s2n_ecc_evp_parse_params(conn, &ecdhe_data, &client_params));
 
             /* The client got the curve */
             EXPECT_EQUAL(client_params.negotiated_curve, server_params.negotiated_curve);
@@ -345,5 +359,52 @@ int main(int argc, char **argv) {
         }
     }
 
+    /* Test that the client does not negotiate a group that was not
+     * offered in EC preferences */
+    {
+        const struct s2n_security_policy* security_policy = NULL;
+        DEFER_CLEANUP(struct s2n_connection* conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(conn);
+        /* Version does not include the unsupported curve and secp521r1, which will be used by a malicious server */
+        EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(conn, "20190802"));
+        EXPECT_SUCCESS(s2n_connection_get_security_policy(conn, &security_policy));
+
+        /* Setup & verify invalid curves, which will be selected by a malicious server */
+        const struct s2n_ecc_named_curve* const unrequested_curves[] = {
+                &s2n_unsupported_curve,
+                &s2n_ecc_curve_secp521r1,
+        };
+
+        /* Verify that the client errors when the server attempts to
+         * negotiate a curve that was never offered */
+        for (size_t i = 0; i < s2n_array_len(unrequested_curves); i++) {
+            struct s2n_ecc_evp_params server_params = {0};
+            struct s2n_ecc_evp_params client_params = {0};
+            struct s2n_stuffer wire = {0};
+            struct s2n_blob ecdh_params_sent = {0}, ecdh_params_received = {0};
+
+            EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&wire, 1024));
+
+            /* Server maliciously chooses an unsupported curve */
+            server_params.negotiated_curve = unrequested_curves[i];
+            EXPECT_SUCCESS(s2n_ecc_evp_generate_ephemeral_key(&server_params));
+            EXPECT_NOT_NULL(server_params.evp_pkey);
+            /* Server sends the public */
+            EXPECT_SUCCESS(s2n_ecc_evp_write_params(&server_params, &wire, &ecdh_params_sent));
+            /* Client reads the public */
+            struct s2n_ecdhe_raw_server_params ecdhe_data = {0};
+            EXPECT_SUCCESS(s2n_ecc_evp_read_params(&wire, &ecdh_params_received, &ecdhe_data));
+            EXPECT_FAILURE_WITH_ERRNO(
+                    s2n_ecc_evp_parse_params(conn, &ecdhe_data, &client_params), S2N_ERR_ECDHE_UNSUPPORTED_CURVE);
+
+            /* The client didn't agree on a curve */
+            EXPECT_NULL(client_params.negotiated_curve);
+
+            /* Clean up */
+            EXPECT_SUCCESS(s2n_stuffer_free(&wire));
+            EXPECT_SUCCESS(s2n_ecc_evp_params_free(&server_params));
+            EXPECT_SUCCESS(s2n_ecc_evp_params_free(&client_params));
+        }
+    }
     END_TEST();
 }

tls/s2n_server_key_exchange.c

@@ -100,7 +100,7 @@ int s2n_ecdhe_server_key_recv_read_data(struct s2n_connection *conn, struct s2n_
 
 int s2n_ecdhe_server_key_recv_parse_data(struct s2n_connection *conn, struct s2n_kex_raw_server_data *raw_server_data)
 {
-    POSIX_GUARD(s2n_ecc_evp_parse_params(&raw_server_data->ecdhe_data, &conn->kex_params.server_ecc_evp_params));
+    POSIX_GUARD(s2n_ecc_evp_parse_params(conn, &raw_server_data->ecdhe_data, &conn->kex_params.server_ecc_evp_params));
 
     return 0;
 }