crypto: handle cert with invalid SPKI gracefully

When attempting to convert the SPKI of a X509Certificate to a KeyObject,
throw an error if the subjectPublicKey cannot be parsed instead of
aborting the process.

Fixes: https://hackerone.com/bugs?report_id=1884159
PR-URL: nodejs-private/node-private#393
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
CVE-ID: CVE-2023-30588

Author: tniessen

src/crypto/crypto_x509.cc

@@ -318,7 +318,11 @@ void X509Certificate::PublicKey(const FunctionCallbackInfo<Value>& args) {
   X509Certificate* cert;
   ASSIGN_OR_RETURN_UNWRAP(&cert, args.Holder());
 
+  // TODO(tniessen): consider checking X509_get_pubkey() when the
+  // X509Certificate object is being created.
+  ClearErrorOnReturn clear_error_on_return;
   EVPKeyPointer pkey(X509_get_pubkey(cert->get()));
+  if (!pkey) return ThrowCryptoError(env, ERR_get_error());
   ManagedEVPPKey epkey(std::move(pkey));
   std::shared_ptr<KeyObjectData> key_data =
       KeyObjectData::CreateAsymmetric(kKeyTypePublic, epkey);

test/parallel/test-crypto-x509.js

@@ -320,3 +320,42 @@ oans248kpal88CGqsN2so/wZKxVnpiXlPHMdiNL7hRSUqlHkUi07FrP2Htg8kjI=
     legacyObject.serialNumber,
     legacyObjectCheck.serialNumber);
 }
+
+{
+  // This X.509 Certificate can be parsed by OpenSSL because it contains a
+  // structurally sound TBSCertificate structure. However, the SPKI field of the
+  // TBSCertificate contains the subjectPublicKey as a BIT STRING, and this bit
+  // sequence is not a valid public key. Ensure that X509Certificate.publicKey
+  // does not abort in this case.
+
+  const certPem = `-----BEGIN CERTIFICATE-----
+MIIDpDCCAw0CFEc1OZ8g17q+PZnna3iQ/gfoZ7f3MA0GCSqGSIb3DQEBBQUAMIHX
+MRMwEQYLKwYBBAGCNzwCAQMTAkdJMR0wGwYDVQQPExRQcml2YXRlIE9yZ2FuaXph
+dGlvbjEOMAwGA1UEBRMFOTkxOTExCzAJBgNVBAYTAkdJMRIwEAYDVQQIFAlHaWJy
+YWx0YXIxEjAQBgNVBAcUCUdpYnJhbHRhcjEgMB4GA1UEChQXV0hHIChJbnRlcm5h
+dGlvbmFsKSBMdGQxHDAaBgNVBAsUE0ludGVyYWN0aXZlIEJldHRpbmcxHDAaBgNV
+BAMUE3d3dy53aWxsaWFtaGlsbC5jb20wIhgPMjAxNDAyMDcwMDAwMDBaGA8yMDE1
+MDIyMTIzNTk1OVowgbAxCzAJBgNVBAYTAklUMQ0wCwYDVQQIEwRSb21lMRAwDgYD
+VQQHEwdQb21lemlhMRYwFAYDVQQKEw1UZWxlY29taXRhbGlhMRIwEAYDVQQrEwlB
+RE0uQVAuUE0xHTAbBgNVBAMTFHd3dy50ZWxlY29taXRhbGlhLml0MTUwMwYJKoZI
+hvcNAQkBFiZ2YXNlc2VyY2l6aW9wb3J0YWxpY29AdGVsZWNvbWl0YWxpYS5pdDCB
+nzANBgkqhkiG9w0BAQEFAAOBjQA4gYkCgYEA5m/Vf7PevH+inMfUJOc8GeR7WVhM
+CQwcMM5k46MSZo7kCk7VZuaq5G2JHGAGnLPaPUkeXlrf5qLpTxXXxHNtz+WrDlFt
+boAdnTcqpX3+72uBGOaT6Wi/9YRKuCs5D5/cAxAc3XjHfpRXMoXObj9Vy7mLndfV
+/wsnTfU9QVeBkgsCAwEAAaOBkjCBjzAdBgNVHQ4EFgQUfLjAjEiC83A+NupGrx5+
+Qe6nhRMwbgYIKwYBBQUHAQwEYjBgoV6gXDBaMFgwVhYJaW1hZ2UvZ2lmMCEwHzAH
+BgUrDgMCGgQUS2u5KJYGDLvQUjibKaxLB4shBRgwJhYkaHR0cDovL2xvZ28udmVy
+aXNpZ24uY29tL3ZzbG9nbzEuZ2lmMA0GCSqGSIb3DQEBBQUAA4GBALLiAMX0cIMp
++V/JgMRhMEUKbrt5lYKfv9dil/f22ezZaFafb070jGMMPVy9O3/PavDOkHtTv3vd
+tAt3hIKFD1bJt6c6WtMH2Su3syosWxmdmGk5ihslB00lvLpfj/wed8i3bkcB1doq
+UcXd/5qu2GhokrKU2cPttU+XAN2Om6a0
+-----END CERTIFICATE-----`;
+
+  const cert = new X509Certificate(certPem);
+  assert.throws(() => cert.publicKey, {
+    message: common.hasOpenSSL3 ? /decode error/ : /wrong tag/,
+    name: 'Error'
+  });
+
+  assert.strictEqual(cert.checkIssued(cert), false);
+}