Skip to content

Commit

Permalink
crypto: handle cert with invalid SPKI gracefully
Browse files Browse the repository at this point in the history
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 <rafael.nunu@hotmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
CVE-ID: CVE-2023-30588
  • Loading branch information
tniessen authored and RafaelGSS committed Jun 19, 2023
1 parent 70f9449 commit bf3e2c8
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 0 deletions.
4 changes: 4 additions & 0 deletions src/crypto/crypto_x509.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
39 changes: 39 additions & 0 deletions test/parallel/test-crypto-x509.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

0 comments on commit bf3e2c8

Please sign in to comment.