Handle ASN.1 type detection errors (#3855)

crypto/s2n_certificate.c

@@ -113,21 +113,21 @@ int s2n_cert_chain_and_key_set_cert_chain(struct s2n_cert_chain_and_key *cert_an
     return S2N_SUCCESS;
 }
 
-int s2n_cert_chain_and_key_set_private_key_from_stuffer(struct s2n_cert_chain_and_key *cert_and_key, struct s2n_stuffer *key_in_stuffer, struct s2n_stuffer *key_out_stuffer)
+int s2n_cert_chain_and_key_set_private_key_from_stuffer(struct s2n_cert_chain_and_key *cert_and_key,
+        struct s2n_stuffer *key_in_stuffer, struct s2n_stuffer *key_out_stuffer)
 {
     struct s2n_blob key_blob = { 0 };
 
     POSIX_GUARD(s2n_pkey_zero_init(cert_and_key->private_key));
 
     /* Convert pem to asn1 and asn1 to the private key. Handles both PKCS#1 and PKCS#8 formats */
-    POSIX_GUARD(s2n_stuffer_private_key_from_pem(key_in_stuffer, key_out_stuffer));
+    int type = 0;
+    POSIX_GUARD(s2n_stuffer_private_key_from_pem(key_in_stuffer, key_out_stuffer, &type));
     key_blob.size = s2n_stuffer_data_available(key_out_stuffer);
     key_blob.data = s2n_stuffer_raw_read(key_out_stuffer, key_blob.size);
     POSIX_ENSURE_REF(key_blob.data);
 
-    /* Get key type and create appropriate key context */
-    POSIX_GUARD(s2n_asn1der_to_private_key(cert_and_key->private_key, &key_blob));
-
+    POSIX_GUARD(s2n_asn1der_to_private_key(cert_and_key->private_key, &key_blob, type));
     return S2N_SUCCESS;
 }
 

crypto/s2n_pkey.c

@@ -129,14 +129,29 @@ int s2n_pkey_free(struct s2n_pkey *key)
     return S2N_SUCCESS;
 }
 
-int s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob *asn1der)
+int s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob *asn1der, int type_hint)
 {
-    uint8_t *key_to_parse = asn1der->data;
-
-    /* Detect key type */
-    DEFER_CLEANUP(EVP_PKEY *evp_private_key = d2i_AutoPrivateKey(NULL, (const unsigned char **) (void *) &key_to_parse, asn1der->size),
+    const unsigned char *key_to_parse = asn1der->data;
+
+    /* We use "d2i_AutoPrivateKey" instead of "PEM_read_bio_PrivateKey" because
+     * s2n-tls prefers to perform its own custom PEM parsing. Historically,
+     * openssl's PEM parsing tended to ignore invalid certificates rather than
+     * error on them. We prefer to fail early rather than continue without
+     * the full and correct chain intended by the application.
+     */
+    DEFER_CLEANUP(EVP_PKEY *evp_private_key = d2i_AutoPrivateKey(NULL, &key_to_parse, asn1der->size),
             EVP_PKEY_free_pointer);
-    S2N_ERROR_IF(evp_private_key == NULL, S2N_ERR_DECODE_PRIVATE_KEY);
+
+    /* We have found cases where d2i_AutoPrivateKey fails to detect the type of
+     * the key. For example, openssl fails to identify an EC key without the
+     * optional publicKey field.
+     *
+     * If d2i_AutoPrivateKey fails, try once more with the type we parsed from the PEM.
+     */
+    if (evp_private_key == NULL) {
+        evp_private_key = d2i_PrivateKey(type_hint, NULL, &key_to_parse, asn1der->size);
+    }
+    POSIX_ENSURE(evp_private_key, S2N_ERR_DECODE_PRIVATE_KEY);
 
     /* If key parsing is successful, d2i_AutoPrivateKey increments *key_to_parse to the byte following the parsed data */
     uint32_t parsed_len = key_to_parse - asn1der->data;

crypto/s2n_pkey.h

@@ -69,5 +69,5 @@ int s2n_pkey_decrypt(const struct s2n_pkey *pkey, struct s2n_blob *in, struct s2
 int s2n_pkey_match(const struct s2n_pkey *pub_key, const struct s2n_pkey *priv_key);
 int s2n_pkey_free(struct s2n_pkey *pkey);
 
-int s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob *asn1der);
+int s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob *asn1der, int type_hint);
 int s2n_asn1der_to_public_key_and_type(struct s2n_pkey *pub_key, s2n_pkey_type *pkey_type, struct s2n_blob *asn1der);

stuffer/s2n_stuffer.h

@@ -166,7 +166,7 @@ int s2n_stuffer_alloc_ro_from_string(struct s2n_stuffer *stuffer, const char *st
 int s2n_stuffer_init_ro_from_string(struct s2n_stuffer *stuffer, uint8_t *data, uint32_t length);
 
 /* Read a private key from a PEM encoded stuffer to an ASN1/DER encoded one */
-int s2n_stuffer_private_key_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1);
+int 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 */
 int s2n_stuffer_certificate_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1);

stuffer/s2n_stuffer_pem.c

@@ -13,6 +13,7 @@
  * permissions and limitations under the License.
  */
 
+#include <openssl/evp.h>
 #include <string.h>
 
 #include "error/s2n_errno.h"
@@ -127,15 +128,15 @@ static int s2n_stuffer_data_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer
     return S2N_SUCCESS;
 }
 
-int s2n_stuffer_private_key_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1)
+int s2n_stuffer_private_key_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1, int *type)
 {
     POSIX_PRECONDITION(s2n_stuffer_validate(pem));
     POSIX_PRECONDITION(s2n_stuffer_validate(asn1));
-    int rc;
+    POSIX_ENSURE_REF(type);
 
-    rc = s2n_stuffer_data_from_pem(pem, asn1, S2N_PEM_PKCS1_RSA_PRIVATE_KEY);
-    if (!rc) {
-        return rc;
+    if (s2n_stuffer_data_from_pem(pem, asn1, S2N_PEM_PKCS1_RSA_PRIVATE_KEY) == S2N_SUCCESS) {
+        *type = EVP_PKEY_RSA;
+        return S2N_SUCCESS;
     }
 
     s2n_stuffer_reread(pem);
@@ -146,21 +147,25 @@ int s2n_stuffer_private_key_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer
      * compatible with OpenSSL's default output, and since "EC PARAMETERS" is
      * only needed for non-standard curves that aren't currently supported.
      */
-    rc = s2n_stuffer_data_from_pem(pem, asn1, S2N_PEM_EC_PARAMETERS);
-    if (rc < 0) {
+    if (s2n_stuffer_data_from_pem(pem, asn1, S2N_PEM_EC_PARAMETERS) != S2N_SUCCESS) {
         s2n_stuffer_reread(pem);
     }
     s2n_stuffer_wipe(asn1);
 
-    rc = s2n_stuffer_data_from_pem(pem, asn1, S2N_PEM_PKCS1_EC_PRIVATE_KEY);
-    if (!rc) {
-        return rc;
+    if (s2n_stuffer_data_from_pem(pem, asn1, S2N_PEM_PKCS1_EC_PRIVATE_KEY) == S2N_SUCCESS) {
+        *type = EVP_PKEY_EC;
+        return S2N_SUCCESS;
     }
 
     /* If it does not match either format, try PKCS#8 */
     s2n_stuffer_reread(pem);
     s2n_stuffer_reread(asn1);
-    return s2n_stuffer_data_from_pem(pem, asn1, S2N_PEM_PKCS8_PRIVATE_KEY);
+    if (s2n_stuffer_data_from_pem(pem, asn1, S2N_PEM_PKCS8_PRIVATE_KEY) == S2N_SUCCESS) {
+        *type = EVP_PKEY_RSA;
+        return S2N_SUCCESS;
+    }
+
+    POSIX_BAIL(S2N_ERR_INVALID_PEM);
 }
 
 int s2n_stuffer_certificate_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1)

tests/cbmc/proofs/s2n_stuffer_private_key_from_pem/Makefile

@@ -44,6 +44,7 @@ PROJECT_SOURCES += $(SRCDIR)/utils/s2n_safety.c
 PROJECT_SOURCES += $(SRCDIR)/utils/s2n_result.c
 
 # We abstract this function because manual inspection demonstrates it is unreachable.
+REMOVE_FUNCTION_BODY += __CPROVER_file_local_s2n_mem_c_s2n_mem_cleanup_impl
 REMOVE_FUNCTION_BODY += s2n_stuffer_resize
 REMOVE_FUNCTION_BODY += s2n_stuffer_rewrite
 REMOVE_FUNCTION_BODY += s2n_add_overflow

tests/cbmc/proofs/s2n_stuffer_private_key_from_pem/s2n_stuffer_private_key_from_pem_harness.c

@@ -16,16 +16,14 @@
 #include <assert.h>
 #include <cbmc_proof/cbmc_utils.h>
 #include <cbmc_proof/make_common_datastructures.h>
-#include <string.h>
 
 #include "api/s2n.h"
-#include "stuffer/s2n_stuffer.h"
-#include "utils/s2n_mem.h"
 
 void s2n_stuffer_private_key_from_pem_harness()
 {
     /* Non-deterministic inputs. */
     struct s2n_stuffer *pem = cbmc_allocate_s2n_stuffer();
+    int *type = malloc(sizeof(int));
     __CPROVER_assume(s2n_result_is_ok(s2n_stuffer_validate(pem)));
     __CPROVER_assume(s2n_blob_is_bounded(&pem->blob, MAX_BLOB_SIZE));
 
@@ -35,7 +33,7 @@ void s2n_stuffer_private_key_from_pem_harness()
     nondet_s2n_mem_init();
 
     /* Operation under verification. */
-    s2n_stuffer_private_key_from_pem(pem, asn1);
+    s2n_stuffer_private_key_from_pem(pem, asn1, type);
 
     /* Post-conditions. */
     assert(s2n_result_is_ok(s2n_stuffer_validate(pem)));

tests/pems/missing_public_key_ecdsa_cert.pem

@@ -0,0 +1,13 @@
+-----BEGIN CERTIFICATE-----
+MIICCTCCAa+gAwIBAgIUF8n7U9p/VpPXs5UyR2OIdvmd7GcwCgYIKoZIzj0EAwIw
+YTELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAldBMRAwDgYDVQQHDAdTZWF0dGxlMQ8w
+DQYDVQQKDAZBbWF6b24xDDAKBgNVBAsMA3MybjEUMBIGA1UEAwwLczJuVGVzdENl
+cnQwIBcNMjMwMjI4MDA0MzM5WhgPMjEyMzAyMDQwMDQzMzlaMGExCzAJBgNVBAYT
+AlVTMQswCQYDVQQIDAJXQTEQMA4GA1UEBwwHU2VhdHRsZTEPMA0GA1UECgwGQW1h
+em9uMQwwCgYDVQQLDANzMm4xFDASBgNVBAMMC3MyblRlc3RDZXJ0MFkwEwYHKoZI
+zj0CAQYIKoZIzj0DAQcDQgAEonlNIwdEHIAMXOi5BTrRvHBTPP8k/jNDPzvPoYME
+9hvlB2CueE7JMqfdlp3Pojy8+TWh/pW8pjdwxawlciIZ5aNDMEEwCwYDVR0PBAQD
+AgSwMBMGA1UdJQQMMAoGCCsGAQUFBwMBMB0GA1UdDgQWBBRjlLyKlq2vgnfkr2FQ
+SbP0Zowv3DAKBggqhkjOPQQDAgNIADBFAiEAl2eUzzK12NH+Dyfq3yrcqKlvT9oN
+DcsE3gCLILl6OuwCIDqk/uFgmUELVA/wqmbVaIJvV5JrraW+JyQjpyO8fpu+
+-----END CERTIFICATE-----

tests/pems/missing_public_key_ecdsa_key.pem

@@ -0,0 +1,3 @@
+-----BEGIN EC PRIVATE KEY-----
+MDECAQEEIOTVZZcmX0I5bIvyWuiblXk9jpODLnCBn1XkJd8M8sZSoAoGCCqGSM49AwEH
+-----END EC PRIVATE KEY-----

tests/unit/s2n_ecdsa_test.c

@@ -107,9 +107,12 @@ int main(int argc, char **argv)
     EXPECT_SUCCESS(s2n_blob_init(&b, (uint8_t *) unmatched_private_key, sizeof(unmatched_private_key)));
     EXPECT_SUCCESS(s2n_stuffer_write(&unmatched_ecdsa_key_in, &b));
 
+    int type = 0;
     EXPECT_SUCCESS(s2n_stuffer_certificate_from_pem(&certificate_in, &certificate_out));
-    EXPECT_SUCCESS(s2n_stuffer_private_key_from_pem(&ecdsa_key_in, &ecdsa_key_out));
-    EXPECT_SUCCESS(s2n_stuffer_private_key_from_pem(&unmatched_ecdsa_key_in, &unmatched_ecdsa_key_out));
+    EXPECT_SUCCESS(s2n_stuffer_private_key_from_pem(&ecdsa_key_in, &ecdsa_key_out, &type));
+    EXPECT_EQUAL(type, EVP_PKEY_EC);
+    EXPECT_SUCCESS(s2n_stuffer_private_key_from_pem(&unmatched_ecdsa_key_in, &unmatched_ecdsa_key_out, &type));
+    EXPECT_EQUAL(type, EVP_PKEY_EC);
 
     struct s2n_pkey pub_key = { 0 };
     struct s2n_pkey priv_key = { 0 };
@@ -121,13 +124,17 @@ int main(int argc, char **argv)
     EXPECT_SUCCESS(s2n_blob_init(&b, s2n_stuffer_raw_read(&certificate_out, available_size), available_size));
     EXPECT_SUCCESS(s2n_asn1der_to_public_key_and_type(&pub_key, &pkey_type, &b));
 
+    /* Test without a type hint */
+    int wrong_type = 0;
+    EXPECT_NOT_EQUAL(wrong_type, EVP_PKEY_EC);
+
     available_size = s2n_stuffer_data_available(&ecdsa_key_out);
     EXPECT_SUCCESS(s2n_blob_init(&b, s2n_stuffer_raw_read(&ecdsa_key_out, available_size), available_size));
-    EXPECT_SUCCESS(s2n_asn1der_to_private_key(&priv_key, &b));
+    EXPECT_SUCCESS(s2n_asn1der_to_private_key(&priv_key, &b, wrong_type));
 
     available_size = s2n_stuffer_data_available(&unmatched_ecdsa_key_out);
     EXPECT_SUCCESS(s2n_blob_init(&b, s2n_stuffer_raw_read(&unmatched_ecdsa_key_out, available_size), available_size));
-    EXPECT_SUCCESS(s2n_asn1der_to_private_key(&unmatched_priv_key, &b));
+    EXPECT_SUCCESS(s2n_asn1der_to_private_key(&unmatched_priv_key, &b, wrong_type));
 
     /* Verify that the public/private key pair match */
     EXPECT_SUCCESS(s2n_pkey_match(&pub_key, &priv_key));

tests/unit/s2n_pem_rsa_dhe_test.c

@@ -96,9 +96,11 @@ int main(int argc, char **argv)
     EXPECT_SUCCESS(s2n_blob_init(&b, (uint8_t *) dhparams_pem, strlen(dhparams_pem) + 1));
     EXPECT_SUCCESS(s2n_stuffer_write(&dhparams_in, &b));
 
+    int type = 0;
     EXPECT_SUCCESS(s2n_stuffer_certificate_from_pem(&certificate_in, &certificate_out));
-    EXPECT_SUCCESS(s2n_stuffer_private_key_from_pem(&rsa_key_in, &rsa_key_out));
+    EXPECT_SUCCESS(s2n_stuffer_private_key_from_pem(&rsa_key_in, &rsa_key_out, &type));
     EXPECT_SUCCESS(s2n_stuffer_dhparams_from_pem(&dhparams_in, &dhparams_out));
+    EXPECT_EQUAL(type, EVP_PKEY_RSA);
 
     struct s2n_pkey priv_key = { 0 };
     struct s2n_pkey pub_key = { 0 };
@@ -109,9 +111,13 @@ int main(int argc, char **argv)
     EXPECT_SUCCESS(s2n_blob_init(&b, s2n_stuffer_raw_read(&certificate_out, available_size), available_size));
     EXPECT_SUCCESS(s2n_asn1der_to_public_key_and_type(&pub_key, &pkey_type, &b));
 
+    /* Test without a type hint */
+    int wrong_type = 0;
+    EXPECT_NOT_EQUAL(wrong_type, EVP_PKEY_RSA);
+
     available_size = s2n_stuffer_data_available(&rsa_key_out);
     EXPECT_SUCCESS(s2n_blob_init(&b, s2n_stuffer_raw_read(&rsa_key_out, available_size), available_size));
-    EXPECT_SUCCESS(s2n_asn1der_to_private_key(&priv_key, &b));
+    EXPECT_SUCCESS(s2n_asn1der_to_private_key(&priv_key, &b, wrong_type));
 
     EXPECT_SUCCESS(s2n_pkey_match(&pub_key, &priv_key));
 

tests/unit/s2n_pem_test.c

@@ -21,6 +21,11 @@
 #include "testlib/s2n_testlib.h"
 #include "utils/s2n_safety.h"
 
+/* The ECDSA private key is missing the "publicKey" field, which is optional.
+ * The missing field makes the cert type difficult to detect via ASN1 parsing */
+#define S2N_MISSING_ECDSA_PUB_CERT_KEY        "../pems/missing_public_key_ecdsa_key.pem"
+#define S2N_MISSING_ECDSA_PUB_CERT_CERT_CHAIN "../pems/missing_public_key_ecdsa_cert.pem"
+
 static const char *valid_pem_pairs[][2] = {
     { S2N_RSA_2048_PKCS8_CERT_CHAIN, S2N_RSA_2048_PKCS8_KEY },
     { S2N_RSA_2048_PKCS1_CERT_CHAIN, S2N_RSA_2048_PKCS1_KEY },
@@ -34,6 +39,7 @@ static const char *valid_pem_pairs[][2] = {
     { S2N_LEADING_COMMENT_TEXT_CERT_CHAIN, S2N_RSA_2048_PKCS1_KEY },
     { S2N_LONG_BASE64_LINES_CERT_CHAIN, S2N_RSA_2048_PKCS1_KEY },
     { S2N_MISSING_LINE_ENDINGS_CERT_CHAIN, S2N_RSA_2048_PKCS1_KEY },
+    { S2N_MISSING_ECDSA_PUB_CERT_CERT_CHAIN, S2N_MISSING_ECDSA_PUB_CERT_KEY },
 
     /* Technically Invalid according to RFC, but that we are lenient towards */
     { S2N_INVALID_HEADER_CERT_CHAIN, S2N_RSA_2048_PKCS1_KEY },