From 92f33010a42483a54307eef81aecafa91386715b Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Wed, 10 Jan 2024 03:43:44 +0000
Subject: [PATCH 01/13] refactor: ossl x509 parsing

This commit introduces a helper method to standardize our openssl X509
parsing, and also restructures the validator and cert loading to remove
some cert-reparsing that was occuring.
---
 crypto/s2n_certificate.c             | 47 +++++++-------
 crypto/s2n_openssl_x509.c            | 43 +++++++++++++
 crypto/s2n_openssl_x509.h            | 26 ++++++++
 crypto/s2n_pkey.c                    | 34 ++++------
 crypto/s2n_pkey.h                    |  1 +
 tests/unit/s2n_certificate_test.c    |  2 +-
 tests/unit/s2n_openssl_x509_test.c   | 96 ++++++++++++++++++++++++++++
 tests/unit/s2n_x509_validator_test.c | 58 +++++++++++++++++
 tls/s2n_x509_validator.c             | 64 ++++++++++---------
 tls/s2n_x509_validator.h             |  1 +
 10 files changed, 296 insertions(+), 76 deletions(-)
 create mode 100644 tests/unit/s2n_openssl_x509_test.c

diff --git a/crypto/s2n_certificate.c b/crypto/s2n_certificate.c
index b52e9498567..31dd9bb9eca 100644
--- a/crypto/s2n_certificate.c
+++ b/crypto/s2n_certificate.c
@@ -58,6 +58,7 @@ int s2n_create_cert_chain_from_stuffer(struct s2n_cert_chain *cert_chain_out, st
         }
         struct s2n_blob mem = { 0 };
         POSIX_GUARD(s2n_alloc(&mem, sizeof(struct s2n_cert)));
+        POSIX_GUARD(s2n_blob_zero(&mem));
         new_node = (struct s2n_cert *) (void *) mem.data;
 
         if (s2n_alloc(&new_node->raw, s2n_stuffer_data_available(&cert_out_stuffer)) != S2N_SUCCESS) {
@@ -297,7 +298,7 @@ int s2n_cert_chain_and_key_load_cns(struct s2n_cert_chain_and_key *chain_and_key
         /* We need to try and decode the CN since it may be encoded as unicode with a
          * direct ASCII equivalent. Any non ASCII bytes in the string will fail later when we
          * actually compare hostnames.
-         * 
+         *
          * `ASN1_STRING_to_UTF8` allocates in both the success case and in the zero return case, but
          * not in the failure case (negative return value). Therefore, we use `ZERO_TO_DISABLE_DEFER_CLEANUP`
          * in the failure case to prevent double-freeing `utf8_str`. For the zero and success cases, `utf8_str`
@@ -311,8 +312,8 @@ int s2n_cert_chain_and_key_load_cns(struct s2n_cert_chain_and_key *chain_and_key
             continue;
         } else if (utf8_out_len == 0) {
             /* We still need to free memory for this case, so let the DEFER_CLEANUP free it
-             * see https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-7521 and 
-             * https://security.archlinux.org/CVE-2017-7521 
+             * see https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-7521 and
+             * https://security.archlinux.org/CVE-2017-7521
             */
         } else {
             struct s2n_blob *cn_name = NULL;
@@ -334,22 +335,14 @@ int s2n_cert_chain_and_key_load_cns(struct s2n_cert_chain_and_key *chain_and_key
     return 0;
 }
 
-static int s2n_cert_chain_and_key_set_names(struct s2n_cert_chain_and_key *chain_and_key, struct s2n_blob *leaf_bytes)
+static int s2n_cert_chain_and_key_set_names(struct s2n_cert_chain_and_key *chain_and_key, X509 *cert)
 {
-    const unsigned char *leaf_der = leaf_bytes->data;
-    X509 *cert = d2i_X509(NULL, &leaf_der, leaf_bytes->size);
-    if (!cert) {
-        POSIX_BAIL(S2N_ERR_INVALID_PEM);
-    }
-
     POSIX_GUARD(s2n_cert_chain_and_key_load_sans(chain_and_key, cert));
     /* For current use cases, we *could* avoid populating the common names if any sans were loaded in
      * s2n_cert_chain_and_key_load_sans. Let's unconditionally populate this field to avoid surprises
      * in the future.
      */
     POSIX_GUARD(s2n_cert_chain_and_key_load_cns(chain_and_key, cert));
-
-    X509_free(cert);
     return 0;
 }
 
@@ -361,10 +354,14 @@ int s2n_cert_chain_and_key_load(struct s2n_cert_chain_and_key *chain_and_key)
     POSIX_ENSURE_REF(chain_and_key->private_key);
     struct s2n_cert *head = chain_and_key->cert_chain->head;
 
+    DEFER_CLEANUP(X509 *leaf_cert = NULL, X509_free_pointer);
+    POSIX_GUARD_RESULT(s2n_openssl_x509_parse_without_length_validation(&head->raw, &leaf_cert));
+
     /* Parse the leaf cert for the public key and certificate type */
     DEFER_CLEANUP(struct s2n_pkey public_key = { 0 }, s2n_pkey_free);
     s2n_pkey_type pkey_type = S2N_PKEY_TYPE_UNKNOWN;
-    POSIX_GUARD_RESULT(s2n_asn1der_to_public_key_and_type(&public_key, &pkey_type, &head->raw));
+    POSIX_GUARD_RESULT(s2n_pkey_x509_to_public_key(leaf_cert, &public_key, &pkey_type));
+
     POSIX_ENSURE(pkey_type != S2N_PKEY_TYPE_UNKNOWN, S2N_ERR_CERT_TYPE_UNSUPPORTED);
     POSIX_GUARD(s2n_cert_set_cert_type(head, pkey_type));
 
@@ -374,7 +371,7 @@ int s2n_cert_chain_and_key_load(struct s2n_cert_chain_and_key *chain_and_key)
     }
 
     /* Populate name information from the SAN/CN for the leaf certificate */
-    POSIX_GUARD(s2n_cert_chain_and_key_set_names(chain_and_key, &head->raw));
+    POSIX_GUARD(s2n_cert_chain_and_key_set_names(chain_and_key, leaf_cert));
 
     /* Populate ec curve libcrypto nid */
     if (pkey_type == S2N_PKEY_TYPE_ECDSA) {
@@ -722,7 +719,7 @@ static int s2n_utf8_string_from_extension_data(const uint8_t *extension_data, ui
     asn1_str = d2i_ASN1_UTF8STRING(NULL, (const unsigned char **) (void *) &asn1_str_data, extension_len);
     POSIX_ENSURE(asn1_str != NULL, S2N_ERR_INVALID_X509_EXTENSION_TYPE);
     /* ASN1_STRING_type() returns the type of `asn1_str`, using standard constants such as V_ASN1_OCTET_STRING.
-     * Ref: https://www.openssl.org/docs/man1.1.0/man3/ASN1_STRING_type.html. 
+     * Ref: https://www.openssl.org/docs/man1.1.0/man3/ASN1_STRING_type.html.
      */
     int type = ASN1_STRING_type(asn1_str);
     POSIX_ENSURE(type == V_ASN1_UTF8STRING, S2N_ERR_INVALID_X509_EXTENSION_TYPE);
@@ -730,7 +727,7 @@ static int s2n_utf8_string_from_extension_data(const uint8_t *extension_data, ui
     int len = ASN1_STRING_length(asn1_str);
     if (out_data != NULL) {
         POSIX_ENSURE((int64_t) *out_len >= (int64_t) len, S2N_ERR_INSUFFICIENT_MEM_SIZE);
-        /* ASN1_STRING_data() returns an internal pointer to the data. 
+        /* ASN1_STRING_data() returns an internal pointer to the data.
         * Since this is an internal pointer it should not be freed or modified in any way.
         * Ref: https://www.openssl.org/docs/man1.0.2/man3/ASN1_STRING_data.html.
         */
@@ -769,7 +766,7 @@ static int s2n_parse_x509_extension(struct s2n_cert *cert, const uint8_t *oid,
         uint8_t *ext_value, uint32_t *ext_value_len, bool *critical)
 {
     POSIX_ENSURE_REF(cert->raw.data);
-    /* Obtain the openssl x509 cert from the ASN1 DER certificate input. 
+    /* Obtain the openssl x509 cert from the ASN1 DER certificate input.
      * Note that d2i_X509 increments *der_in to the byte following the parsed data.
      * Using a temporary variable is mandatory to prevent memory free-ing errors.
      * Ref to the warning section here for more information:
@@ -780,8 +777,8 @@ static int s2n_parse_x509_extension(struct s2n_cert *cert, const uint8_t *oid,
             X509_free_pointer);
     POSIX_ENSURE_REF(x509_cert);
 
-    /* Retrieve the number of x509 extensions present in the certificate 
-     * X509_get_ext_count returns the number of extensions in the x509 certificate. 
+    /* Retrieve the number of x509 extensions present in the certificate
+     * X509_get_ext_count returns the number of extensions in the x509 certificate.
      * Ref: https://www.openssl.org/docs/man1.1.0/man3/X509_get_ext_count.html.
      */
     int ext_count_value = X509_get_ext_count(x509_cert);
@@ -790,7 +787,7 @@ static int s2n_parse_x509_extension(struct s2n_cert *cert, const uint8_t *oid,
 
     /* OBJ_txt2obj() converts the input text string into an ASN1_OBJECT structure.
      * If no_name is 0 then long names and short names will be interpreted as well as numerical forms.
-     * If no_name is 1 only the numerical form is acceptable. 
+     * If no_name is 1 only the numerical form is acceptable.
      * Ref: https://www.openssl.org/docs/man1.1.0/man3/OBJ_txt2obj.html.
      */
     DEFER_CLEANUP(ASN1_OBJECT *asn1_obj_in = OBJ_txt2obj((const char *) oid, 0), s2n_asn1_obj_free);
@@ -809,9 +806,9 @@ static int s2n_parse_x509_extension(struct s2n_cert *cert, const uint8_t *oid,
         X509_EXTENSION *x509_ext = X509_get_ext(x509_cert, loc);
         POSIX_ENSURE_REF(x509_ext);
 
-        /* Retrieve the extension object/OID/extnId. 
-         * X509_EXTENSION_get_object() returns the extension type of `x509_ext` as an ASN1_OBJECT pointer. 
-         * The returned pointer is an internal value which must not be freed up. 
+        /* Retrieve the extension object/OID/extnId.
+         * X509_EXTENSION_get_object() returns the extension type of `x509_ext` as an ASN1_OBJECT pointer.
+         * The returned pointer is an internal value which must not be freed up.
          * Ref: https://www.openssl.org/docs/man1.1.0/man3/X509_EXTENSION_get_object.html.
          */
         ASN1_OBJECT *asn1_obj = X509_EXTENSION_get_object(x509_ext);
@@ -824,7 +821,7 @@ static int s2n_parse_x509_extension(struct s2n_cert *cert, const uint8_t *oid,
 
         /* If match found, retrieve the corresponding OID value for the x509 extension */
         if (match_found) {
-            /* X509_EXTENSION_get_data() returns the data of extension `x509_ext`. 
+            /* X509_EXTENSION_get_data() returns the data of extension `x509_ext`.
              * The returned pointer is an internal value which must not be freed up.
              * Ref: https://www.openssl.org/docs/man1.1.0/man3/X509_EXTENSION_get_data.html.
              */
@@ -836,7 +833,7 @@ static int s2n_parse_x509_extension(struct s2n_cert *cert, const uint8_t *oid,
             if (ext_value != NULL) {
                 POSIX_ENSURE_GTE(len, 0);
                 POSIX_ENSURE(*ext_value_len >= (uint32_t) len, S2N_ERR_INSUFFICIENT_MEM_SIZE);
-                /* ASN1_STRING_data() returns an internal pointer to the data. 
+                /* ASN1_STRING_data() returns an internal pointer to the data.
                  * Since this is an internal pointer it should not be freed or modified in any way.
                  * Ref: https://www.openssl.org/docs/man1.0.2/man3/ASN1_STRING_data.html.
                  */
diff --git a/crypto/s2n_openssl_x509.c b/crypto/s2n_openssl_x509.c
index bc659e28a89..ff62267983d 100644
--- a/crypto/s2n_openssl_x509.c
+++ b/crypto/s2n_openssl_x509.c
@@ -39,3 +39,46 @@ 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_impl(struct s2n_blob *asn1der, X509 **cert_out, uint32_t *parsed_length)
+{
+    RESULT_ENSURE_REF(asn1der);
+    RESULT_ENSURE_REF(cert_out);
+    RESULT_ENSURE_REF(parsed_length);
+
+    uint8_t *cert_to_parse = asn1der->data;
+    *cert_out = d2i_X509(NULL, (const unsigned char **) (void *) &cert_to_parse, asn1der->size);
+    RESULT_ENSURE(*cert_out != NULL, S2N_ERR_DECODE_CERTIFICATE);
+
+    /* If cert parsing is successful, d2i_X509 increments *cert_to_parse to the byte following the parsed data */
+    *parsed_length = cert_to_parse - asn1der->data;
+
+    return S2N_RESULT_OK;
+}
+
+S2N_RESULT s2n_openssl_x509_parse_without_length_validation(struct s2n_blob *asn1der, X509 **cert_out)
+{
+    RESULT_ENSURE_REF(asn1der);
+    RESULT_ENSURE_REF(cert_out);
+
+    uint32_t parsed_len;
+    RESULT_GUARD(s2n_openssl_x509_parse_impl(asn1der, cert_out, &parsed_len));
+
+    return S2N_RESULT_OK;
+}
+
+S2N_RESULT s2n_openssl_x509_parse(struct s2n_blob *asn1der, X509 **cert_out)
+{
+    RESULT_ENSURE_REF(asn1der);
+    RESULT_ENSURE_REF(cert_out);
+
+    uint32_t parsed_len;
+    RESULT_GUARD(s2n_openssl_x509_parse_impl(asn1der, cert_out, &parsed_len));
+
+    /* 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;
+    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
index aac6d873150..7e56fc96cc4 100644
--- a/crypto/s2n_openssl_x509.h
+++ b/crypto/s2n_openssl_x509.h
@@ -19,10 +19,36 @@
 #include <openssl/x509.h>
 #include <stdint.h>
 
+#include "utils/s2n_blob.h"
 #include "utils/s2n_safety.h"
 
+#define S2N_MAX_ALLOWED_CERT_TRAILING_BYTES 3
+
 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);
+
+/*
+ * This function is used to convert an s2n_blob into an openssl X509 cert. It
+ * will additionally ensure that there are 3 or fewer trailing bytes in
+ * `asn1der`.
+ *
+ * @param asn1der the s2n_blob containing the der-encoded bytes of the x509
+ * certificate.
+ * @param cert_out the pointer where the parsed `X509*` will be stored.
+ */
+S2N_RESULT s2n_openssl_x509_parse(struct s2n_blob *asn1der, X509 **cert_out);
+
+/*
+ * This function is used to convert an s2n_blob into an openssl X509 cert.
+ * Unlike `s2n_openssl_x509_parse` no additional validation is done. This
+ * function should only be used in places where it is necessary to maintain
+ * compatability with previous permissive parsing behavior.
+ *
+ * @param asn1der the s2n_blob containing the der-encoded bytes of the x509
+ * certificate.
+ * @param cert_out the pointer where the parsed `X509*` will be stored.
+ */
+S2N_RESULT s2n_openssl_x509_parse_without_length_validation(struct s2n_blob *asn1der, X509 **cert_out);
diff --git a/crypto/s2n_pkey.c b/crypto/s2n_pkey.c
index 28671f9199c..177b0ccc044 100644
--- 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;
@@ -185,20 +183,16 @@ S2N_RESULT s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob
 S2N_RESULT 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;
     DEFER_CLEANUP(X509 *cert = NULL, X509_free_pointer);
+    RESULT_GUARD(s2n_openssl_x509_parse(asn1der, &cert));
+    RESULT_GUARD(s2n_pkey_x509_to_public_key(cert, pub_key, pkey_type_out));
 
-    cert = d2i_X509(NULL, (const unsigned char **) (void *) &cert_to_parse, asn1der->size);
-    RESULT_ENSURE(cert != NULL, S2N_ERR_DECODE_CERTIFICATE);
-
-    /* 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;
-
-    /* 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;
-    RESULT_ENSURE(trailing_bytes <= S2N_MAX_ALLOWED_CERT_TRAILING_BYTES, S2N_ERR_DECODE_CERTIFICATE);
+    return S2N_RESULT_OK;
+}
 
+S2N_RESULT s2n_pkey_x509_to_public_key(X509 *cert, struct s2n_pkey *pub_key_out,
+        s2n_pkey_type *pkey_type_out)
+{
     DEFER_CLEANUP(EVP_PKEY *evp_public_key = X509_get_pubkey(cert), EVP_PKEY_free_pointer);
     RESULT_ENSURE(evp_public_key != NULL, S2N_ERR_DECODE_CERTIFICATE);
 
@@ -206,25 +200,25 @@ S2N_RESULT s2n_asn1der_to_public_key_and_type(struct s2n_pkey *pub_key,
     int type = EVP_PKEY_base_id(evp_public_key);
     switch (type) {
         case EVP_PKEY_RSA:
-            RESULT_GUARD(s2n_rsa_pkey_init(pub_key));
-            RESULT_GUARD(s2n_evp_pkey_to_rsa_public_key(&pub_key->key.rsa_key, evp_public_key));
+            RESULT_GUARD(s2n_rsa_pkey_init(pub_key_out));
+            RESULT_GUARD(s2n_evp_pkey_to_rsa_public_key(&pub_key_out->key.rsa_key, evp_public_key));
             *pkey_type_out = S2N_PKEY_TYPE_RSA;
             break;
         case EVP_PKEY_RSA_PSS:
-            RESULT_GUARD(s2n_rsa_pss_pkey_init(pub_key));
-            RESULT_GUARD(s2n_evp_pkey_to_rsa_pss_public_key(&pub_key->key.rsa_key, evp_public_key));
+            RESULT_GUARD(s2n_rsa_pss_pkey_init(pub_key_out));
+            RESULT_GUARD(s2n_evp_pkey_to_rsa_pss_public_key(&pub_key_out->key.rsa_key, evp_public_key));
             *pkey_type_out = S2N_PKEY_TYPE_RSA_PSS;
             break;
         case EVP_PKEY_EC:
-            RESULT_GUARD(s2n_ecdsa_pkey_init(pub_key));
-            RESULT_GUARD(s2n_evp_pkey_to_ecdsa_public_key(&pub_key->key.ecdsa_key, evp_public_key));
+            RESULT_GUARD(s2n_ecdsa_pkey_init(pub_key_out));
+            RESULT_GUARD(s2n_evp_pkey_to_ecdsa_public_key(&pub_key_out->key.ecdsa_key, evp_public_key));
             *pkey_type_out = S2N_PKEY_TYPE_ECDSA;
             break;
         default:
             RESULT_BAIL(S2N_ERR_DECODE_CERTIFICATE);
     }
 
-    pub_key->pkey = evp_public_key;
+    pub_key_out->pkey = evp_public_key;
     ZERO_TO_DISABLE_DEFER_CLEANUP(evp_public_key);
 
     return S2N_RESULT_OK;
diff --git a/crypto/s2n_pkey.h b/crypto/s2n_pkey.h
index 33870a25295..609848103dc 100644
--- a/crypto/s2n_pkey.h
+++ b/crypto/s2n_pkey.h
@@ -71,3 +71,4 @@ int s2n_pkey_free(struct s2n_pkey *pkey);
 
 S2N_RESULT s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob *asn1der, int type_hint);
 S2N_RESULT s2n_asn1der_to_public_key_and_type(struct s2n_pkey *pub_key, s2n_pkey_type *pkey_type, struct s2n_blob *asn1der);
+S2N_RESULT s2n_pkey_x509_to_public_key(X509 *cert, struct s2n_pkey *pub_key_out, s2n_pkey_type *pkey_type_out);
diff --git a/tests/unit/s2n_certificate_test.c b/tests/unit/s2n_certificate_test.c
index 22fd45fc2e0..bbe9fca0cd0 100644
--- a/tests/unit/s2n_certificate_test.c
+++ b/tests/unit/s2n_certificate_test.c
@@ -903,7 +903,7 @@ int main(int argc, char **argv)
                 EXPECT_SUCCESS(s2n_stuffer_copy(&input, &server->handshake.io,
                         s2n_stuffer_data_available(&input)));
 
-                EXPECT_FAILURE_WITH_ERRNO(s2n_client_cert_recv(server), S2N_ERR_CERT_INVALID);
+                EXPECT_FAILURE_WITH_ERRNO(s2n_client_cert_recv(server), S2N_ERR_DECODE_CERTIFICATE);
                 EXPECT_NOT_EQUAL(server->handshake_params.client_cert_chain.size, 0);
                 EXPECT_NOT_NULL(server->handshake_params.client_cert_chain.data);
             }
diff --git a/tests/unit/s2n_openssl_x509_test.c b/tests/unit/s2n_openssl_x509_test.c
new file mode 100644
index 00000000000..dd43c6d42ca
--- /dev/null
+++ b/tests/unit/s2n_openssl_x509_test.c
@@ -0,0 +1,96 @@
+/*
+ * 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 "crypto/s2n_openssl_x509.h"
+
+#include "s2n_test.h"
+#include "testlib/s2n_testlib.h"
+#include "tls/s2n_x509_validator.h"
+
+static S2N_RESULT s2n_x509_validator_read_asn1_cert(struct s2n_stuffer* cert_chain_in_stuffer, struct s2n_blob* asn1_cert)
+{
+    uint32_t certificate_size = 0;
+
+    RESULT_GUARD_POSIX(s2n_stuffer_read_uint24(cert_chain_in_stuffer, &certificate_size));
+    RESULT_ENSURE(certificate_size > 0, S2N_ERR_CERT_INVALID);
+    RESULT_ENSURE(certificate_size <= s2n_stuffer_data_available(cert_chain_in_stuffer), S2N_ERR_CERT_INVALID);
+
+    asn1_cert->size = certificate_size;
+    asn1_cert->data = s2n_stuffer_raw_read(cert_chain_in_stuffer, certificate_size);
+    RESULT_ENSURE_REF(asn1_cert->data);
+
+    return S2N_RESULT_OK;
+}
+
+int main(int argc, char** argv)
+{
+    BEGIN_TEST();
+
+    /* 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(X509* cert = NULL, X509_free_pointer);
+                EXPECT_OK(s2n_openssl_x509_parse(&cert_asn1_der, &cert));
+            }
+            {
+                DEFER_CLEANUP(X509* cert = NULL, X509_free_pointer);
+                EXPECT_OK(s2n_openssl_x509_parse_without_length_validation(&cert_asn1_der, &cert));
+            }
+        }
+
+        /* 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(X509* cert = NULL, X509_free_pointer);
+                EXPECT_ERROR(s2n_openssl_x509_parse(&cert_asn1_der, &cert));
+            }
+            {
+                DEFER_CLEANUP(X509* cert = NULL, X509_free_pointer);
+                EXPECT_OK(s2n_openssl_x509_parse_without_length_validation(&cert_asn1_der, &cert));
+            }
+        }
+    }
+
+    END_TEST();
+}
diff --git a/tests/unit/s2n_x509_validator_test.c b/tests/unit/s2n_x509_validator_test.c
index 117d714ffc3..163fb5528a7 100644
--- a/tests/unit/s2n_x509_validator_test.c
+++ b/tests/unit/s2n_x509_validator_test.c
@@ -1850,6 +1850,47 @@ int main(int argc, char **argv)
                 S2N_ERR_DECODE_CERTIFICATE);
     };
 
+    /* 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;
@@ -1936,6 +1977,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
index 7403a8f2886..4849b9bb889 100644
--- a/tls/s2n_x509_validator.c
+++ b/tls/s2n_x509_validator.c
@@ -410,30 +410,33 @@ static S2N_RESULT s2n_x509_validator_read_cert_chain(struct s2n_x509_validator *
     RESULT_GUARD_POSIX(s2n_stuffer_init(&cert_chain_in_stuffer, &cert_chain_blob));
     RESULT_GUARD_POSIX(s2n_stuffer_write(&cert_chain_in_stuffer, &cert_chain_blob));
 
-    X509 *server_cert = NULL;
-
     while (s2n_stuffer_data_available(&cert_chain_in_stuffer)
             && sk_X509_num(validator->cert_chain_from_wire) < validator->max_chain_depth) {
         struct s2n_blob asn1_cert = { 0 };
         RESULT_GUARD(s2n_x509_validator_read_asn1_cert(&cert_chain_in_stuffer, &asn1_cert));
+        /* We only do the trailing byte validation whe parsing the leaf cert to
+         * match historical s2n-tls behavior.
+         */
+        DEFER_CLEANUP(X509 *server_cert = NULL, X509_free_pointer);
+        if (sk_X509_num(validator->cert_chain_from_wire) == 0) {
+            RESULT_GUARD(s2n_openssl_x509_parse(&asn1_cert, &server_cert));
+        } else {
+            RESULT_GUARD(s2n_openssl_x509_parse_without_length_validation(&asn1_cert, &server_cert));
+        }
 
-        const uint8_t *data = asn1_cert.data;
-
-        /* the cert is der encoded, just convert it. */
-        server_cert = d2i_X509(NULL, &data, asn1_cert.size);
-        RESULT_ENSURE(server_cert, S2N_ERR_CERT_INVALID);
+        if (!validator->skip_cert_validation) {
+            RESULT_ENSURE_OK(s2n_validate_certificate_signature(conn, server_cert), S2N_ERR_CERT_UNTRUSTED);
+        }
 
         /* add the cert to the chain. */
         if (!sk_X509_push(validator->cert_chain_from_wire, server_cert)) {
-            /* After the cert is added to cert_chain_from_wire, it will be freed with the call to
-             * s2n_x509_validator_wipe. If adding the cert fails, free it now instead. */
-            X509_free(server_cert);
             RESULT_BAIL(S2N_ERR_INTERNAL_LIBCRYPTO_ERROR);
         }
-
-        if (!validator->skip_cert_validation) {
-            RESULT_ENSURE_OK(s2n_validate_certificate_signature(conn, server_cert), S2N_ERR_CERT_UNTRUSTED);
-        }
+        /* After the cert is added to cert_chain_from_wire, it will be freed
+         * with the call to s2n_x509_validator_wipe. We disable the cleanup
+         * function since cleanup is no longer "owned" by server_cert.
+         */
+        ZERO_TO_DISABLE_DEFER_CLEANUP(server_cert);
 
         /* certificate extensions is a field in TLS 1.3 - https://tools.ietf.org/html/rfc8446#section-4.4.2 */
         if (conn->actual_protocol_version >= S2N_TLS13) {
@@ -632,8 +635,8 @@ static S2N_RESULT s2n_x509_validator_verify_cert_chain(struct s2n_x509_validator
     return S2N_RESULT_OK;
 }
 
-static S2N_RESULT s2n_x509_validator_read_leaf_info(struct s2n_connection *conn, uint8_t *cert_chain_in, uint32_t cert_chain_len,
-        struct s2n_pkey *public_key, s2n_pkey_type *pkey_type, s2n_parsed_extensions_list *first_certificate_extensions)
+static S2N_RESULT s2n_x509_validator_parse_leaf_certificate_extensions(struct s2n_connection *conn, uint8_t *cert_chain_in, uint32_t cert_chain_len,
+        s2n_parsed_extensions_list *first_certificate_extensions)
 {
     struct s2n_blob cert_chain_blob = { 0 };
     RESULT_GUARD_POSIX(s2n_blob_init(&cert_chain_blob, cert_chain_in, cert_chain_len));
@@ -645,15 +648,11 @@ static S2N_RESULT s2n_x509_validator_read_leaf_info(struct s2n_connection *conn,
     struct s2n_blob asn1_cert = { 0 };
     RESULT_GUARD(s2n_x509_validator_read_asn1_cert(&cert_chain_in_stuffer, &asn1_cert));
 
-    RESULT_GUARD(s2n_asn1der_to_public_key_and_type(public_key, pkey_type, &asn1_cert));
-
     /* certificate extensions is a field in TLS 1.3 - https://tools.ietf.org/html/rfc8446#section-4.4.2 */
-    if (conn->actual_protocol_version >= S2N_TLS13) {
-        s2n_parsed_extensions_list parsed_extensions_list = { 0 };
-        RESULT_GUARD_POSIX(s2n_extension_list_parse(&cert_chain_in_stuffer, &parsed_extensions_list));
-
-        *first_certificate_extensions = parsed_extensions_list;
-    }
+    RESULT_ENSURE_EQ(conn->actual_protocol_version, S2N_TLS13);
+    s2n_parsed_extensions_list parsed_extensions_list = { 0 };
+    RESULT_GUARD_POSIX(s2n_extension_list_parse(&cert_chain_in_stuffer, &parsed_extensions_list));
+    *first_certificate_extensions = parsed_extensions_list;
 
     return S2N_RESULT_OK;
 }
@@ -682,12 +681,6 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val
         RESULT_GUARD(s2n_x509_validator_verify_cert_chain(validator, conn));
     }
 
-    DEFER_CLEANUP(struct s2n_pkey public_key = { 0 }, s2n_pkey_free);
-    s2n_pkey_zero_init(&public_key);
-    s2n_parsed_extensions_list first_certificate_extensions = { 0 };
-    RESULT_GUARD(s2n_x509_validator_read_leaf_info(conn, cert_chain_in, cert_chain_len, &public_key, pkey_type,
-            &first_certificate_extensions));
-
     if (conn->actual_protocol_version >= S2N_TLS13) {
         /* Only process certificate extensions received in the first certificate. Extensions received in all other
          * certificates are ignored.
@@ -696,6 +689,8 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val
          *# If an extension applies to the entire chain, it SHOULD be included in
          *# the first CertificateEntry.
          */
+        s2n_parsed_extensions_list first_certificate_extensions = { 0 };
+        RESULT_GUARD(s2n_x509_validator_parse_leaf_certificate_extensions(conn, cert_chain_in, cert_chain_len, &first_certificate_extensions));
         RESULT_GUARD_POSIX(s2n_extension_list_process(S2N_EXTENSION_LIST_CERTIFICATE, conn, &first_certificate_extensions));
     }
 
@@ -707,6 +702,15 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val
         RESULT_ENSURE(info.accepted, S2N_ERR_CERT_REJECTED);
     }
 
+    DEFER_CLEANUP(struct s2n_pkey public_key = { 0 }, s2n_pkey_free);
+    s2n_pkey_zero_init(&public_key);
+
+    /* retrieve information from leaf cert */
+    RESULT_ENSURE_GT(sk_X509_num(validator->cert_chain_from_wire), 0);
+    X509 *leaf_cert = sk_X509_value(validator->cert_chain_from_wire, 0);
+    RESULT_ENSURE_REF(leaf_cert);
+    RESULT_GUARD(s2n_pkey_x509_to_public_key(leaf_cert, &public_key, pkey_type));
+
     *public_key_out = public_key;
 
     /* Reset the old struct, so we don't clean up public_key_out */
diff --git a/tls/s2n_x509_validator.h b/tls/s2n_x509_validator.h
index 8ca2d624d4d..4870c7540e5 100644
--- a/tls/s2n_x509_validator.h
+++ b/tls/s2n_x509_validator.h
@@ -18,6 +18,7 @@
 #include <openssl/x509v3.h>
 
 #include "api/s2n.h"
+#include "crypto/s2n_pkey.h"
 #include "tls/s2n_signature_scheme.h"
 
 /* one day, BoringSSL may add ocsp stapling support. Let's future proof this a bit by grabbing a definition

From eb6529e8c6bc6925bf40fae2e9910a2b9a80add5 Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Wed, 10 Jan 2024 14:22:30 -0800
Subject: [PATCH 02/13] Update tls/s2n_x509_validator.c

Co-authored-by: Sam Clark <3758302+goatgoose@users.noreply.github.com>
---
 tls/s2n_x509_validator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c
index 4849b9bb889..502f0bd6145 100644
--- a/tls/s2n_x509_validator.c
+++ b/tls/s2n_x509_validator.c
@@ -414,7 +414,7 @@ static S2N_RESULT s2n_x509_validator_read_cert_chain(struct s2n_x509_validator *
             && sk_X509_num(validator->cert_chain_from_wire) < validator->max_chain_depth) {
         struct s2n_blob asn1_cert = { 0 };
         RESULT_GUARD(s2n_x509_validator_read_asn1_cert(&cert_chain_in_stuffer, &asn1_cert));
-        /* We only do the trailing byte validation whe parsing the leaf cert to
+        /* We only do the trailing byte validation when parsing the leaf cert to
          * match historical s2n-tls behavior.
          */
         DEFER_CLEANUP(X509 *server_cert = NULL, X509_free_pointer);

From eda8a83f252c243985f9ae6c9c82ccb923c6227b Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Wed, 10 Jan 2024 22:44:34 +0000
Subject: [PATCH 03/13] address PR feedback

* add additional ENSURE_REFs
* switch load_cert method to parse with validation
* make asn1_cert_parse non-static and forward declare
* rename server-cert to cert
* explicitly zero-initialize variables
---
 crypto/s2n_certificate.c           |  4 +++-
 crypto/s2n_openssl_x509.c          |  5 +++--
 crypto/s2n_pkey.c                  |  4 ++++
 tests/unit/s2n_openssl_x509_test.c | 16 +---------------
 tls/s2n_x509_validator.c           | 17 +++++++++--------
 tls/s2n_x509_validator.h           |  1 -
 6 files changed, 20 insertions(+), 27 deletions(-)

diff --git a/crypto/s2n_certificate.c b/crypto/s2n_certificate.c
index 31dd9bb9eca..00e497960fa 100644
--- a/crypto/s2n_certificate.c
+++ b/crypto/s2n_certificate.c
@@ -225,6 +225,7 @@ DEFINE_POINTER_CLEANUP_FUNC(GENERAL_NAMES *, GENERAL_NAMES_free);
 int s2n_cert_chain_and_key_load_sans(struct s2n_cert_chain_and_key *chain_and_key, X509 *x509_cert)
 {
     POSIX_ENSURE_REF(chain_and_key->san_names);
+    POSIX_ENSURE_REF(x509_cert);
 
     DEFER_CLEANUP(GENERAL_NAMES *san_names = X509_get_ext_d2i(x509_cert, NID_subject_alt_name, NULL, NULL), GENERAL_NAMES_free_pointer);
     if (san_names == NULL) {
@@ -277,6 +278,7 @@ DEFINE_POINTER_CLEANUP_FUNC(unsigned char *, OPENSSL_free);
 int s2n_cert_chain_and_key_load_cns(struct s2n_cert_chain_and_key *chain_and_key, X509 *x509_cert)
 {
     POSIX_ENSURE_REF(chain_and_key->cn_names);
+    POSIX_ENSURE_REF(x509_cert);
 
     X509_NAME *subject = X509_get_subject_name(x509_cert);
     if (!subject) {
@@ -355,7 +357,7 @@ int s2n_cert_chain_and_key_load(struct s2n_cert_chain_and_key *chain_and_key)
     struct s2n_cert *head = chain_and_key->cert_chain->head;
 
     DEFER_CLEANUP(X509 *leaf_cert = NULL, X509_free_pointer);
-    POSIX_GUARD_RESULT(s2n_openssl_x509_parse_without_length_validation(&head->raw, &leaf_cert));
+    POSIX_GUARD_RESULT(s2n_openssl_x509_parse(&head->raw, &leaf_cert));
 
     /* Parse the leaf cert for the public key and certificate type */
     DEFER_CLEANUP(struct s2n_pkey public_key = { 0 }, s2n_pkey_free);
diff --git a/crypto/s2n_openssl_x509.c b/crypto/s2n_openssl_x509.c
index ff62267983d..81eb04c6ef5 100644
--- a/crypto/s2n_openssl_x509.c
+++ b/crypto/s2n_openssl_x509.c
@@ -43,6 +43,7 @@ S2N_CLEANUP_RESULT s2n_openssl_asn1_time_free_pointer(ASN1_GENERALIZEDTIME **tim
 S2N_RESULT s2n_openssl_x509_parse_impl(struct s2n_blob *asn1der, X509 **cert_out, uint32_t *parsed_length)
 {
     RESULT_ENSURE_REF(asn1der);
+    RESULT_ENSURE_REF(asn1der->data);
     RESULT_ENSURE_REF(cert_out);
     RESULT_ENSURE_REF(parsed_length);
 
@@ -61,7 +62,7 @@ S2N_RESULT s2n_openssl_x509_parse_without_length_validation(struct s2n_blob *asn
     RESULT_ENSURE_REF(asn1der);
     RESULT_ENSURE_REF(cert_out);
 
-    uint32_t parsed_len;
+    uint32_t parsed_len = 0;
     RESULT_GUARD(s2n_openssl_x509_parse_impl(asn1der, cert_out, &parsed_len));
 
     return S2N_RESULT_OK;
@@ -72,7 +73,7 @@ S2N_RESULT s2n_openssl_x509_parse(struct s2n_blob *asn1der, X509 **cert_out)
     RESULT_ENSURE_REF(asn1der);
     RESULT_ENSURE_REF(cert_out);
 
-    uint32_t parsed_len;
+    uint32_t parsed_len = 0;
     RESULT_GUARD(s2n_openssl_x509_parse_impl(asn1der, cert_out, &parsed_len));
 
     /* Some TLS clients in the wild send extra trailing bytes after the Certificate.
diff --git a/crypto/s2n_pkey.c b/crypto/s2n_pkey.c
index 177b0ccc044..97a1838a823 100644
--- a/crypto/s2n_pkey.c
+++ b/crypto/s2n_pkey.c
@@ -193,6 +193,10 @@ S2N_RESULT s2n_asn1der_to_public_key_and_type(struct s2n_pkey *pub_key,
 S2N_RESULT s2n_pkey_x509_to_public_key(X509 *cert, struct s2n_pkey *pub_key_out,
         s2n_pkey_type *pkey_type_out)
 {
+    RESULT_ENSURE_REF(cert);
+    RESULT_ENSURE_REF(pub_key_out);
+    RESULT_ENSURE_REF(pkey_type_out);
+
     DEFER_CLEANUP(EVP_PKEY *evp_public_key = X509_get_pubkey(cert), EVP_PKEY_free_pointer);
     RESULT_ENSURE(evp_public_key != NULL, S2N_ERR_DECODE_CERTIFICATE);
 
diff --git a/tests/unit/s2n_openssl_x509_test.c b/tests/unit/s2n_openssl_x509_test.c
index dd43c6d42ca..a212fc5af85 100644
--- a/tests/unit/s2n_openssl_x509_test.c
+++ b/tests/unit/s2n_openssl_x509_test.c
@@ -16,22 +16,8 @@
 
 #include "s2n_test.h"
 #include "testlib/s2n_testlib.h"
-#include "tls/s2n_x509_validator.h"
 
-static S2N_RESULT s2n_x509_validator_read_asn1_cert(struct s2n_stuffer* cert_chain_in_stuffer, struct s2n_blob* asn1_cert)
-{
-    uint32_t certificate_size = 0;
-
-    RESULT_GUARD_POSIX(s2n_stuffer_read_uint24(cert_chain_in_stuffer, &certificate_size));
-    RESULT_ENSURE(certificate_size > 0, S2N_ERR_CERT_INVALID);
-    RESULT_ENSURE(certificate_size <= s2n_stuffer_data_available(cert_chain_in_stuffer), S2N_ERR_CERT_INVALID);
-
-    asn1_cert->size = certificate_size;
-    asn1_cert->data = s2n_stuffer_raw_read(cert_chain_in_stuffer, certificate_size);
-    RESULT_ENSURE_REF(asn1_cert->data);
-
-    return S2N_RESULT_OK;
-}
+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)
 {
diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c
index 502f0bd6145..c4121744c7d 100644
--- a/tls/s2n_x509_validator.c
+++ b/tls/s2n_x509_validator.c
@@ -22,6 +22,7 @@
 #include "crypto/s2n_libcrypto.h"
 #include "crypto/s2n_openssl.h"
 #include "crypto/s2n_openssl_x509.h"
+#include "crypto/s2n_pkey.h"
 #include "tls/extensions/s2n_extension_list.h"
 #include "tls/s2n_config.h"
 #include "tls/s2n_connection.h"
@@ -382,7 +383,7 @@ static S2N_RESULT s2n_verify_host_information(struct s2n_connection *conn, X509
     return S2N_RESULT_OK;
 }
 
-static S2N_RESULT s2n_x509_validator_read_asn1_cert(struct s2n_stuffer *cert_chain_in_stuffer, struct s2n_blob *asn1_cert)
+S2N_RESULT s2n_x509_validator_read_asn1_cert(struct s2n_stuffer *cert_chain_in_stuffer, struct s2n_blob *asn1_cert)
 {
     uint32_t certificate_size = 0;
 
@@ -417,26 +418,26 @@ static S2N_RESULT s2n_x509_validator_read_cert_chain(struct s2n_x509_validator *
         /* We only do the trailing byte validation when parsing the leaf cert to
          * match historical s2n-tls behavior.
          */
-        DEFER_CLEANUP(X509 *server_cert = NULL, X509_free_pointer);
+        DEFER_CLEANUP(X509 *cert = NULL, X509_free_pointer);
         if (sk_X509_num(validator->cert_chain_from_wire) == 0) {
-            RESULT_GUARD(s2n_openssl_x509_parse(&asn1_cert, &server_cert));
+            RESULT_GUARD(s2n_openssl_x509_parse(&asn1_cert, &cert));
         } else {
-            RESULT_GUARD(s2n_openssl_x509_parse_without_length_validation(&asn1_cert, &server_cert));
+            RESULT_GUARD(s2n_openssl_x509_parse_without_length_validation(&asn1_cert, &cert));
         }
 
         if (!validator->skip_cert_validation) {
-            RESULT_ENSURE_OK(s2n_validate_certificate_signature(conn, server_cert), S2N_ERR_CERT_UNTRUSTED);
+            RESULT_ENSURE_OK(s2n_validate_certificate_signature(conn, cert), S2N_ERR_CERT_UNTRUSTED);
         }
 
         /* add the cert to the chain. */
-        if (!sk_X509_push(validator->cert_chain_from_wire, server_cert)) {
+        if (!sk_X509_push(validator->cert_chain_from_wire, cert)) {
             RESULT_BAIL(S2N_ERR_INTERNAL_LIBCRYPTO_ERROR);
         }
         /* After the cert is added to cert_chain_from_wire, it will be freed
          * with the call to s2n_x509_validator_wipe. We disable the cleanup
-         * function since cleanup is no longer "owned" by server_cert.
+         * function since cleanup is no longer "owned" by cert.
          */
-        ZERO_TO_DISABLE_DEFER_CLEANUP(server_cert);
+        ZERO_TO_DISABLE_DEFER_CLEANUP(cert);
 
         /* certificate extensions is a field in TLS 1.3 - https://tools.ietf.org/html/rfc8446#section-4.4.2 */
         if (conn->actual_protocol_version >= S2N_TLS13) {
diff --git a/tls/s2n_x509_validator.h b/tls/s2n_x509_validator.h
index 4870c7540e5..8ca2d624d4d 100644
--- a/tls/s2n_x509_validator.h
+++ b/tls/s2n_x509_validator.h
@@ -18,7 +18,6 @@
 #include <openssl/x509v3.h>
 
 #include "api/s2n.h"
-#include "crypto/s2n_pkey.h"
 #include "tls/s2n_signature_scheme.h"
 
 /* one day, BoringSSL may add ocsp stapling support. Let's future proof this a bit by grabbing a definition

From 3c624d74823e563a3fe9a0c820f8fe0e3a039364 Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Thu, 11 Jan 2024 23:23:52 +0000
Subject: [PATCH 04/13] attempted fix  for valgrind failures

---
 tests/unit/s2n_examples_test.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/tests/unit/s2n_examples_test.c b/tests/unit/s2n_examples_test.c
index 78ec10e276d..3fe076866c6 100644
--- a/tests/unit/s2n_examples_test.c
+++ b/tests/unit/s2n_examples_test.c
@@ -233,19 +233,25 @@ static S2N_RESULT s2n_run_self_talk_test(s2n_test_scenario scenario_fn)
 
     pid_t client_pid = fork();
     if (client_pid == 0) {
-        /* Suppress stdout when running the examples.
-         * This only affects the new client process.
+        /* use a new scope to force DEFER_CLEANUP functions to run before the
+         * exit system call
          */
-        fclose(stdout);
+        {
+            /* Suppress stdout when running the examples.
+             * This only affects the new client process.
+             */
+            fclose(stdout);
 
-        DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
-                s2n_connection_ptr_free);
-        EXPECT_SUCCESS(s2n_connection_set_config(client, config));
+            DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
+                    s2n_connection_ptr_free);
+            EXPECT_SUCCESS(s2n_connection_set_config(client, config));
 
-        EXPECT_SUCCESS(s2n_io_pair_close_one_end(&io_pair, S2N_SERVER));
-        EXPECT_SUCCESS(s2n_connection_set_io_pair(client, &io_pair));
+            EXPECT_SUCCESS(s2n_io_pair_close_one_end(&io_pair, S2N_SERVER));
+            EXPECT_SUCCESS(s2n_connection_set_io_pair(client, &io_pair));
+
+            EXPECT_OK(scenario_fn(client, &input));
+        }
 
-        EXPECT_OK(scenario_fn(client, &input));
 
         exit(EXIT_SUCCESS);
     }

From 53e008e61c6468218e0727a203e644b4f56d5b62 Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Tue, 16 Jan 2024 23:06:47 +0000
Subject: [PATCH 05/13] address PR feedback

- remove unnecessary test header in openssl_x509_tests
- RESULT_GUARD for s2n_pkey_zero_init
- move version check to start of method
- initialize public-key along with other structs
- remove unnecessary pkey_zero_init
- add extra line for trailing byte validation
- use RESULT_GUARD_OSSL instead of manual check
- rename s2n_pkey_x509... method
---
 crypto/s2n_certificate.c           |  2 +-
 crypto/s2n_pkey.c                  |  4 +-
 crypto/s2n_pkey.h                  |  2 +-
 tests/unit/s2n_examples_test.c     |  1 -
 tests/unit/s2n_openssl_x509_test.c | 79 ++++++++++++++----------------
 tls/s2n_x509_validator.c           | 20 ++++----
 6 files changed, 52 insertions(+), 56 deletions(-)

diff --git a/crypto/s2n_certificate.c b/crypto/s2n_certificate.c
index 00e497960fa..71650a62638 100644
--- a/crypto/s2n_certificate.c
+++ b/crypto/s2n_certificate.c
@@ -362,7 +362,7 @@ int s2n_cert_chain_and_key_load(struct s2n_cert_chain_and_key *chain_and_key)
     /* Parse the leaf cert for the public key and certificate type */
     DEFER_CLEANUP(struct s2n_pkey public_key = { 0 }, s2n_pkey_free);
     s2n_pkey_type pkey_type = S2N_PKEY_TYPE_UNKNOWN;
-    POSIX_GUARD_RESULT(s2n_pkey_x509_to_public_key(leaf_cert, &public_key, &pkey_type));
+    POSIX_GUARD_RESULT(s2n_pkey_from_x509(leaf_cert, &public_key, &pkey_type));
 
     POSIX_ENSURE(pkey_type != S2N_PKEY_TYPE_UNKNOWN, S2N_ERR_CERT_TYPE_UNSUPPORTED);
     POSIX_GUARD(s2n_cert_set_cert_type(head, pkey_type));
diff --git a/crypto/s2n_pkey.c b/crypto/s2n_pkey.c
index 97a1838a823..190c73e303a 100644
--- a/crypto/s2n_pkey.c
+++ b/crypto/s2n_pkey.c
@@ -185,12 +185,12 @@ S2N_RESULT s2n_asn1der_to_public_key_and_type(struct s2n_pkey *pub_key,
 {
     DEFER_CLEANUP(X509 *cert = NULL, X509_free_pointer);
     RESULT_GUARD(s2n_openssl_x509_parse(asn1der, &cert));
-    RESULT_GUARD(s2n_pkey_x509_to_public_key(cert, pub_key, pkey_type_out));
+    RESULT_GUARD(s2n_pkey_from_x509(cert, pub_key, pkey_type_out));
 
     return S2N_RESULT_OK;
 }
 
-S2N_RESULT s2n_pkey_x509_to_public_key(X509 *cert, struct s2n_pkey *pub_key_out,
+S2N_RESULT s2n_pkey_from_x509(X509 *cert, struct s2n_pkey *pub_key_out,
         s2n_pkey_type *pkey_type_out)
 {
     RESULT_ENSURE_REF(cert);
diff --git a/crypto/s2n_pkey.h b/crypto/s2n_pkey.h
index 609848103dc..da8bcd1cadf 100644
--- a/crypto/s2n_pkey.h
+++ b/crypto/s2n_pkey.h
@@ -71,4 +71,4 @@ int s2n_pkey_free(struct s2n_pkey *pkey);
 
 S2N_RESULT s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob *asn1der, int type_hint);
 S2N_RESULT s2n_asn1der_to_public_key_and_type(struct s2n_pkey *pub_key, s2n_pkey_type *pkey_type, struct s2n_blob *asn1der);
-S2N_RESULT s2n_pkey_x509_to_public_key(X509 *cert, struct s2n_pkey *pub_key_out, s2n_pkey_type *pkey_type_out);
+S2N_RESULT s2n_pkey_from_x509(X509 *cert, struct s2n_pkey *pub_key_out, s2n_pkey_type *pkey_type_out);
diff --git a/tests/unit/s2n_examples_test.c b/tests/unit/s2n_examples_test.c
index 3fe076866c6..c3a36b58a65 100644
--- a/tests/unit/s2n_examples_test.c
+++ b/tests/unit/s2n_examples_test.c
@@ -252,7 +252,6 @@ static S2N_RESULT s2n_run_self_talk_test(s2n_test_scenario scenario_fn)
             EXPECT_OK(scenario_fn(client, &input));
         }
 
-
         exit(EXIT_SUCCESS);
     }
 
diff --git a/tests/unit/s2n_openssl_x509_test.c b/tests/unit/s2n_openssl_x509_test.c
index a212fc5af85..e40f6ef05bb 100644
--- a/tests/unit/s2n_openssl_x509_test.c
+++ b/tests/unit/s2n_openssl_x509_test.c
@@ -23,58 +23,55 @@ int main(int argc, char** argv)
 {
     BEGIN_TEST();
 
-    /* s2n_asn1der_to_public_key_and_type tests */
+    /* A certificate with one trailing byte is parsed successfully */
     {
-        /* 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));
+        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));
+        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));
+        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));
+        struct s2n_blob cert_asn1_der = { 0 };
+        EXPECT_OK(s2n_x509_validator_read_asn1_cert(&cert_chain_stuffer, &cert_asn1_der));
 
-            {
-                DEFER_CLEANUP(X509* cert = NULL, X509_free_pointer);
-                EXPECT_OK(s2n_openssl_x509_parse(&cert_asn1_der, &cert));
-            }
-            {
-                DEFER_CLEANUP(X509* cert = NULL, X509_free_pointer);
-                EXPECT_OK(s2n_openssl_x509_parse_without_length_validation(&cert_asn1_der, &cert));
-            }
+        {
+            DEFER_CLEANUP(X509* cert = NULL, X509_free_pointer);
+            EXPECT_OK(s2n_openssl_x509_parse(&cert_asn1_der, &cert));
         }
-
-        /* 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));
+            DEFER_CLEANUP(X509* cert = NULL, X509_free_pointer);
+            EXPECT_OK(s2n_openssl_x509_parse_without_length_validation(&cert_asn1_der, &cert));
+        }
+    }
 
-            struct s2n_blob cert_chain_blob = { 0 };
-            EXPECT_SUCCESS(s2n_blob_init(&cert_chain_blob, cert_chain_data, cert_chain_len));
+    /* 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));
+        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));
+        struct s2n_blob cert_asn1_der = { 0 };
+        EXPECT_OK(s2n_x509_validator_read_asn1_cert(&cert_chain_stuffer, &cert_asn1_der));
 
-            {
-                DEFER_CLEANUP(X509* cert = NULL, X509_free_pointer);
-                EXPECT_ERROR(s2n_openssl_x509_parse(&cert_asn1_der, &cert));
-            }
-            {
-                DEFER_CLEANUP(X509* cert = NULL, X509_free_pointer);
-                EXPECT_OK(s2n_openssl_x509_parse_without_length_validation(&cert_asn1_der, &cert));
-            }
+        {
+            DEFER_CLEANUP(X509* cert = NULL, X509_free_pointer);
+            EXPECT_ERROR(s2n_openssl_x509_parse(&cert_asn1_der, &cert));
+        }
+        {
+            DEFER_CLEANUP(X509* cert = NULL, X509_free_pointer);
+            EXPECT_OK(s2n_openssl_x509_parse_without_length_validation(&cert_asn1_der, &cert));
         }
     }
 
diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c
index c4121744c7d..42d1366f106 100644
--- a/tls/s2n_x509_validator.c
+++ b/tls/s2n_x509_validator.c
@@ -415,6 +415,7 @@ static S2N_RESULT s2n_x509_validator_read_cert_chain(struct s2n_x509_validator *
             && sk_X509_num(validator->cert_chain_from_wire) < validator->max_chain_depth) {
         struct s2n_blob asn1_cert = { 0 };
         RESULT_GUARD(s2n_x509_validator_read_asn1_cert(&cert_chain_in_stuffer, &asn1_cert));
+
         /* We only do the trailing byte validation when parsing the leaf cert to
          * match historical s2n-tls behavior.
          */
@@ -429,10 +430,9 @@ static S2N_RESULT s2n_x509_validator_read_cert_chain(struct s2n_x509_validator *
             RESULT_ENSURE_OK(s2n_validate_certificate_signature(conn, cert), S2N_ERR_CERT_UNTRUSTED);
         }
 
-        /* add the cert to the chain. */
-        if (!sk_X509_push(validator->cert_chain_from_wire, cert)) {
-            RESULT_BAIL(S2N_ERR_INTERNAL_LIBCRYPTO_ERROR);
-        }
+        /* add the cert to the chain */
+        RESULT_ENSURE(sk_X509_push(validator->cert_chain_from_wire, cert) > 0, S2N_ERR_INTERNAL_LIBCRYPTO_ERROR);
+
         /* After the cert is added to cert_chain_from_wire, it will be freed
          * with the call to s2n_x509_validator_wipe. We disable the cleanup
          * function since cleanup is no longer "owned" by cert.
@@ -639,6 +639,9 @@ static S2N_RESULT s2n_x509_validator_verify_cert_chain(struct s2n_x509_validator
 static S2N_RESULT s2n_x509_validator_parse_leaf_certificate_extensions(struct s2n_connection *conn, uint8_t *cert_chain_in, uint32_t cert_chain_len,
         s2n_parsed_extensions_list *first_certificate_extensions)
 {
+    /* certificate extensions is a field in TLS 1.3 - https://tools.ietf.org/html/rfc8446#section-4.4.2 */
+    RESULT_ENSURE_EQ(conn->actual_protocol_version, S2N_TLS13);
+
     struct s2n_blob cert_chain_blob = { 0 };
     RESULT_GUARD_POSIX(s2n_blob_init(&cert_chain_blob, cert_chain_in, cert_chain_len));
     DEFER_CLEANUP(struct s2n_stuffer cert_chain_in_stuffer = { 0 }, s2n_stuffer_free);
@@ -649,8 +652,6 @@ static S2N_RESULT s2n_x509_validator_parse_leaf_certificate_extensions(struct s2
     struct s2n_blob asn1_cert = { 0 };
     RESULT_GUARD(s2n_x509_validator_read_asn1_cert(&cert_chain_in_stuffer, &asn1_cert));
 
-    /* certificate extensions is a field in TLS 1.3 - https://tools.ietf.org/html/rfc8446#section-4.4.2 */
-    RESULT_ENSURE_EQ(conn->actual_protocol_version, S2N_TLS13);
     s2n_parsed_extensions_list parsed_extensions_list = { 0 };
     RESULT_GUARD_POSIX(s2n_extension_list_parse(&cert_chain_in_stuffer, &parsed_extensions_list));
     *first_certificate_extensions = parsed_extensions_list;
@@ -703,19 +704,18 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val
         RESULT_ENSURE(info.accepted, S2N_ERR_CERT_REJECTED);
     }
 
-    DEFER_CLEANUP(struct s2n_pkey public_key = { 0 }, s2n_pkey_free);
-    s2n_pkey_zero_init(&public_key);
 
     /* retrieve information from leaf cert */
     RESULT_ENSURE_GT(sk_X509_num(validator->cert_chain_from_wire), 0);
     X509 *leaf_cert = sk_X509_value(validator->cert_chain_from_wire, 0);
     RESULT_ENSURE_REF(leaf_cert);
-    RESULT_GUARD(s2n_pkey_x509_to_public_key(leaf_cert, &public_key, pkey_type));
+    DEFER_CLEANUP(struct s2n_pkey public_key = { 0 }, s2n_pkey_free);
+    RESULT_GUARD(s2n_pkey_from_x509(leaf_cert, &public_key, pkey_type));
 
     *public_key_out = public_key;
 
     /* Reset the old struct, so we don't clean up public_key_out */
-    s2n_pkey_zero_init(&public_key);
+    RESULT_GUARD_POSIX(s2n_pkey_zero_init(&public_key));
 
     return S2N_RESULT_OK;
 }

From 33098d0ad4e35cdfc897d7f05ca73c306b5eccdc Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Tue, 16 Jan 2024 23:27:18 +0000
Subject: [PATCH 06/13] address pr feedback

- remove param information from internal comments
---
 crypto/s2n_openssl_x509.h | 8 --------
 tls/s2n_x509_validator.c  | 1 -
 2 files changed, 9 deletions(-)

diff --git a/crypto/s2n_openssl_x509.h b/crypto/s2n_openssl_x509.h
index 7e56fc96cc4..33b78453277 100644
--- a/crypto/s2n_openssl_x509.h
+++ b/crypto/s2n_openssl_x509.h
@@ -34,10 +34,6 @@ S2N_CLEANUP_RESULT s2n_openssl_asn1_time_free_pointer(ASN1_GENERALIZEDTIME **tim
  * This function is used to convert an s2n_blob into an openssl X509 cert. It
  * will additionally ensure that there are 3 or fewer trailing bytes in
  * `asn1der`.
- *
- * @param asn1der the s2n_blob containing the der-encoded bytes of the x509
- * certificate.
- * @param cert_out the pointer where the parsed `X509*` will be stored.
  */
 S2N_RESULT s2n_openssl_x509_parse(struct s2n_blob *asn1der, X509 **cert_out);
 
@@ -46,9 +42,5 @@ S2N_RESULT s2n_openssl_x509_parse(struct s2n_blob *asn1der, X509 **cert_out);
  * Unlike `s2n_openssl_x509_parse` no additional validation is done. This
  * function should only be used in places where it is necessary to maintain
  * compatability with previous permissive parsing behavior.
- *
- * @param asn1der the s2n_blob containing the der-encoded bytes of the x509
- * certificate.
- * @param cert_out the pointer where the parsed `X509*` will be stored.
  */
 S2N_RESULT s2n_openssl_x509_parse_without_length_validation(struct s2n_blob *asn1der, X509 **cert_out);
diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c
index 42d1366f106..1c66de1a9e9 100644
--- a/tls/s2n_x509_validator.c
+++ b/tls/s2n_x509_validator.c
@@ -704,7 +704,6 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val
         RESULT_ENSURE(info.accepted, S2N_ERR_CERT_REJECTED);
     }
 
-
     /* retrieve information from leaf cert */
     RESULT_ENSURE_GT(sk_X509_num(validator->cert_chain_from_wire), 0);
     X509 *leaf_cert = sk_X509_value(validator->cert_chain_from_wire, 0);

From ede159490e3b4e2524e4ed3e80ae370efb53b0c6 Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Tue, 16 Jan 2024 23:33:06 +0000
Subject: [PATCH 07/13] address PR feedback

- manually format function definition

I really thought that clang format would handle this for me :'(
---
 tls/s2n_x509_validator.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c
index 1c66de1a9e9..8753963952b 100644
--- a/tls/s2n_x509_validator.c
+++ b/tls/s2n_x509_validator.c
@@ -636,7 +636,8 @@ static S2N_RESULT s2n_x509_validator_verify_cert_chain(struct s2n_x509_validator
     return S2N_RESULT_OK;
 }
 
-static S2N_RESULT s2n_x509_validator_parse_leaf_certificate_extensions(struct s2n_connection *conn, uint8_t *cert_chain_in, uint32_t cert_chain_len,
+static S2N_RESULT s2n_x509_validator_parse_leaf_certificate_extensions(
+        struct s2n_connection *conn, uint8_t *cert_chain_in, uint32_t cert_chain_len,
         s2n_parsed_extensions_list *first_certificate_extensions)
 {
     /* certificate extensions is a field in TLS 1.3 - https://tools.ietf.org/html/rfc8446#section-4.4.2 */

From 3b3e96c08c00d82590cb1e4be8e1b7abcb2f9b9b Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Fri, 19 Jan 2024 00:35:24 +0000
Subject: [PATCH 08/13] add explicit free

---
 tests/unit/s2n_examples_test.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/tests/unit/s2n_examples_test.c b/tests/unit/s2n_examples_test.c
index c3a36b58a65..8b835757a6b 100644
--- a/tests/unit/s2n_examples_test.c
+++ b/tests/unit/s2n_examples_test.c
@@ -233,24 +233,20 @@ static S2N_RESULT s2n_run_self_talk_test(s2n_test_scenario scenario_fn)
 
     pid_t client_pid = fork();
     if (client_pid == 0) {
-        /* use a new scope to force DEFER_CLEANUP functions to run before the
-         * exit system call
+        /* Suppress stdout when running the examples.
+         * This only affects the new client process.
          */
-        {
-            /* Suppress stdout when running the examples.
-             * This only affects the new client process.
-             */
-            fclose(stdout);
+        fclose(stdout);
 
-            DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
-                    s2n_connection_ptr_free);
-            EXPECT_SUCCESS(s2n_connection_set_config(client, config));
+        struct s2n_connection *client = s2n_connection_new(S2N_CLIENT);
+        EXPECT_SUCCESS(s2n_connection_set_config(client, config));
 
-            EXPECT_SUCCESS(s2n_io_pair_close_one_end(&io_pair, S2N_SERVER));
-            EXPECT_SUCCESS(s2n_connection_set_io_pair(client, &io_pair));
+        EXPECT_SUCCESS(s2n_io_pair_close_one_end(&io_pair, S2N_SERVER));
+        EXPECT_SUCCESS(s2n_connection_set_io_pair(client, &io_pair));
 
-            EXPECT_OK(scenario_fn(client, &input));
-        }
+        EXPECT_OK(scenario_fn(client, &input));
+
+        EXPECT_SUCCESS(s2n_connection_free(client));
 
         exit(EXIT_SUCCESS);
     }
@@ -262,8 +258,7 @@ static S2N_RESULT s2n_run_self_talk_test(s2n_test_scenario scenario_fn)
          */
         fclose(stdout);
 
-        DEFER_CLEANUP(struct s2n_connection *server = s2n_connection_new(S2N_SERVER),
-                s2n_connection_ptr_free);
+        struct s2n_connection *server = s2n_connection_new(S2N_SERVER);
         EXPECT_SUCCESS(s2n_connection_set_config(server, config));
 
         EXPECT_SUCCESS(s2n_io_pair_close_one_end(&io_pair, S2N_CLIENT));
@@ -271,6 +266,8 @@ static S2N_RESULT s2n_run_self_talk_test(s2n_test_scenario scenario_fn)
 
         EXPECT_OK(scenario_fn(server, &input));
 
+        EXPECT_SUCCESS(s2n_connection_free(server));
+
         exit(EXIT_SUCCESS);
     }
 

From 28eb545edddca8154b2b973207e1afe940837c69 Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Fri, 19 Jan 2024 23:01:27 +0000
Subject: [PATCH 09/13] run git clang-format

---
 crypto/s2n_pkey.h                  |  3 ++-
 tests/unit/s2n_openssl_x509_test.c | 11 ++++++-----
 tls/s2n_x509_validator.c           | 13 ++++++++-----
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/crypto/s2n_pkey.h b/crypto/s2n_pkey.h
index da8bcd1cadf..c00ad3d846d 100644
--- a/crypto/s2n_pkey.h
+++ b/crypto/s2n_pkey.h
@@ -71,4 +71,5 @@ int s2n_pkey_free(struct s2n_pkey *pkey);
 
 S2N_RESULT s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob *asn1der, int type_hint);
 S2N_RESULT s2n_asn1der_to_public_key_and_type(struct s2n_pkey *pub_key, s2n_pkey_type *pkey_type, struct s2n_blob *asn1der);
-S2N_RESULT s2n_pkey_from_x509(X509 *cert, struct s2n_pkey *pub_key_out, s2n_pkey_type *pkey_type_out);
+S2N_RESULT s2n_pkey_from_x509(X509 *cert, struct s2n_pkey *pub_key_out,
+        s2n_pkey_type *pkey_type_out);
diff --git a/tests/unit/s2n_openssl_x509_test.c b/tests/unit/s2n_openssl_x509_test.c
index e40f6ef05bb..4c6354a460a 100644
--- a/tests/unit/s2n_openssl_x509_test.c
+++ b/tests/unit/s2n_openssl_x509_test.c
@@ -17,7 +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);
+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)
 {
@@ -27,8 +28,8 @@ int main(int argc, char** argv)
     {
         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));
+        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));
@@ -53,8 +54,8 @@ int main(int argc, char** argv)
     {
         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));
+        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));
diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c
index 8753963952b..b237ce1f1b0 100644
--- a/tls/s2n_x509_validator.c
+++ b/tls/s2n_x509_validator.c
@@ -383,7 +383,8 @@ static S2N_RESULT s2n_verify_host_information(struct s2n_connection *conn, X509
     return S2N_RESULT_OK;
 }
 
-S2N_RESULT s2n_x509_validator_read_asn1_cert(struct s2n_stuffer *cert_chain_in_stuffer, struct s2n_blob *asn1_cert)
+S2N_RESULT s2n_x509_validator_read_asn1_cert(struct s2n_stuffer *cert_chain_in_stuffer,
+        struct s2n_blob *asn1_cert)
 {
     uint32_t certificate_size = 0;
 
@@ -427,11 +428,13 @@ static S2N_RESULT s2n_x509_validator_read_cert_chain(struct s2n_x509_validator *
         }
 
         if (!validator->skip_cert_validation) {
-            RESULT_ENSURE_OK(s2n_validate_certificate_signature(conn, cert), S2N_ERR_CERT_UNTRUSTED);
+            RESULT_ENSURE_OK(s2n_validate_certificate_signature(conn, cert),
+                    S2N_ERR_CERT_UNTRUSTED);
         }
 
         /* add the cert to the chain */
-        RESULT_ENSURE(sk_X509_push(validator->cert_chain_from_wire, cert) > 0, S2N_ERR_INTERNAL_LIBCRYPTO_ERROR);
+        RESULT_ENSURE(sk_X509_push(validator->cert_chain_from_wire, cert) > 0,
+                S2N_ERR_INTERNAL_LIBCRYPTO_ERROR);
 
         /* After the cert is added to cert_chain_from_wire, it will be freed
          * with the call to s2n_x509_validator_wipe. We disable the cleanup
@@ -636,8 +639,8 @@ static S2N_RESULT s2n_x509_validator_verify_cert_chain(struct s2n_x509_validator
     return S2N_RESULT_OK;
 }
 
-static S2N_RESULT s2n_x509_validator_parse_leaf_certificate_extensions(
-        struct s2n_connection *conn, uint8_t *cert_chain_in, uint32_t cert_chain_len,
+static S2N_RESULT s2n_x509_validator_parse_leaf_certificate_extensions(struct s2n_connection *conn,
+        uint8_t *cert_chain_in, uint32_t cert_chain_len,
         s2n_parsed_extensions_list *first_certificate_extensions)
 {
     /* certificate extensions is a field in TLS 1.3 - https://tools.ietf.org/html/rfc8446#section-4.4.2 */

From 1a99012fc9f840be7826a107d4ae4ddf4e32d766 Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Fri, 19 Jan 2024 23:05:53 +0000
Subject: [PATCH 10/13] address pr feedback

- use gte rather than equal for 1.3 check
- use explicit ZERO_TO_DISABLE_DEFER_CLEANUP method
---
 tls/s2n_x509_validator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tls/s2n_x509_validator.c b/tls/s2n_x509_validator.c
index b237ce1f1b0..907cc445993 100644
--- a/tls/s2n_x509_validator.c
+++ b/tls/s2n_x509_validator.c
@@ -644,7 +644,7 @@ static S2N_RESULT s2n_x509_validator_parse_leaf_certificate_extensions(struct s2
         s2n_parsed_extensions_list *first_certificate_extensions)
 {
     /* certificate extensions is a field in TLS 1.3 - https://tools.ietf.org/html/rfc8446#section-4.4.2 */
-    RESULT_ENSURE_EQ(conn->actual_protocol_version, S2N_TLS13);
+    RESULT_ENSURE_GTE(conn->actual_protocol_version, S2N_TLS13);
 
     struct s2n_blob cert_chain_blob = { 0 };
     RESULT_GUARD_POSIX(s2n_blob_init(&cert_chain_blob, cert_chain_in, cert_chain_len));
@@ -718,7 +718,7 @@ S2N_RESULT s2n_x509_validator_validate_cert_chain(struct s2n_x509_validator *val
     *public_key_out = public_key;
 
     /* Reset the old struct, so we don't clean up public_key_out */
-    RESULT_GUARD_POSIX(s2n_pkey_zero_init(&public_key));
+    ZERO_TO_DISABLE_DEFER_CLEANUP(public_key);
 
     return S2N_RESULT_OK;
 }

From 4e489c47330484fdd5a0decda57b415ab7314f90 Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Sat, 20 Jan 2024 02:13:58 +0000
Subject: [PATCH 11/13] add readable s2n_mem logs

---
 tests/unit/s2n_mem_usage_test.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/tests/unit/s2n_mem_usage_test.c b/tests/unit/s2n_mem_usage_test.c
index 1180c655889..b5dd444b3ba 100644
--- a/tests/unit/s2n_mem_usage_test.c
+++ b/tests/unit/s2n_mem_usage_test.c
@@ -254,14 +254,14 @@ int main(int argc, char **argv)
     free(clients);
     free(servers);
 
-    TEST_DEBUG_PRINT("\n");
-    TEST_DEBUG_PRINT("VmData initial:              %10zd\n", vm_data_initial);
-    TEST_DEBUG_PRINT("VmData after allocations:    %10zd\n", vm_data_after_allocation);
-    TEST_DEBUG_PRINT("VmData after handshakes:     %10zd\n", vm_data_after_handshakes);
-    TEST_DEBUG_PRINT("VmData after free handshake: %10zd\n", vm_data_after_free_handshake);
-    TEST_DEBUG_PRINT("VmData after release:        %10zd\n", vm_data_after_release_buffers);
-    TEST_DEBUG_PRINT("Max VmData diff allowed:     %10zd\n", maxAllowedMemDiff);
-    TEST_DEBUG_PRINT("Number of connections used:  %10zu\n", connectionsToUse);
+    fprintf(stdout, "\n");
+    fprintf(stdout, "VmData initial:              %10zd\n", vm_data_initial);
+    fprintf(stdout, "VmData after allocations:    %10zd\n", vm_data_after_allocation);
+    fprintf(stdout, "VmData after handshakes:     %10zd\n", vm_data_after_handshakes);
+    fprintf(stdout, "VmData after free handshake: %10zd\n", vm_data_after_free_handshake);
+    fprintf(stdout, "VmData after release:        %10zd\n", vm_data_after_release_buffers);
+    fprintf(stdout, "Max VmData diff allowed:     %10zd\n", maxAllowedMemDiff);
+    fprintf(stdout, "Number of connections used:  %10zu\n", connectionsToUse);
 
     EXPECT_TRUE(vm_data_after_free_handshake <= vm_data_after_handshakes);
     EXPECT_TRUE(vm_data_after_release_buffers <= vm_data_after_free_handshake);
@@ -269,6 +269,10 @@ int main(int argc, char **argv)
     ssize_t handshake_diff = (vm_data_after_handshakes - vm_data_initial);
     ssize_t allocation_diff = (vm_data_after_allocation - vm_data_initial);
 
+    fprintf(stdout, "handshake diff:      %10zd\n", handshake_diff);
+    fprintf(stdout, "allocation diff:     %10zd\n", allocation_diff);
+    fprintf(stdout, "max allowed diff:    %10zd\n", maxAllowedMemDiff);
+
     /*
      * get_vm_data_size is required for this test to succeed.
      * Any platform that doesn't implement get_vm_data_size should be excluded here.

From 2a8b988ef17a6c59ffeb64b90664e9737c7630c6 Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Sat, 20 Jan 2024 20:32:27 +0000
Subject: [PATCH 12/13] adjust openbsd mem

---
 tests/unit/s2n_mem_usage_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/unit/s2n_mem_usage_test.c b/tests/unit/s2n_mem_usage_test.c
index b5dd444b3ba..cdfa2e5772e 100644
--- a/tests/unit/s2n_mem_usage_test.c
+++ b/tests/unit/s2n_mem_usage_test.c
@@ -54,7 +54,7 @@
 #ifdef __FreeBSD__
     #define MEM_PER_CONNECTION 57
 #elif defined(__OpenBSD__)
-    #define MEM_PER_CONNECTION 60
+    #define MEM_PER_CONNECTION 61
 #else
     #define MEM_PER_CONNECTION 49
 #endif

From 02529ef7e66c424853703ff2b08114805a352a0e Mon Sep 17 00:00:00 2001
From: James Mayclin <maycj@amazon.com>
Date: Mon, 22 Jan 2024 20:45:22 +0000
Subject: [PATCH 13/13] address PR feedback

- the given preference was to address the TEST_DEBUG_PRINT issues
  systematically rather than in this specific instance
---
 tests/unit/s2n_mem_usage_test.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/tests/unit/s2n_mem_usage_test.c b/tests/unit/s2n_mem_usage_test.c
index cdfa2e5772e..765c7e634cd 100644
--- a/tests/unit/s2n_mem_usage_test.c
+++ b/tests/unit/s2n_mem_usage_test.c
@@ -254,14 +254,14 @@ int main(int argc, char **argv)
     free(clients);
     free(servers);
 
-    fprintf(stdout, "\n");
-    fprintf(stdout, "VmData initial:              %10zd\n", vm_data_initial);
-    fprintf(stdout, "VmData after allocations:    %10zd\n", vm_data_after_allocation);
-    fprintf(stdout, "VmData after handshakes:     %10zd\n", vm_data_after_handshakes);
-    fprintf(stdout, "VmData after free handshake: %10zd\n", vm_data_after_free_handshake);
-    fprintf(stdout, "VmData after release:        %10zd\n", vm_data_after_release_buffers);
-    fprintf(stdout, "Max VmData diff allowed:     %10zd\n", maxAllowedMemDiff);
-    fprintf(stdout, "Number of connections used:  %10zu\n", connectionsToUse);
+    TEST_DEBUG_PRINT("\n");
+    TEST_DEBUG_PRINT("VmData initial:              %10zd\n", vm_data_initial);
+    TEST_DEBUG_PRINT("VmData after allocations:    %10zd\n", vm_data_after_allocation);
+    TEST_DEBUG_PRINT("VmData after handshakes:     %10zd\n", vm_data_after_handshakes);
+    TEST_DEBUG_PRINT("VmData after free handshake: %10zd\n", vm_data_after_free_handshake);
+    TEST_DEBUG_PRINT("VmData after release:        %10zd\n", vm_data_after_release_buffers);
+    TEST_DEBUG_PRINT("Max VmData diff allowed:     %10zd\n", maxAllowedMemDiff);
+    TEST_DEBUG_PRINT("Number of connections used:  %10zu\n", connectionsToUse);
 
     EXPECT_TRUE(vm_data_after_free_handshake <= vm_data_after_handshakes);
     EXPECT_TRUE(vm_data_after_release_buffers <= vm_data_after_free_handshake);
@@ -269,10 +269,6 @@ int main(int argc, char **argv)
     ssize_t handshake_diff = (vm_data_after_handshakes - vm_data_initial);
     ssize_t allocation_diff = (vm_data_after_allocation - vm_data_initial);
 
-    fprintf(stdout, "handshake diff:      %10zd\n", handshake_diff);
-    fprintf(stdout, "allocation diff:     %10zd\n", allocation_diff);
-    fprintf(stdout, "max allowed diff:    %10zd\n", maxAllowedMemDiff);
-
     /*
      * get_vm_data_size is required for this test to succeed.
      * Any platform that doesn't implement get_vm_data_size should be excluded here.