Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: ossl x509 parsing #4351

Merged
merged 16 commits into from
Jan 22, 2024
49 changes: 24 additions & 25 deletions crypto/s2n_certificate.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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.
*
*
Comment on lines -300 to +303
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware of the option to hide whitespace, but I would suggest setting up your IDE so that it doesn't make these changes to lines you don't touch. Since it seems like none of our linters care, you're going to end up with some messy diffs otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have disabled for the time being, but this is the default preference of clang-format and I would be in favor of just adding a linter that does care 😄. Opened #4362 to track this

* `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`
Expand All @@ -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;
Expand All @@ -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) {
jmayclin marked this conversation as resolved.
Show resolved Hide resolved
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;
}

Expand All @@ -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;
goatgoose marked this conversation as resolved.
Show resolved Hide resolved

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

Expand All @@ -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) {
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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:
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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.
*/
Expand All @@ -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.
*/
Expand Down
44 changes: 44 additions & 0 deletions crypto/s2n_openssl_x509.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
jmayclin marked this conversation as resolved.
Show resolved Hide resolved
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;
}
26 changes: 26 additions & 0 deletions crypto/s2n_openssl_x509.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
jmayclin marked this conversation as resolved.
Show resolved Hide resolved
*/
S2N_RESULT s2n_openssl_x509_parse_without_length_validation(struct s2n_blob *asn1der, X509 **cert_out);
36 changes: 17 additions & 19 deletions crypto/s2n_pkey.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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_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;
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_x509_to_public_key(X509 *cert, struct s2n_pkey *pub_key_out,
s2n_pkey_type *pkey_type_out)
{
jmayclin marked this conversation as resolved.
Show resolved Hide resolved
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);
Expand All @@ -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;
Expand Down
1 change: 1 addition & 0 deletions crypto/s2n_pkey.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
jmayclin marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion tests/unit/s2n_certificate_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only behavior change here is that failing the openssl x509 parsing call returns a "decode certificate" error rather than the previous "cert invalid error". The same d2i_X509 failure is occurring that previously occurred.

        server_cert = d2i_X509(NULL, &data, asn1_cert.size);
        RESULT_ENSURE(server_cert, S2N_ERR_DECODE_CERTIFICATE);

I'm confused by this comment on the test case

Validate that the certificate chain we generated is parseable.

The certificate is "readable" by s2n_x509_validator_read_asn1_cert, but not parseable by openssl.

EXPECT_NOT_EQUAL(server->handshake_params.client_cert_chain.size, 0);
EXPECT_NOT_NULL(server->handshake_params.client_cert_chain.data);
}
Expand Down
Loading
Loading