fix: pem parsing detection of last cert errors (#4908)

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)

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

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

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