Support TLS 1.3 clients that do not specify signature algorithms (#2222)

zz85 · baldwinmatt · web-flow · commit 1616f313f69c · 2020-08-21T11:21:50.000-07:00
* Pick a certificate signature algorithm in TLS 1.3 even if client does
not indicate support.
* int -&gt; size_t
* Tests
* Use s2n_is_rsa_pss_signing_supported()
* Simplify to GUARD, rename to s2n_tls13_preferred_sig_scheme for clarity
* Add line break
* Add s2n_is_signature_scheme_usable() to reduce duplication
* S2N_ERR_EMPTY_SIGNATURE_SCHEME is no longer useful, remove it
* Reorder TLS 1.3 default signature scheme selection
* Update tls/s2n_signature_algorithms.c
Co-authored-by: Matthew Baldwin &lt;baldwinmatt@users.noreply.github.com&gt;
diff --git a/error/s2n_errno.c b/error/s2n_errno.c
@@ -70,7 +70,6 @@ static const char *no_such_error = "Internal s2n error";
     ERR_ENTRY(S2N_ERR_DECODE_PRIVATE_KEY, "error decoding private key") \
     ERR_ENTRY(S2N_ERR_INVALID_SIGNATURE_ALGORITHM, "Invalid signature algorithm") \
     ERR_ENTRY(S2N_ERR_INVALID_SIGNATURE_SCHEME, "Invalid signature scheme") \
-    ERR_ENTRY(S2N_ERR_EMPTY_SIGNATURE_SCHEME, "Empty signature scheme") \
     ERR_ENTRY(S2N_ERR_CBC_VERIFY, "Failed CBC verification") \
     ERR_ENTRY(S2N_ERR_DH_COPYING_PUBLIC_KEY, "error copying Diffie-Hellman public key") \
     ERR_ENTRY(S2N_ERR_SIGN, "error signing data") \
diff --git a/error/s2n_errno.h b/error/s2n_errno.h
@@ -88,7 +88,6 @@ typedef enum {
     S2N_ERR_INVALID_HELLO_RETRY,
     S2N_ERR_INVALID_SIGNATURE_ALGORITHM,
     S2N_ERR_INVALID_SIGNATURE_SCHEME,
-    S2N_ERR_EMPTY_SIGNATURE_SCHEME,
     S2N_ERR_CBC_VERIFY,
     S2N_ERR_DH_COPYING_PUBLIC_KEY,
     S2N_ERR_SIGN,
diff --git a/tests/unit/s2n_record_size_test.c b/tests/unit/s2n_record_size_test.c
@@ -43,6 +43,7 @@ static int destroy_server_keys(struct s2n_connection *server_conn)
 {
     GUARD(server_conn->initial.cipher_suite->record_alg->cipher->destroy_key(&server_conn->initial.server_key));
     GUARD(server_conn->initial.cipher_suite->record_alg->cipher->destroy_key(&server_conn->initial.client_key));
+
     return S2N_SUCCESS;
 }
 
diff --git a/tests/unit/s2n_signature_algorithms_test.c b/tests/unit/s2n_signature_algorithms_test.c
@@ -401,14 +401,13 @@ int main(int argc, char **argv)
             EXPECT_SUCCESS(s2n_choose_sig_scheme_from_peer_preference_list(conn, &peer_list, &result));
             EXPECT_EQUAL(result.iana_value, default_scheme.iana_value);
 
-            /* TLS1.3 does not allow defaults */
+            /* If we cannot find a match in TLS1.3, allow defaults for success */
             conn->secure.cipher_suite = TLS13_CIPHER_SUITE;
             conn->actual_protocol_version = S2N_TLS13;
-            EXPECT_FAILURE_WITH_ERRNO(s2n_choose_sig_scheme_from_peer_preference_list(conn, &peer_list, &result),
-                    S2N_ERR_EMPTY_SIGNATURE_SCHEME);
+            EXPECT_SUCCESS(s2n_choose_sig_scheme_from_peer_preference_list(conn, &peer_list, &result));
         }
 
-        /* Test: no shared valid signature schemes, using TLS1.3 */
+        /* Test: no shared valid signature schemes, using TLS1.3. Server picks preferred */
         {
             conn->secure.cipher_suite = TLS13_CIPHER_SUITE;
             conn->actual_protocol_version = S2N_TLS13;
@@ -420,8 +419,9 @@ int main(int argc, char **argv)
                     },
             };
 
-            EXPECT_FAILURE_WITH_ERRNO(s2n_choose_sig_scheme_from_peer_preference_list(conn, &peer_list, &result),
-                    S2N_ERR_INVALID_SIGNATURE_SCHEME);
+            /* behavior is that we fallback to a preferred signature algorithm */
+            EXPECT_SUCCESS(s2n_choose_sig_scheme_from_peer_preference_list(conn, &peer_list, &result));
+            EXPECT_EQUAL(result.iana_value, s2n_ecdsa_sha384.iana_value);
         }
 
         /* Test: no shared valid signature schemes, using TLS1.2 */
@@ -625,6 +625,76 @@ int main(int argc, char **argv)
         s2n_config_free(config);
     }
 
+    /* Test fallback of TLS 1.3 signature algorithms */
+    if (s2n_is_rsa_pss_signing_supported())
+    {
+        struct s2n_config *config = s2n_config_new();
+
+        struct s2n_connection *conn = s2n_connection_new(S2N_SERVER);
+        EXPECT_SUCCESS(s2n_connection_set_config(conn, config));
+
+        const struct s2n_security_policy *security_policy = NULL;
+        EXPECT_SUCCESS(s2n_connection_get_security_policy(conn, &security_policy));
+        EXPECT_NOT_NULL(security_policy);
+
+        const struct s2n_signature_scheme *const test_rsae_signature_schemes[] = {
+            &s2n_rsa_pss_rsae_sha256,
+        };
+
+        const struct s2n_signature_preferences test_rsae_preferences = {
+            .count = 1,
+            .signature_schemes = test_rsae_signature_schemes,
+        };
+
+        struct s2n_security_policy test_security_policy = {
+            .minimum_protocol_version = security_policy->minimum_protocol_version,
+            .cipher_preferences = security_policy->cipher_preferences,
+            .kem_preferences = security_policy->kem_preferences,
+            .signature_preferences = &test_rsae_preferences,
+            .ecc_preferences = security_policy->ecc_preferences,
+        };
+
+        config->security_policy = &test_security_policy;
+
+        struct s2n_signature_scheme result;
+
+        /* Test: no shared valid signature schemes, using TLS1.3. Server cant pick preferred */
+        {
+            conn->secure.cipher_suite = TLS13_CIPHER_SUITE;
+            conn->actual_protocol_version = S2N_TLS13;
+
+            struct s2n_sig_scheme_list peer_list = {
+                    .len = 1, .iana_list = {
+                        s2n_rsa_pkcs1_sha224.iana_value, /* Invalid (wrong protocol version) */
+                    },
+            };
+
+            EXPECT_FAILURE_WITH_ERRNO(s2n_choose_sig_scheme_from_peer_preference_list(conn, &peer_list, &result),
+                S2N_ERR_INVALID_SIGNATURE_SCHEME);
+        }
+
+        EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, rsa_cert_chain));
+
+        /* Test: no shared valid signature schemes, using TLS1.3. Server picks a preferred */
+        {
+            conn->secure.cipher_suite = TLS13_CIPHER_SUITE;
+            conn->actual_protocol_version = S2N_TLS13;
+
+            struct s2n_sig_scheme_list peer_list = {
+                    .len = 1, .iana_list = {
+                        s2n_rsa_pkcs1_sha224.iana_value, /* Invalid (wrong protocol version) */
+                    },
+            };
+
+            /* behavior is that we fallback to a preferred signature algorithm */
+            EXPECT_SUCCESS(s2n_choose_sig_scheme_from_peer_preference_list(conn, &peer_list, &result));
+            EXPECT_EQUAL(result.iana_value, s2n_rsa_pss_rsae_sha256.iana_value);
+        }
+
+        s2n_connection_free(conn);
+        s2n_config_free(config);
+    }
+
     EXPECT_SUCCESS(s2n_cert_chain_and_key_free(rsa_cert_chain));
     EXPECT_SUCCESS(s2n_cert_chain_and_key_free(ecdsa_cert_chain));
 
diff --git a/tls/s2n_signature_algorithms.c b/tls/s2n_signature_algorithms.c
@@ -57,6 +57,16 @@ static int s2n_signature_scheme_valid_to_accept(struct s2n_connection *conn, con
     return 0;
 }
 
+static int s2n_is_signature_scheme_usable(struct s2n_connection *conn, const struct s2n_signature_scheme *candidate) {
+    notnull_check(conn);
+    notnull_check(candidate);
+
+    GUARD(s2n_signature_scheme_valid_to_accept(conn, candidate));
+    GUARD(s2n_is_sig_scheme_valid_for_auth(conn, candidate));
+
+    return S2N_SUCCESS;
+}
+
 static int s2n_choose_sig_scheme(struct s2n_connection *conn, struct s2n_sig_scheme_list *peer_wire_prefs,
                           struct s2n_signature_scheme *chosen_scheme_out)
 {
@@ -68,18 +78,14 @@ static int s2n_choose_sig_scheme(struct s2n_connection *conn, struct s2n_sig_sch
     struct s2n_cipher_suite *cipher_suite = conn->secure.cipher_suite;
     notnull_check(cipher_suite);
 
-    for (int i = 0; i < signature_preferences->count; i++) {
+    for (size_t i = 0; i < signature_preferences->count; i++) {
         const struct s2n_signature_scheme *candidate = signature_preferences->signature_schemes[i];
 
-        if (s2n_signature_scheme_valid_to_accept(conn, candidate) != S2N_SUCCESS) {
-            continue;
-        }
-
-        if (s2n_is_sig_scheme_valid_for_auth(conn, candidate) != S2N_SUCCESS) {
+        if (s2n_is_signature_scheme_usable(conn, candidate) != S2N_SUCCESS) {
             continue;
         }
 
-        for (int j = 0; j < peer_wire_prefs->len; j++) {
+        for (size_t j = 0; j < peer_wire_prefs->len; j++) {
             uint16_t their_iana_val = peer_wire_prefs->iana_list[j];
 
             if (candidate->iana_value == their_iana_val) {
@@ -89,6 +95,32 @@ static int s2n_choose_sig_scheme(struct s2n_connection *conn, struct s2n_sig_sch
         }
     }
 
+    /* do not error even if there's no match */
+    return S2N_SUCCESS;
+}
+
+/* similar to s2n_choose_sig_scheme() without matching client's preference */
+static int s2n_tls13_default_sig_scheme(struct s2n_connection *conn, struct s2n_signature_scheme *chosen_scheme_out)
+{
+    notnull_check(conn);
+    const struct s2n_signature_preferences *signature_preferences = NULL;
+    GUARD(s2n_connection_get_signature_preferences(conn, &signature_preferences));
+    notnull_check(signature_preferences);
+
+    struct s2n_cipher_suite *cipher_suite = conn->secure.cipher_suite;
+    notnull_check(cipher_suite);
+
+    for (size_t i = 0; i < signature_preferences->count; i++) {
+        const struct s2n_signature_scheme *candidate = signature_preferences->signature_schemes[i];
+
+        if (s2n_is_signature_scheme_usable(conn, candidate) != S2N_SUCCESS) {
+            continue;
+        }
+
+        *chosen_scheme_out = *candidate;
+        return S2N_SUCCESS;
+    }
+
     S2N_ERROR(S2N_ERR_INVALID_SIGNATURE_SCHEME);
 }
 
@@ -102,7 +134,7 @@ int s2n_get_and_validate_negotiated_signature_scheme(struct s2n_connection *conn
     GUARD(s2n_connection_get_signature_preferences(conn, &signature_preferences));
     notnull_check(signature_preferences);
 
-    for (int i = 0; i < signature_preferences->count; i++) {
+    for (size_t i = 0; i < signature_preferences->count; i++) {
         const struct s2n_signature_scheme *candidate = signature_preferences->signature_schemes[i];
 
         if (0 != s2n_signature_scheme_valid_to_accept(conn, candidate)) {
@@ -167,18 +199,17 @@ int s2n_choose_sig_scheme_from_peer_preference_list(struct s2n_connection *conn,
 
     struct s2n_signature_scheme chosen_scheme;
 
-    GUARD(s2n_choose_default_sig_scheme(conn, &chosen_scheme));
+    if (conn->actual_protocol_version < S2N_TLS13) {
+        GUARD(s2n_choose_default_sig_scheme(conn, &chosen_scheme));
+    } else {
+        /* Pick a default signature algorithm in TLS 1.3 https://tools.ietf.org/html/rfc8446#section-4.4.2.2 */
+        GUARD(s2n_tls13_default_sig_scheme(conn, &chosen_scheme));
+    }
 
     /* SignatureScheme preference list was first added in TLS 1.2. It will be empty in older TLS versions. */
     if (peer_wire_prefs != NULL && peer_wire_prefs->len > 0) {
-        int result = s2n_choose_sig_scheme(conn, peer_wire_prefs, &chosen_scheme);
-
-        /* We require an exact match in TLS 1.3, but all previous versions can fall back to the default.
-         * The pre-TLS1.3 behavior is an intentional choice to maximize support. */
-        S2N_ERROR_IF(result != S2N_SUCCESS && conn->actual_protocol_version == S2N_TLS13,
-                S2N_ERR_INVALID_SIGNATURE_SCHEME);
-    } else {
-        S2N_ERROR_IF(conn->actual_protocol_version == S2N_TLS13, S2N_ERR_EMPTY_SIGNATURE_SCHEME);
+        /* Use a best effort approach to selecting a signature scheme matching client's preferences */
+        GUARD(s2n_choose_sig_scheme(conn, peer_wire_prefs, &chosen_scheme));
     }
 
     *sig_scheme_out = chosen_scheme;
@@ -194,7 +225,7 @@ int s2n_send_supported_sig_scheme_list(struct s2n_connection *conn, struct s2n_s
 
     GUARD(s2n_stuffer_write_uint16(out, s2n_supported_sig_scheme_list_size(conn)));
 
-    for (int i =  0; i < signature_preferences->count; i++) {
+    for (size_t i = 0; i < signature_preferences->count; i++) {
         const struct s2n_signature_scheme *const scheme = signature_preferences->signature_schemes[i];
         if (0 == s2n_signature_scheme_valid_to_offer(conn, scheme)) {
             GUARD(s2n_stuffer_write_uint16(out, scheme->iana_value));
@@ -216,7 +247,7 @@ int s2n_supported_sig_schemes_count(struct s2n_connection *conn)
     notnull_check(signature_preferences);
 
     uint8_t count = 0;
-    for (int i =  0; i < signature_preferences->count; i++) {
+    for (size_t i = 0; i < signature_preferences->count; i++) {
         if (0 == s2n_signature_scheme_valid_to_offer(conn, signature_preferences->signature_schemes[i])) {
             count ++;
         }
@@ -247,7 +278,7 @@ int s2n_recv_supported_sig_scheme_list(struct s2n_stuffer *in, struct s2n_sig_sc
     
     sig_hash_algs->len = 0;
 
-    for (int i = 0; i < pairs_available; i++) {
+    for (size_t i = 0; i < pairs_available; i++) {
         uint16_t sig_scheme = 0;
         GUARD(s2n_stuffer_read_uint16(in, &sig_scheme));
 

Original file line number	Diff line number	Diff line change
`@@ -43,6 +43,7 @@ static int destroy_server_keys(struct s2n_connection *server_conn)`
`43`	`43`	`{`
`44`	`44`	`GUARD(server_conn->initial.cipher_suite->record_alg->cipher->destroy_key(&server_conn->initial.server_key));`
`45`	`45`	`GUARD(server_conn->initial.cipher_suite->record_alg->cipher->destroy_key(&server_conn->initial.client_key));`
	`46`	`+`
`46`	`47`	`return S2N_SUCCESS;`
`47`	`48`	`}`
`48`	`49`