Remove duplicate leaf cert parse in x509 validator

goatgoose · goatgoose · commit cac7e81cd4e3 · 2023-08-31T10:15:31.000-04:00
Squashed commit of the following: commit 80e4852 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Wed Aug 30 11:11:11 2023 -0400 clang-format commit 5cb354e Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Wed Aug 30 11:04:18 2023 -0400 add test for validating empty cert chain commit 18898dc Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Wed Aug 30 10:42:39 2023 -0400 change length validate error to S2N_ERR_DECODE_CERTIFICATE commit ac44a47 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Tue Aug 29 17:04:52 2023 -0400 fixes commit 382077b Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Tue Aug 29 16:54:58 2023 -0400 s2n_asn1der_to_public_key_and_type tests commit 3a28208 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Tue Aug 29 15:54:48 2023 -0400 test for checking trailing bytes with multiple certs commit 5a45a57 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Tue Aug 29 11:32:38 2023 -0400 refactor cert parsing functions commit b3bc30a Merge: ff40e48 5d5400a Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Tue Aug 29 10:16:34 2023 -0400 Merge branch 'main' into leaf-cert-fix commit ff40e48 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Tue Aug 29 10:15:55 2023 -0400 wip commit 4e6a05a Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Fri Aug 25 22:10:22 2023 -0400 expects leaf cert parse even with tls 1.2 commit a6236ba Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Thu Aug 24 19:09:42 2023 -0400 wip
diff --git a/crypto/s2n_openssl_x509.c b/crypto/s2n_openssl_x509.c
@@ -17,6 +17,8 @@
 
 #include "api/s2n.h"
 
+#define S2N_MAX_ALLOWED_CERT_TRAILING_BYTES 3
+
 S2N_CLEANUP_RESULT s2n_openssl_x509_stack_pop_free(STACK_OF(X509) **cert_chain)
 {
     RESULT_ENSURE_REF(*cert_chain);
@@ -39,3 +41,37 @@ S2N_CLEANUP_RESULT s2n_openssl_asn1_time_free_pointer(ASN1_GENERALIZEDTIME **tim
     *time_ptr = NULL;
     return S2N_RESULT_OK;
 }
+
+S2N_RESULT s2n_openssl_x509_parse(struct s2n_blob *cert_asn1_der, X509 **cert, uint32_t *cert_len)
+{
+    RESULT_ENSURE_REF(cert_asn1_der);
+    RESULT_ENSURE_REF(cert);
+    RESULT_ENSURE_REF(cert_len);
+
+    const uint8_t *cert_data_ptr = cert_asn1_der->data;
+
+    *cert = d2i_X509(NULL, (const unsigned char **) &cert_data_ptr, cert_asn1_der->size);
+    RESULT_ENSURE(*cert, S2N_ERR_CERT_INVALID);
+
+    /* If cert parsing is successful, d2i_X509 increments *cert_data_ptr to the byte following the
+     * parsed data.
+     */
+    *cert_len = cert_data_ptr - cert_asn1_der->data;
+
+    return S2N_RESULT_OK;
+}
+
+S2N_RESULT s2n_openssl_x509_validate_length(struct s2n_blob *cert_asn1_der, uint32_t cert_len)
+{
+    RESULT_ENSURE_REF(cert_asn1_der);
+
+    RESULT_ENSURE_GTE(cert_asn1_der->size, cert_len);
+
+    /* Some asn1-encoded certificates contain extraneous trailing bytes. s2n-tls permits some
+     * number of trailing bytes for compatibility with these certificates.
+     */
+    uint32_t trailing_bytes = cert_asn1_der->size - cert_len;
+    RESULT_ENSURE(trailing_bytes <= S2N_MAX_ALLOWED_CERT_TRAILING_BYTES, S2N_ERR_DECODE_CERTIFICATE);
+
+    return S2N_RESULT_OK;
+}
diff --git a/crypto/s2n_openssl_x509.h b/crypto/s2n_openssl_x509.h
@@ -19,10 +19,14 @@
 #include <openssl/x509.h>
 #include <stdint.h>
 
+#include "utils/s2n_blob.h"
 #include "utils/s2n_safety.h"
 
 DEFINE_POINTER_CLEANUP_FUNC(X509 *, X509_free);
 
 S2N_CLEANUP_RESULT s2n_openssl_x509_stack_pop_free(STACK_OF(X509) **cert_chain);
 
 S2N_CLEANUP_RESULT s2n_openssl_asn1_time_free_pointer(ASN1_GENERALIZEDTIME **time);
+
+S2N_RESULT s2n_openssl_x509_parse(struct s2n_blob *cert_asn1_der, X509 **cert, uint32_t *cert_len);
+S2N_RESULT s2n_openssl_x509_validate_length(struct s2n_blob *cert_asn1_der, uint32_t cert_len);
diff --git a/crypto/s2n_pkey.c b/crypto/s2n_pkey.c
@@ -24,8 +24,6 @@
 #include "utils/s2n_result.h"
 #include "utils/s2n_safety.h"
 
-#define S2N_MAX_ALLOWED_CERT_TRAILING_BYTES 3
-
 int s2n_pkey_zero_init(struct s2n_pkey *pkey)
 {
     pkey->pkey = NULL;
@@ -198,59 +196,54 @@ int s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob *asn1d
 
 int s2n_asn1der_to_public_key_and_type(struct s2n_pkey *pub_key, s2n_pkey_type *pkey_type_out, struct s2n_blob *asn1der)
 {
-    uint8_t *cert_to_parse = asn1der->data;
+    POSIX_ENSURE_REF(pub_key);
+    POSIX_ENSURE_REF(pkey_type_out);
+    POSIX_ENSURE_REF(asn1der);
+
     DEFER_CLEANUP(X509 *cert = NULL, X509_free_pointer);
+    uint32_t cert_len = 0;
+    POSIX_GUARD_RESULT(s2n_openssl_x509_parse(asn1der, &cert, &cert_len));
+    POSIX_GUARD_RESULT(s2n_openssl_x509_validate_length(asn1der, cert_len));
 
-    cert = d2i_X509(NULL, (const unsigned char **) (void *) &cert_to_parse, asn1der->size);
-    S2N_ERROR_IF(cert == NULL, S2N_ERR_DECODE_CERTIFICATE);
+    POSIX_GUARD_RESULT(s2n_pkey_x509_to_public_key_and_type(cert, pub_key, pkey_type_out));
 
-    /* If cert parsing is successful, d2i_X509 increments *cert_to_parse to the byte following the parsed data */
-    uint32_t parsed_len = cert_to_parse - asn1der->data;
+    return S2N_SUCCESS;
+}
 
-    /* Some TLS clients in the wild send extra trailing bytes after the Certificate.
-     * Allow this in s2n for backwards compatibility with existing clients. */
-    uint32_t trailing_bytes = asn1der->size - parsed_len;
-    POSIX_ENSURE(trailing_bytes <= S2N_MAX_ALLOWED_CERT_TRAILING_BYTES, S2N_ERR_DECODE_CERTIFICATE);
+S2N_RESULT s2n_pkey_x509_to_public_key_and_type(X509 *cert, struct s2n_pkey *pub_key, s2n_pkey_type *pkey_type_out)
+{
+    RESULT_ENSURE_REF(cert);
+    RESULT_ENSURE_REF(pub_key);
+    RESULT_ENSURE_REF(pkey_type_out);
 
     DEFER_CLEANUP(EVP_PKEY *evp_public_key = X509_get_pubkey(cert), EVP_PKEY_free_pointer);
-    S2N_ERROR_IF(evp_public_key == NULL, S2N_ERR_DECODE_CERTIFICATE);
+    RESULT_ENSURE(evp_public_key, S2N_ERR_DECODE_CERTIFICATE);
 
     /* Check for success in decoding certificate according to type */
     int type = EVP_PKEY_base_id(evp_public_key);
 
-    int ret;
     switch (type) {
         case EVP_PKEY_RSA:
-            ret = s2n_rsa_pkey_init(pub_key);
-            if (ret != 0) {
-                break;
-            }
-            ret = s2n_evp_pkey_to_rsa_public_key(&pub_key->key.rsa_key, evp_public_key);
+            RESULT_GUARD_POSIX(s2n_rsa_pkey_init(pub_key));
+            RESULT_GUARD_POSIX(s2n_evp_pkey_to_rsa_public_key(&pub_key->key.rsa_key, evp_public_key));
             *pkey_type_out = S2N_PKEY_TYPE_RSA;
             break;
         case EVP_PKEY_RSA_PSS:
-            ret = s2n_rsa_pss_pkey_init(pub_key);
-            if (ret != 0) {
-                break;
-            }
-            ret = s2n_evp_pkey_to_rsa_pss_public_key(&pub_key->key.rsa_key, evp_public_key);
+            RESULT_GUARD_POSIX(s2n_rsa_pss_pkey_init(pub_key));
+            RESULT_GUARD_POSIX(s2n_evp_pkey_to_rsa_pss_public_key(&pub_key->key.rsa_key, evp_public_key));
             *pkey_type_out = S2N_PKEY_TYPE_RSA_PSS;
             break;
         case EVP_PKEY_EC:
-            ret = s2n_ecdsa_pkey_init(pub_key);
-            if (ret != 0) {
-                break;
-            }
-            ret = s2n_evp_pkey_to_ecdsa_public_key(&pub_key->key.ecdsa_key, evp_public_key);
+            RESULT_GUARD_POSIX(s2n_ecdsa_pkey_init(pub_key));
+            RESULT_GUARD_POSIX(s2n_evp_pkey_to_ecdsa_public_key(&pub_key->key.ecdsa_key, evp_public_key));
             *pkey_type_out = S2N_PKEY_TYPE_ECDSA;
             break;
         default:
-            POSIX_BAIL(S2N_ERR_DECODE_CERTIFICATE);
+            RESULT_BAIL(S2N_ERR_DECODE_CERTIFICATE);
     }
 
     pub_key->pkey = evp_public_key;
-    /* Reset to avoid DEFER_CLEANUP freeing our key */
-    evp_public_key = NULL;
+    ZERO_TO_DISABLE_DEFER_CLEANUP(evp_public_key);
 
-    return ret;
+    return S2N_RESULT_OK;
 }
diff --git a/crypto/s2n_pkey.h b/crypto/s2n_pkey.h
@@ -70,4 +70,5 @@ int s2n_pkey_match(const struct s2n_pkey *pub_key, const struct s2n_pkey *priv_k
 int s2n_pkey_free(struct s2n_pkey *pkey);
 
 int s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob *asn1der, int type_hint);
-int s2n_asn1der_to_public_key_and_type(struct s2n_pkey *pub_key, s2n_pkey_type *pkey_type, struct s2n_blob *asn1der);
+int s2n_asn1der_to_public_key_and_type(struct s2n_pkey *pub_key, s2n_pkey_type *pkey_type_out, struct s2n_blob *asn1der);
+S2N_RESULT s2n_pkey_x509_to_public_key_and_type(X509 *cert, struct s2n_pkey *pub_key, s2n_pkey_type *pkey_type_out);
diff --git a/tests/unit/s2n_pkey_test.c b/tests/unit/s2n_pkey_test.c
@@ -17,6 +17,8 @@
 #include "s2n_test.h"
 #include "testlib/s2n_testlib.h"
 
+S2N_RESULT s2n_x509_validator_read_asn1_cert(struct s2n_stuffer *cert_chain_in_stuffer, struct s2n_blob *asn1_cert);
+
 int main(int argc, char **argv)
 {
     BEGIN_TEST();
@@ -167,5 +169,53 @@ int main(int argc, char **argv)
         }
     };
 
+    /* s2n_asn1der_to_public_key_and_type tests */
+    {
+        /* A certificate with one trailing byte is parsed successfully */
+        {
+            uint8_t cert_chain_data[S2N_MAX_TEST_PEM_SIZE] = { 0 };
+            uint32_t cert_chain_len = 0;
+            EXPECT_SUCCESS(s2n_read_test_pem_and_len(S2N_ONE_TRAILING_BYTE_CERT_BIN, cert_chain_data, &cert_chain_len,
+                    S2N_MAX_TEST_PEM_SIZE));
+
+            struct s2n_blob cert_chain_blob = { 0 };
+            EXPECT_SUCCESS(s2n_blob_init(&cert_chain_blob, cert_chain_data, cert_chain_len));
+            DEFER_CLEANUP(struct s2n_stuffer cert_chain_stuffer = { 0 }, s2n_stuffer_free);
+            EXPECT_SUCCESS(s2n_stuffer_init_written(&cert_chain_stuffer, &cert_chain_blob));
+
+            struct s2n_blob cert_asn1_der = { 0 };
+            EXPECT_OK(s2n_x509_validator_read_asn1_cert(&cert_chain_stuffer, &cert_asn1_der));
+
+            DEFER_CLEANUP(struct s2n_pkey public_key = { 0 }, s2n_pkey_free);
+            EXPECT_SUCCESS(s2n_pkey_zero_init(&public_key));
+            s2n_pkey_type pkey_type = S2N_PKEY_TYPE_UNKNOWN;
+
+            EXPECT_SUCCESS(s2n_asn1der_to_public_key_and_type(&public_key, &pkey_type, &cert_asn1_der));
+        }
+
+        /* A certificate with too many trailing bytes errors */
+        {
+            uint8_t cert_chain_data[S2N_MAX_TEST_PEM_SIZE] = { 0 };
+            uint32_t cert_chain_len = 0;
+            EXPECT_SUCCESS(s2n_read_test_pem_and_len(S2N_FOUR_TRAILING_BYTE_CERT_BIN, cert_chain_data, &cert_chain_len,
+                    S2N_MAX_TEST_PEM_SIZE));
+
+            struct s2n_blob cert_chain_blob = { 0 };
+            EXPECT_SUCCESS(s2n_blob_init(&cert_chain_blob, cert_chain_data, cert_chain_len));
+            DEFER_CLEANUP(struct s2n_stuffer cert_chain_stuffer = { 0 }, s2n_stuffer_free);
+            EXPECT_SUCCESS(s2n_stuffer_init_written(&cert_chain_stuffer, &cert_chain_blob));
+
+            struct s2n_blob cert_asn1_der = { 0 };
+            EXPECT_OK(s2n_x509_validator_read_asn1_cert(&cert_chain_stuffer, &cert_asn1_der));
+
+            DEFER_CLEANUP(struct s2n_pkey public_key = { 0 }, s2n_pkey_free);
+            EXPECT_SUCCESS(s2n_pkey_zero_init(&public_key));
+            s2n_pkey_type pkey_type = S2N_PKEY_TYPE_UNKNOWN;
+
+            EXPECT_FAILURE_WITH_ERRNO(s2n_asn1der_to_public_key_and_type(&public_key, &pkey_type, &cert_asn1_der),
+                    S2N_ERR_DECODE_CERTIFICATE);
+        }
+    }
+
     END_TEST();
 }
diff --git a/tests/unit/s2n_x509_validator_test.c b/tests/unit/s2n_x509_validator_test.c
@@ -1812,6 +1812,47 @@ int main(int argc, char **argv)
         s2n_x509_validator_wipe(&validator);
     };
 
+    /* Ensure that certs after the leaf cert can have an arbitrary number of trailing bytes */
+    {
+        DEFER_CLEANUP(struct s2n_x509_validator validator = { 0 }, s2n_x509_validator_wipe);
+        EXPECT_SUCCESS(s2n_x509_validator_init_no_x509_validation(&validator));
+
+        DEFER_CLEANUP(struct s2n_connection *connection = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(connection);
+
+        DEFER_CLEANUP(struct s2n_stuffer one_trailing_byte_chain = { 0 }, s2n_stuffer_free);
+        EXPECT_SUCCESS(read_file(&one_trailing_byte_chain, S2N_ONE_TRAILING_BYTE_CERT_BIN, S2N_MAX_TEST_PEM_SIZE));
+        uint32_t one_trailing_byte_chain_len = s2n_stuffer_data_available(&one_trailing_byte_chain);
+        uint8_t *one_trailing_byte_chain_data = s2n_stuffer_raw_read(&one_trailing_byte_chain,
+                one_trailing_byte_chain_len);
+
+        DEFER_CLEANUP(struct s2n_stuffer four_trailing_bytes_chain = { 0 }, s2n_stuffer_free);
+        EXPECT_SUCCESS(read_file(&four_trailing_bytes_chain, S2N_FOUR_TRAILING_BYTE_CERT_BIN, S2N_MAX_TEST_PEM_SIZE));
+        uint32_t four_trailing_bytes_chain_len = s2n_stuffer_data_available(&four_trailing_bytes_chain);
+        uint8_t *four_trailing_bytes_chain_data = s2n_stuffer_raw_read(&four_trailing_bytes_chain,
+                four_trailing_bytes_chain_len);
+
+        DEFER_CLEANUP(struct s2n_stuffer chain_stuffer = { 0 }, s2n_stuffer_free);
+        EXPECT_SUCCESS(s2n_stuffer_alloc(&chain_stuffer, S2N_MAX_TEST_PEM_SIZE * 2));
+        EXPECT_SUCCESS(s2n_stuffer_write_bytes(&chain_stuffer, one_trailing_byte_chain_data,
+                one_trailing_byte_chain_len));
+        EXPECT_SUCCESS(s2n_stuffer_write_bytes(&chain_stuffer, four_trailing_bytes_chain_data,
+                four_trailing_bytes_chain_len));
+
+        uint32_t chain_len = s2n_stuffer_data_available(&chain_stuffer);
+        EXPECT_TRUE(chain_len > 0);
+        uint8_t *chain_data = s2n_stuffer_raw_read(&chain_stuffer, chain_len);
+
+        DEFER_CLEANUP(struct s2n_pkey public_key = { 0 }, s2n_pkey_free);
+        EXPECT_SUCCESS(s2n_pkey_zero_init(&public_key));
+        s2n_pkey_type pkey_type = S2N_PKEY_TYPE_UNKNOWN;
+
+        EXPECT_OK(s2n_x509_validator_validate_cert_chain(&validator, connection, chain_data, chain_len, &pkey_type,
+                &public_key));
+
+        EXPECT_EQUAL(sk_X509_num(validator.cert_chain_from_wire), 2);
+    };
+
     /* Test validator trusts a SHA-1 signature in a certificate chain if certificate validation is off */
     {
         struct s2n_x509_trust_store trust_store;
@@ -1898,6 +1939,23 @@ int main(int argc, char **argv)
         s2n_x509_trust_store_wipe(&trust_store);
     };
 
+    /* Validator fails if cert chain is empty */
+    {
+        DEFER_CLEANUP(struct s2n_x509_validator validator = { 0 }, s2n_x509_validator_wipe);
+        EXPECT_SUCCESS(s2n_x509_validator_init_no_x509_validation(&validator));
+
+        DEFER_CLEANUP(struct s2n_connection *connection = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(connection);
+
+        struct s2n_pkey public_key = { 0 };
+        EXPECT_SUCCESS(s2n_pkey_zero_init(&public_key));
+        s2n_pkey_type pkey_type = S2N_PKEY_TYPE_UNKNOWN;
+
+        EXPECT_ERROR_WITH_ERRNO(s2n_x509_validator_validate_cert_chain(&validator, connection, NULL, 0, &pkey_type,
+                                        &public_key),
+                S2N_ERR_NO_CERT_FOUND);
+    }
+
     /* Test trust store can be wiped */
     {
         /* Wipe new s2n_config, which is initialized with certs from the system default locations. */
diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c
