diff --git a/src/crypto/crypto_tls_certificates.cc b/src/crypto/crypto_tls_certificates.cc index ace1d41aefe..362270619ae 100644 --- a/src/crypto/crypto_tls_certificates.cc +++ b/src/crypto/crypto_tls_certificates.cc @@ -57,16 +57,33 @@ 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; + + // 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))) { + 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; + } + + // 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; } PrivateKeyResult UsePrivateKey(SSL_CTX* ctx, diff --git a/src/crypto/crypto_tls_certificates.h b/src/crypto/crypto_tls_certificates.h index e36a583e6d4..84d388b7fab 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 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 new file mode 100644 index 00000000000..f5f9cd6c10d --- /dev/null +++ b/test/parallel/test-tls-crl-bundle.js @@ -0,0 +1,42 @@ +'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')); + +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(); +})); + +// 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' });