Skip to content

Commit 2a8b5d9

Browse files
lazergaduh95
authored andcommitted
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 <lazerg2@gmail.com> PR-URL: #65577 Fixes: #65576 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent 88ba17b commit 2a8b5d9

3 files changed

Lines changed: 71 additions & 11 deletions

File tree

src/crypto/crypto_tls_certificates.cc

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -57,16 +57,33 @@ bool AddCRL(Environment* env,
5757
X509_STORE** cache) {
5858
if (!bio) return false;
5959

60-
DeleteFnPtr<X509_CRL, X509_CRL_free> crl(
61-
PEM_read_bio_X509_CRL(bio.get(), nullptr, NoPasswordCallback, nullptr));
62-
if (!crl) return false;
63-
64-
X509_STORE* cert_store = GetOrCreateOwnedCertStore(env, ctx, cache);
65-
CHECK_EQ(1, X509_STORE_add_crl(cert_store, crl.get()));
66-
CHECK_EQ(1,
67-
X509_STORE_set_flags(
68-
cert_store, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL));
69-
return true;
60+
using CRLPointer = DeleteFnPtr<X509_CRL, X509_CRL_free>;
61+
62+
// So that ERR_peek_last_error() below only reports errors from this loop.
63+
ERR_clear_error();
64+
65+
bool added = false;
66+
while (CRLPointer crl = CRLPointer(PEM_read_bio_X509_CRL(
67+
bio.get(), nullptr, NoPasswordCallback, nullptr))) {
68+
X509_STORE* cert_store = GetOrCreateOwnedCertStore(env, ctx, cache);
69+
CHECK_EQ(1, X509_STORE_add_crl(cert_store, crl.get()));
70+
CHECK_EQ(
71+
1,
72+
X509_STORE_set_flags(
73+
cert_store, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL));
74+
added = true;
75+
}
76+
77+
// The loop stops either because the BIO is exhausted, which OpenSSL reports
78+
// as PEM_R_NO_START_LINE, or because a CRL failed to parse. Only the former
79+
// means every CRL in the bundle made it into the store.
80+
unsigned long err = ERR_peek_last_error(); // NOLINT(runtime/int)
81+
if (ERR_GET_LIB(err) != ERR_LIB_PEM ||
82+
ERR_GET_REASON(err) != PEM_R_NO_START_LINE) {
83+
return false;
84+
}
85+
86+
return added;
7087
}
7188

7289
PrivateKeyResult UsePrivateKey(SSL_CTX* ctx,

src/crypto/crypto_tls_certificates.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,8 @@ size_t AddCACertificates(Environment* env,
3030
const ncrypto::BIOPointer& bio,
3131
X509_STORE** cache = nullptr);
3232

33-
// Add one PEM CRL and enable CRL checking.
33+
// Add every PEM CRL in |bio| to the context's certificate store and enable CRL
34+
// checking. Returns false unless every CRL in the bundle was read.
3435
bool AddCRL(Environment* env,
3536
SSL_CTX* ctx,
3637
const ncrypto::BIOPointer& bio,
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
'use strict';
2+
const common = require('../common');
3+
if (!common.hasCrypto)
4+
common.skip('missing crypto');
5+
6+
// Verify that every CRL in a concatenated PEM bundle is loaded, not just the
7+
// first one. agent3 is revoked by ca2-crl-agent3.pem, but not by ca2-crl.pem.
8+
9+
const fixtures = require('../common/fixtures');
10+
const tls = require('tls');
11+
const {
12+
assert, connect, keys
13+
} = require(fixtures.path('tls-connect'));
14+
15+
const crl = fixtures.readKey('ca2-crl.pem') +
16+
fixtures.readKey('ca2-crl-agent3.pem');
17+
18+
connect({
19+
client: {
20+
servername: 'agent3',
21+
ca: keys.agent3.ca,
22+
crl,
23+
},
24+
server: {
25+
cert: keys.agent3.cert,
26+
key: keys.agent3.key,
27+
},
28+
}, common.mustCall((err, pair, cleanup) => {
29+
assert(err);
30+
assert.strictEqual(err.code, 'CERT_REVOKED');
31+
return cleanup();
32+
}));
33+
34+
// A bundle whose second entry does not parse must throw rather than quietly
35+
// apply only the CRLs that were read.
36+
const lines = fixtures.readKey('ca2-crl-agent3.pem', 'utf8').split('\n');
37+
lines[2] = 'AAAA' + lines[2].slice(4);
38+
assert.throws(() => {
39+
tls.createSecureContext({
40+
crl: fixtures.readKey('ca2-crl.pem', 'utf8') + lines.join('\n'),
41+
});
42+
}, { code: 'ERR_CRYPTO_OPERATION_FAILED' });

0 commit comments

Comments
 (0)