refactor: ossl x509 parsing (#4351)

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 occurring.

Co-authored-by: Sam Clark <3758302+goatgoose@users.noreply.github.com>

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) {
@@ -224,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) {
@@ -276,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) {
@@ -297,7 +300,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 +314,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 +337,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 +356,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(&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_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));
 
@@ -374,7 +373,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,15 +721,15 @@ 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);
 
     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 +768,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 +779,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 +789,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 +808,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 +823,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 +835,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.
                  */

crypto/s2n_openssl_x509.c

@@ -39,3 +39,47 @@ 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(asn1der->data);
+    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 = 0;
+    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 = 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.
+     * 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;
+}

crypto/s2n_openssl_x509.h

@@ -19,10 +19,28 @@
 #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`.
+ */
+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.
+ */
+S2N_RESULT s2n_openssl_x509_parse_without_length_validation(struct s2n_blob *asn1der, X509 **cert_out);

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,19 +183,19 @@ 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_from_x509(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;
+    return S2N_RESULT_OK;
+}
 
-    /* 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);
+S2N_RESULT s2n_pkey_from_x509(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);
@@ -206,25 +204,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;

crypto/s2n_pkey.h

@@ -71,3 +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);

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);
             }

tests/unit/s2n_examples_test.c

@@ -238,15 +238,16 @@ static S2N_RESULT s2n_run_self_talk_test(s2n_test_scenario scenario_fn)
          */
         fclose(stdout);
 
-        DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
-                s2n_connection_ptr_free);
+        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_OK(scenario_fn(client, &input));
 
+        EXPECT_SUCCESS(s2n_connection_free(client));
+
         exit(EXIT_SUCCESS);
     }
 
@@ -257,15 +258,16 @@ 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));
         EXPECT_SUCCESS(s2n_connection_set_io_pair(server, &io_pair));
 
         EXPECT_OK(scenario_fn(server, &input));
 
+        EXPECT_SUCCESS(s2n_connection_free(server));
+
         exit(EXIT_SUCCESS);
     }
 

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