diff --git a/crypto/s2n_certificate.c b/crypto/s2n_certificate.c index 77629baf9b4..0961e309837 100644 --- a/crypto/s2n_certificate.c +++ b/crypto/s2n_certificate.c @@ -47,45 +47,30 @@ int s2n_create_cert_chain_from_stuffer(struct s2n_cert_chain *cert_chain_out, st struct s2n_cert **insert = &cert_chain_out->head; uint32_t chain_size = 0; - do { - struct s2n_cert *new_node = NULL; + while (s2n_stuffer_pem_has_certificate(chain_in_stuffer)) { + int result = s2n_stuffer_certificate_from_pem(chain_in_stuffer, &cert_out_stuffer); + POSIX_ENSURE(result == S2N_SUCCESS, S2N_ERR_INVALID_PEM); - if (s2n_stuffer_certificate_from_pem(chain_in_stuffer, &cert_out_stuffer) < 0) { - if (chain_size == 0) { - POSIX_BAIL(S2N_ERR_NO_CERTIFICATE_IN_PEM); - } - break; - } - struct s2n_blob mem = { 0 }; + DEFER_CLEANUP(struct s2n_blob mem = { 0 }, s2n_free); 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) { - POSIX_GUARD(s2n_free(&mem)); - S2N_ERROR_PRESERVE_ERRNO(); - } - if (s2n_stuffer_read(&cert_out_stuffer, &new_node->raw) != S2N_SUCCESS) { - POSIX_GUARD(s2n_free(&mem)); - S2N_ERROR_PRESERVE_ERRNO(); - } + struct s2n_cert *new_node = (struct s2n_cert *) (void *) mem.data; + POSIX_GUARD(s2n_alloc(&new_node->raw, s2n_stuffer_data_available(&cert_out_stuffer))); + POSIX_GUARD(s2n_stuffer_read(&cert_out_stuffer, &new_node->raw)); + ZERO_TO_DISABLE_DEFER_CLEANUP(mem); /* Additional 3 bytes for the length field in the protocol */ chain_size += new_node->raw.size + 3; new_node->next = NULL; *insert = new_node; insert = &new_node->next; - } while (s2n_stuffer_data_available(chain_in_stuffer)); - - /* Leftover data at this point means one of two things: - * A bug in s2n's PEM parsing OR a malformed PEM in the user's chain. - * Be conservative and fail instead of using a partial chain. - */ - S2N_ERROR_IF(s2n_stuffer_data_available(chain_in_stuffer) > 0, S2N_ERR_INVALID_PEM); + }; + POSIX_ENSURE(chain_size > 0, S2N_ERR_NO_CERTIFICATE_IN_PEM); cert_chain_out->chain_size = chain_size; - return 0; + return S2N_SUCCESS; } int s2n_cert_chain_and_key_set_cert_chain_from_stuffer(struct s2n_cert_chain_and_key *cert_and_key, struct s2n_stuffer *chain_in_stuffer) diff --git a/stuffer/s2n_stuffer.h b/stuffer/s2n_stuffer.h index 05e46d49006..f3d8f29cb3c 100644 --- a/stuffer/s2n_stuffer.h +++ b/stuffer/s2n_stuffer.h @@ -212,8 +212,9 @@ int S2N_RESULT_MUST_USE s2n_stuffer_vprintf(struct s2n_stuffer *stuffer, const c /* Read a private key from a PEM encoded stuffer to an ASN1/DER encoded one */ int S2N_RESULT_MUST_USE s2n_stuffer_private_key_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1, int *type); -/* Read a certificate from a PEM encoded stuffer to an ASN1/DER encoded one */ +/* Read a certificate from a PEM encoded stuffer to an ASN1/DER encoded one */ int S2N_RESULT_MUST_USE s2n_stuffer_certificate_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1); +bool s2n_stuffer_pem_has_certificate(struct s2n_stuffer *pem); /* Read a CRL from a PEM encoded stuffer to an ASN1/DER encoded one */ int S2N_RESULT_MUST_USE s2n_stuffer_crl_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1); diff --git a/stuffer/s2n_stuffer_pem.c b/stuffer/s2n_stuffer_pem.c index cf5d2ecb205..38448e0e37c 100644 --- a/stuffer/s2n_stuffer_pem.c +++ b/stuffer/s2n_stuffer_pem.c @@ -34,19 +34,30 @@ #define S2N_PEM_CERTIFICATE "CERTIFICATE" #define S2N_PEM_CRL "X509 CRL" -static int s2n_stuffer_pem_read_encapsulation_line(struct s2n_stuffer *pem, const char *encap_marker, - const char *keyword) +static S2N_RESULT s2n_stuffer_pem_read_delimiter_chars(struct s2n_stuffer *pem) { - /* Skip any number of Chars until a "--" is reached. + RESULT_ENSURE_REF(pem); + RESULT_ENSURE(s2n_stuffer_data_available(pem) >= S2N_PEM_DELIMITER_MIN_COUNT, + S2N_ERR_INVALID_PEM); + + /* Skip any number of chars until a "--" is reached. * We use "--" instead of "-" to account for dashes that appear in comments. * We do not accept comments that contain "--". */ - POSIX_GUARD(s2n_stuffer_skip_read_until(pem, S2N_PEM_DELIMITER_TOKEN)); - POSIX_GUARD(s2n_stuffer_rewind_read(pem, strlen(S2N_PEM_DELIMITER_TOKEN))); + RESULT_GUARD_POSIX(s2n_stuffer_skip_read_until(pem, S2N_PEM_DELIMITER_TOKEN)); + RESULT_GUARD_POSIX(s2n_stuffer_rewind_read(pem, strlen(S2N_PEM_DELIMITER_TOKEN))); /* Ensure between 2 and 64 '-' chars at start of line. */ - POSIX_GUARD(s2n_stuffer_skip_expected_char(pem, S2N_PEM_DELIMITER_CHAR, S2N_PEM_DELIMITER_MIN_COUNT, - S2N_PEM_DELIMITER_MAX_COUNT, NULL)); + RESULT_GUARD_POSIX(s2n_stuffer_skip_expected_char(pem, S2N_PEM_DELIMITER_CHAR, + S2N_PEM_DELIMITER_MIN_COUNT, S2N_PEM_DELIMITER_MAX_COUNT, NULL)); + + return S2N_RESULT_OK; +} + +static int s2n_stuffer_pem_read_encapsulation_line(struct s2n_stuffer *pem, const char *encap_marker, + const char *keyword) +{ + POSIX_GUARD_RESULT(s2n_stuffer_pem_read_delimiter_chars(pem)); /* Ensure next string in stuffer is "BEGIN " or "END " */ POSIX_GUARD(s2n_stuffer_read_expected_str(pem, encap_marker)); @@ -173,6 +184,19 @@ int s2n_stuffer_private_key_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer POSIX_BAIL(S2N_ERR_INVALID_PEM); } +/* We define a pem containing a certificate as a pem containing the delimiter chars. + * If the delimiter chars exist but the certificate keyword doesn't, that is a parsing error. + * That ensures that we don't ignore unexpected keywords like "END CERTIFICATE" + * instead of "BEGIN CERTIFICATE" or malformed keywords like "BEGAN CERTIFICATE" + * instead of "BEGIN CERTIFICATE". + */ +bool s2n_stuffer_pem_has_certificate(struct s2n_stuffer *pem) +{ + /* Operate on a copy of pem to avoid modifying pem */ + struct s2n_stuffer pem_copy = *pem; + return s2n_result_is_ok(s2n_stuffer_pem_read_delimiter_chars(&pem_copy)); +} + int s2n_stuffer_certificate_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1) { return s2n_stuffer_data_from_pem(pem, asn1, S2N_PEM_CERTIFICATE); diff --git a/tests/unit/s2n_certificate_parsing_test.c b/tests/unit/s2n_certificate_parsing_test.c index 260f763fc5f..d14d3fb7a18 100644 --- a/tests/unit/s2n_certificate_parsing_test.c +++ b/tests/unit/s2n_certificate_parsing_test.c @@ -250,6 +250,23 @@ int main(int argc, char **argv) "--" BEGIN_CERT "--" CERTIFICATE_2 "--" END_CERT "--" BEGIN_CERT "--" CERTIFICATE_3 "--" END_CERT "--" , }, + { + .name = "non-certificate data", + .input = + "this is not a certificate\n" + BEGIN_CERT_LINE "\n" + CERTIFICATE_1 "\n" + END_CERT_LINE "\n" + "\n" + "this is not a certificate either\n" + "\n" + BEGIN_CERT_LINE "\n" + CERTIFICATE_2 "\n" + END_CERT_LINE "not a certificate\n" + "not a certificate" BEGIN_CERT_LINE "\n" + CERTIFICATE_3 "\n" + END_CERT_LINE "\n", + }, { .name = "comments", .input = @@ -304,6 +321,50 @@ int main(int argc, char **argv) CERTIFICATE_3 "\n" END_CERT_LINE "\n", }, + { + .name = "trailing comment", + .input = + BEGIN_CERT_LINE "\n" + CERTIFICATE_1 "\n" + END_CERT_LINE "\n" + BEGIN_CERT_LINE "\n" + CERTIFICATE_2 "\n" + END_CERT_LINE "\n" + BEGIN_CERT_LINE "\n" + CERTIFICATE_3 "\n" + END_CERT_LINE "\n" + "# trailing comment \n" + }, + { + .name = "trailing whitespace", + .input = + BEGIN_CERT_LINE "\n" + CERTIFICATE_1 "\n" + END_CERT_LINE "\n" + BEGIN_CERT_LINE "\n" + CERTIFICATE_2 "\n" + END_CERT_LINE "\n" + BEGIN_CERT_LINE "\n" + CERTIFICATE_3 "\n" + END_CERT_LINE "\n" + "\n" + "\n", + }, + { + .name = "trailing non-certificate data", + .input = + BEGIN_CERT_LINE "\n" + CERTIFICATE_1 "\n" + END_CERT_LINE "\n" + BEGIN_CERT_LINE "\n" + CERTIFICATE_2 "\n" + END_CERT_LINE "\n" + BEGIN_CERT_LINE "\n" + CERTIFICATE_3 "\n" + END_CERT_LINE "\n" + "this is not a certificate\n" + "neither is this", + }, }; /* clang-format on */ @@ -420,6 +481,32 @@ int main(int argc, char **argv) CERTIFICATE_1 "\n" END_CERT_LINE "\n" }, + { + .name = "multiple certs: trailing marker", + .input = + BEGIN_CERT_LINE "\n" + CERTIFICATE_1 "\n" + END_CERT_LINE "\n" + BEGIN_CERT_LINE "\n" + }, + { + .name = "multiple certs: trailing partial certificate", + .input = + BEGIN_CERT_LINE "\n" + CERTIFICATE_1 "\n" + END_CERT_LINE "\n" + BEGIN_CERT_LINE "\n" + "MIIBljCCATygAwIBAg\n" + }, + { + .name = "multiple certs: missing end marker", + .input = + BEGIN_CERT_LINE "\n" + CERTIFICATE_1 "\n" + END_CERT_LINE "\n" + BEGIN_CERT_LINE "\n" + CERTIFICATE_1 "\n" + }, }; /* clang-format on */ @@ -439,6 +526,42 @@ int main(int argc, char **argv) }; } + /* Any case that is invalid for a single certificate should also be invalid + * for a certificate chain. + * We do not want to rely solely on our test for 0-length chains. + */ + const char *valid_cert_chain = test_cases[0].input; + for (size_t i = 0; i < s2n_array_len(bad_test_cases); i++) { + const struct s2n_test_case *test_case = &bad_test_cases[i]; + + /* The extra character is for an extra newline */ + size_t test_input_size = strlen(test_case->input) + strlen(valid_cert_chain) + 1; + + DEFER_CLEANUP(struct s2n_stuffer test_input = { 0 }, s2n_stuffer_free); + EXPECT_SUCCESS(s2n_stuffer_alloc(&test_input, test_input_size)); + EXPECT_SUCCESS(s2n_stuffer_write_str(&test_input, valid_cert_chain)); + + /* Sanity check: valid chain is valid */ + DEFER_CLEANUP(struct s2n_cert_chain_and_key *good_chain_and_key = s2n_cert_chain_and_key_new(), + s2n_cert_chain_and_key_ptr_free); + struct s2n_cert_chain *good_chain = good_chain_and_key->cert_chain; + EXPECT_SUCCESS(s2n_create_cert_chain_from_stuffer(good_chain, &test_input)); + + /* Add the invalid input to the end of the proven valid chain */ + EXPECT_SUCCESS(s2n_stuffer_write_char(&test_input, '\n')); + EXPECT_SUCCESS(s2n_stuffer_write_str(&test_input, test_case->input)); + EXPECT_SUCCESS(s2n_stuffer_reread(&test_input)); + + /* Test: valid chain + invalid test case is sill invalid */ + DEFER_CLEANUP(struct s2n_cert_chain_and_key *bad_chain_and_key = s2n_cert_chain_and_key_new(), + s2n_cert_chain_and_key_ptr_free); + struct s2n_cert_chain *bad_chain = bad_chain_and_key->cert_chain; + if (s2n_create_cert_chain_from_stuffer(bad_chain, &test_input) == S2N_SUCCESS) { + fprintf(stderr, "Successfully parsed invalid cert chain \"%s\"\n", test_case->name); + FAIL_MSG("Successfully parsed invalid cert chain"); + }; + } + for (size_t i = 0; i < s2n_array_len(expected_certs); i++) { EXPECT_SUCCESS(s2n_free(&expected_certs[i])); }