From 8e9334a348741f9f232801eca4e8cc2d97061f28 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Sat, 29 Sep 2018 14:26:43 +0200
Subject: [PATCH 1/2] crypto: make PEM parsing RFC7468-compliant

Fixes: https://github.com/nodejs/node/issues/13612
Fixes: https://github.com/nodejs/node/issues/22815
---
 src/node_crypto.cc | 83 ++++++++++++++++++++++++++++------------------
 1 file changed, 51 insertions(+), 32 deletions(-)

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 153bfd2d018466..10a821402369d5 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -45,13 +45,6 @@
 #include <memory>
 #include <vector>
 
-static const char PUBLIC_KEY_PFX[] =  "-----BEGIN PUBLIC KEY-----";
-static const int PUBLIC_KEY_PFX_LEN = sizeof(PUBLIC_KEY_PFX) - 1;
-static const char PUBRSA_KEY_PFX[] =  "-----BEGIN RSA PUBLIC KEY-----";
-static const int PUBRSA_KEY_PFX_LEN = sizeof(PUBRSA_KEY_PFX) - 1;
-static const char CERTIFICATE_PFX[] =  "-----BEGIN CERTIFICATE-----";
-static const int CERTIFICATE_PFX_LEN = sizeof(CERTIFICATE_PFX) - 1;
-
 static const int X509_NAME_FLAGS = ASN1_STRFLGS_ESC_CTRL
                                  | ASN1_STRFLGS_UTF8_CONVERT
                                  | XN_FLAG_SEP_MULTILINE
@@ -3650,6 +3643,31 @@ enum ParsePublicKeyResult {
   kParsePublicFailed
 };
 
+static ParsePublicKeyResult TryParsePublicKey(
+    EVPKeyPointer* pkey,
+    const BIOPointer& bp,
+    char* name,
+    // NOLINTNEXTLINE(runtime/int)
+    std::function<EVP_PKEY*(const unsigned char** p, long l)> parse) {
+  unsigned char* der_data;
+  long der_len;  // NOLINT(runtime/int)
+
+  // This skips surrounding data and decodes PEM to DER.
+  {
+    MarkPopErrorOnReturn mark_pop_error_on_return;
+    if (PEM_bytes_read_bio(&der_data, &der_len, nullptr, name,
+                           bp.get(), nullptr, nullptr) != 1)
+      return kParsePublicNotRecognized;
+  }
+
+  // OpenSSL might modify the pointer, so we need to make a copy before parsing.
+  const unsigned char* p = der_data;
+  pkey->reset(parse(&p, der_len));
+  OPENSSL_clear_free(der_data, der_len);
+
+  return *pkey ? kParsePublicOk : kParsePublicFailed;
+}
+
 static ParsePublicKeyResult ParsePublicKey(EVPKeyPointer* pkey,
                                            const char* key_pem,
                                            int key_pem_len) {
@@ -3657,31 +3675,32 @@ static ParsePublicKeyResult ParsePublicKey(EVPKeyPointer* pkey,
   if (!bp)
     return kParsePublicFailed;
 
-  // Check if this is a PKCS#8 or RSA public key before trying as X.509.
-  if (strncmp(key_pem, PUBLIC_KEY_PFX, PUBLIC_KEY_PFX_LEN) == 0) {
-    pkey->reset(
-        PEM_read_bio_PUBKEY(bp.get(), nullptr, NoPasswordCallback, nullptr));
-  } else if (strncmp(key_pem, PUBRSA_KEY_PFX, PUBRSA_KEY_PFX_LEN) == 0) {
-    RSAPointer rsa(PEM_read_bio_RSAPublicKey(
-        bp.get(), nullptr, PasswordCallback, nullptr));
-    if (rsa) {
-      pkey->reset(EVP_PKEY_new());
-      if (*pkey)
-        EVP_PKEY_set1_RSA(pkey->get(), rsa.get());
-    }
-  } else if (strncmp(key_pem, CERTIFICATE_PFX, CERTIFICATE_PFX_LEN) == 0) {
-    // X.509 fallback
-    X509Pointer x509(PEM_read_bio_X509(
-        bp.get(), nullptr, NoPasswordCallback, nullptr));
-    if (!x509)
-      return kParsePublicFailed;
-
-    pkey->reset(X509_get_pubkey(x509.get()));
-  } else {
-    return kParsePublicNotRecognized;
-  }
-
-  return *pkey ? kParsePublicOk : kParsePublicFailed;
+  ParsePublicKeyResult ret;
+
+  // Try PKCS#8 first.
+  ret = TryParsePublicKey(pkey, bp, "PUBLIC KEY",
+      [](const unsigned char** p, long l) {  // NOLINT(runtime/int)
+        return d2i_PUBKEY(nullptr, p, l);
+      });
+  if (ret != kParsePublicNotRecognized)
+    return ret;
+
+  // Maybe it is PKCS#1.
+  CHECK(BIO_reset(bp.get()));
+  ret = TryParsePublicKey(pkey, bp, "RSA PUBLIC KEY",
+      [](const unsigned char** p, long l) {  // NOLINT(runtime/int)
+        return d2i_PublicKey(EVP_PKEY_RSA, nullptr, p, l);
+      });
+  if (ret != kParsePublicNotRecognized)
+    return ret;
+
+  // X.509 fallback.
+  CHECK(BIO_reset(bp.get()));
+  return TryParsePublicKey(pkey, bp, "CERTIFICATE",
+      [](const unsigned char** p, long l) {  // NOLINT(runtime/int)
+        X509Pointer x509(d2i_X509(nullptr, p, l));
+        return x509 ? X509_get_pubkey(x509.get()) : nullptr;
+      });
 }
 
 void Verify::Initialize(Environment* env, v8::Local<Object> target) {

From 58ebfce4692c6dc7dbdfd75ee89cbc3a1cef2ba3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@users.noreply.github.com>
Date: Mon, 1 Oct 2018 17:08:49 +0200
Subject: [PATCH 2/2] Address bnoordhuis' comment

---
 src/node_crypto.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 10a821402369d5..05d4d8b7f50803 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -3646,7 +3646,7 @@ enum ParsePublicKeyResult {
 static ParsePublicKeyResult TryParsePublicKey(
     EVPKeyPointer* pkey,
     const BIOPointer& bp,
-    char* name,
+    const char* name,
     // NOLINTNEXTLINE(runtime/int)
     std::function<EVP_PKEY*(const unsigned char** p, long l)> parse) {
   unsigned char* der_data;