From 9db73edc12d3e8f9eeb27f52c424d417e22a1e88 Mon Sep 17 00:00:00 2001 From: Lazizbek Ergashev Date: Thu, 27 Aug 2026 13:19:17 +0500 Subject: [PATCH 1/2] tls: load all CRLs from a PEM bundle AddCRL() read a single PEM block and returned, so a crl option holding several concatenated CRLs only ever got its first entry into the store. Any certificate whose issuer's CRL came later in the bundle then failed with UNABLE_TO_GET_CRL. Read in a loop until the BIO is exhausted, the way AddCACertificates() right above it already does for ca bundles. Signed-off-by: Lazizbek Ergashev --- src/crypto/crypto_tls_certificates.cc | 24 +++++++++++++--------- src/crypto/crypto_tls_certificates.h | 3 ++- test/parallel/test-tls-crl-bundle.js | 29 +++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 11 deletions(-) create mode 100644 test/parallel/test-tls-crl-bundle.js diff --git a/src/crypto/crypto_tls_certificates.cc b/src/crypto/crypto_tls_certificates.cc index ace1d41aefec..9ffdfd01accb 100644 --- a/src/crypto/crypto_tls_certificates.cc +++ b/src/crypto/crypto_tls_certificates.cc @@ -57,16 +57,20 @@ bool AddCRL(Environment* env, X509_STORE** cache) { if (!bio) return false; - DeleteFnPtr crl( - PEM_read_bio_X509_CRL(bio.get(), nullptr, NoPasswordCallback, nullptr)); - if (!crl) return false; - - X509_STORE* cert_store = GetOrCreateOwnedCertStore(env, ctx, cache); - CHECK_EQ(1, X509_STORE_add_crl(cert_store, crl.get())); - CHECK_EQ(1, - X509_STORE_set_flags( - cert_store, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL)); - return true; + using CRLPointer = DeleteFnPtr; + + bool added = false; + while (CRLPointer crl = CRLPointer(PEM_read_bio_X509_CRL( + bio.get(), nullptr, NoPasswordCallback, nullptr))) { + X509_STORE* cert_store = GetOrCreateOwnedCertStore(env, ctx, cache); + CHECK_EQ(1, X509_STORE_add_crl(cert_store, crl.get())); + CHECK_EQ( + 1, + X509_STORE_set_flags( + cert_store, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL)); + added = true; + } + return added; } PrivateKeyResult UsePrivateKey(SSL_CTX* ctx, diff --git a/src/crypto/crypto_tls_certificates.h b/src/crypto/crypto_tls_certificates.h index e36a583e6d4b..85eea906d937 100644 --- a/src/crypto/crypto_tls_certificates.h +++ b/src/crypto/crypto_tls_certificates.h @@ -30,7 +30,8 @@ size_t AddCACertificates(Environment* env, const ncrypto::BIOPointer& bio, X509_STORE** cache = nullptr); -// Add one PEM CRL and enable CRL checking. +// Add every PEM CRL in |bio| to the context's certificate store and enable CRL +// checking. Returns false if no CRL was read. bool AddCRL(Environment* env, SSL_CTX* ctx, const ncrypto::BIOPointer& bio, diff --git a/test/parallel/test-tls-crl-bundle.js b/test/parallel/test-tls-crl-bundle.js new file mode 100644 index 000000000000..ed163be5526b --- /dev/null +++ b/test/parallel/test-tls-crl-bundle.js @@ -0,0 +1,29 @@ +'use strict'; +const common = require('../common'); + +// Verify that every CRL in a concatenated PEM bundle is loaded, not just the +// first one. agent3 is revoked by ca2-crl-agent3.pem, but not by ca2-crl.pem. + +const fixtures = require('../common/fixtures'); +const { + assert, connect, keys +} = require(fixtures.path('tls-connect')); + +const crl = fixtures.readKey('ca2-crl.pem') + + fixtures.readKey('ca2-crl-agent3.pem'); + +connect({ + client: { + servername: 'agent3', + ca: keys.agent3.ca, + crl, + }, + server: { + cert: keys.agent3.cert, + key: keys.agent3.key, + }, +}, common.mustCall((err, pair, cleanup) => { + assert(err); + assert.strictEqual(err.code, 'CERT_REVOKED'); + return cleanup(); +})); From 02967d33acc53ce145a03a53410f60b44926bede Mon Sep 17 00:00:00 2001 From: Lazizbek Ergashev Date: Thu, 27 Aug 2026 16:18:39 +0500 Subject: [PATCH 2/2] fixup! tls: load all CRLs from a PEM bundle Return false unless the read loop ran to the end of the BIO, so a bundle holding an unparseable CRL throws instead of quietly applying only the entries that were read. Signed-off-by: Lazizbek Ergashev --- src/crypto/crypto_tls_certificates.cc | 13 +++++++++++++ src/crypto/crypto_tls_certificates.h | 2 +- test/parallel/test-tls-crl-bundle.js | 13 +++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/crypto/crypto_tls_certificates.cc b/src/crypto/crypto_tls_certificates.cc index 9ffdfd01accb..362270619aee 100644 --- a/src/crypto/crypto_tls_certificates.cc +++ b/src/crypto/crypto_tls_certificates.cc @@ -59,6 +59,9 @@ bool AddCRL(Environment* env, using CRLPointer = DeleteFnPtr; + // So that ERR_peek_last_error() below only reports errors from this loop. + ERR_clear_error(); + bool added = false; while (CRLPointer crl = CRLPointer(PEM_read_bio_X509_CRL( bio.get(), nullptr, NoPasswordCallback, nullptr))) { @@ -70,6 +73,16 @@ bool AddCRL(Environment* env, cert_store, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL)); added = true; } + + // The loop stops either because the BIO is exhausted, which OpenSSL reports + // as PEM_R_NO_START_LINE, or because a CRL failed to parse. Only the former + // means every CRL in the bundle made it into the store. + unsigned long err = ERR_peek_last_error(); // NOLINT(runtime/int) + if (ERR_GET_LIB(err) != ERR_LIB_PEM || + ERR_GET_REASON(err) != PEM_R_NO_START_LINE) { + return false; + } + return added; } diff --git a/src/crypto/crypto_tls_certificates.h b/src/crypto/crypto_tls_certificates.h index 85eea906d937..84d388b7fab1 100644 --- a/src/crypto/crypto_tls_certificates.h +++ b/src/crypto/crypto_tls_certificates.h @@ -31,7 +31,7 @@ size_t AddCACertificates(Environment* env, X509_STORE** cache = nullptr); // Add every PEM CRL in |bio| to the context's certificate store and enable CRL -// checking. Returns false if no CRL was read. +// checking. Returns false unless every CRL in the bundle was read. bool AddCRL(Environment* env, SSL_CTX* ctx, const ncrypto::BIOPointer& bio, diff --git a/test/parallel/test-tls-crl-bundle.js b/test/parallel/test-tls-crl-bundle.js index ed163be5526b..f5f9cd6c10df 100644 --- a/test/parallel/test-tls-crl-bundle.js +++ b/test/parallel/test-tls-crl-bundle.js @@ -1,10 +1,13 @@ 'use strict'; const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); // Verify that every CRL in a concatenated PEM bundle is loaded, not just the // first one. agent3 is revoked by ca2-crl-agent3.pem, but not by ca2-crl.pem. const fixtures = require('../common/fixtures'); +const tls = require('tls'); const { assert, connect, keys } = require(fixtures.path('tls-connect')); @@ -27,3 +30,13 @@ connect({ assert.strictEqual(err.code, 'CERT_REVOKED'); return cleanup(); })); + +// A bundle whose second entry does not parse must throw rather than quietly +// apply only the CRLs that were read. +const lines = fixtures.readKey('ca2-crl-agent3.pem', 'utf8').split('\n'); +lines[2] = 'AAAA' + lines[2].slice(4); +assert.throws(() => { + tls.createSecureContext({ + crl: fixtures.readKey('ca2-crl.pem', 'utf8') + lines.join('\n'), + }); +}, { code: 'ERR_CRYPTO_OPERATION_FAILED' });