Skip to content

Commit f7fc619

Browse files
committed
The certification path validators now find a certificate's entry in an indirect CRL by issuer as well as serial number, and look beyond the JDK's own serial number lookup, which sees only the CRL issuer's entries; CRL.isRevoked() no longer stops at another issuer's entry for the same serial number, and the jdk1.4 validators read an entry's certificate issuer through the class the provider's CRLs actually use, relates to github #2464.
1 parent 647b603 commit f7fc619

18 files changed

Lines changed: 884 additions & 157 deletions

File tree

‎CONTRIBUTORS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -532,7 +532,7 @@ We also wish to acknowledge financial and collaborative support from [CISCO](htt
532532
- subbudvk \<https://github.com/subbudvk\> - initial author on S2K parser hardening work for OpenPGP API.
533533
- mkarasik \<https://github.com/mkarasik\> - initial work on EST server-side key generation (RFC 7030 4.4).
534534
- Bernd Prünster (A-SIT Plus) \<bernd.pruenster&#064;a-sit.at\> - reported lenient ASN.1 UTCTime/GeneralizedTime parsing accepting structurally malformed content, with fuzzing-derived test cases. Additional fuzzing around date validity as well.
535-
- Naveed Khan \<https://github.com/rootvector2\> - constant time comparison of membership and confirmation tags in the MLS API. Reported unbounded array allocation in the BKS/UBER keystore load path (OutOfMemoryError DoS from a crafted keystore) and introduction of a capped decompression limit in PGPCompressedData.getDataStream(), both with POC. Original submission creating S/MIME backing temp files with owner-only permissions (PR #2326). Rejecting empty sequences in the X.509 extension parsers whose RFC 5280 syntax is SEQUENCE SIZE (1..MAX) - CRLDistPoint, CertificatePolicies, ExtendedKeyUsage, PolicyMappings, and SubjectDirectoryAttributes (PR #2331). Hardening asn1.cms.SignerInfo decode to reject a non-INTEGER version or non-tagged unsignedAttrs via getInstance rather than leaking a ClassCastException (PR #2342). Fixing an off-by-4 header-length guard in the OpenPGP NotationData signature subpacket parser (PR #2346). Rejecting CR/LF in the S/MIME streaming writer header names and values (SMIMEEnvelopedWriter/SMIMESignedWriter withHeader) to prevent MIME header injection (PR #2348). Adding minimum-length guards to the SM2 decrypt and GOST28147/DSTU7624/DESede/RC2 key-wrap unwrap paths (PR #2359). Guarding the ECDH session-key length in the JCE OpenPGP decryptor (PR #2383). Fixing the identity fast-path type guards in the OER getInstance factories and the transposed isInstance arguments in OEROptional.getObject (PR #2373). Requiring the signature handed to AIMerSigner.verifySignature to be exactly the parameter set's signature size (PR #2401). Constant time comparison of the SRP-6a evidence messages (PR #2406). Surfacing a malformed TSTInfo in a time-stamp token as TSPException rather than an unchecked exception out of the TimeStampResponse constructors (PR #2415). Rejecting a PKCS#12 PBMAC1 KDF keyLength below the RFC 9579 sec. 9 floor of 20 octets (PR #2431). Initial implementation of the empty-label guard on name-constraint host matching, having found that the canonicalisation removed only one RFC 1034 root-label dot, so a dNSName, rfc822Name host or URI host carrying more than one matched no constraint at all (PR #2436).
535+
- Naveed Khan \<https://github.com/rootvector2\> - constant time comparison of membership and confirmation tags in the MLS API. Reported unbounded array allocation in the BKS/UBER keystore load path (OutOfMemoryError DoS from a crafted keystore) and introduction of a capped decompression limit in PGPCompressedData.getDataStream(), both with POC. Original submission creating S/MIME backing temp files with owner-only permissions (PR #2326). Rejecting empty sequences in the X.509 extension parsers whose RFC 5280 syntax is SEQUENCE SIZE (1..MAX) - CRLDistPoint, CertificatePolicies, ExtendedKeyUsage, PolicyMappings, and SubjectDirectoryAttributes (PR #2331). Hardening asn1.cms.SignerInfo decode to reject a non-INTEGER version or non-tagged unsignedAttrs via getInstance rather than leaking a ClassCastException (PR #2342). Fixing an off-by-4 header-length guard in the OpenPGP NotationData signature subpacket parser (PR #2346). Rejecting CR/LF in the S/MIME streaming writer header names and values (SMIMEEnvelopedWriter/SMIMESignedWriter withHeader) to prevent MIME header injection (PR #2348). Adding minimum-length guards to the SM2 decrypt and GOST28147/DSTU7624/DESede/RC2 key-wrap unwrap paths (PR #2359). Guarding the ECDH session-key length in the JCE OpenPGP decryptor (PR #2383). Fixing the identity fast-path type guards in the OER getInstance factories and the transposed isInstance arguments in OEROptional.getObject (PR #2373). Requiring the signature handed to AIMerSigner.verifySignature to be exactly the parameter set's signature size (PR #2401). Constant time comparison of the SRP-6a evidence messages (PR #2406). Surfacing a malformed TSTInfo in a time-stamp token as TSPException rather than an unchecked exception out of the TimeStampResponse constructors (PR #2415). Rejecting a PKCS#12 PBMAC1 KDF keyLength below the RFC 9579 sec. 9 floor of 20 octets (PR #2431). Initial implementation of the empty-label guard on name-constraint host matching, having found that the canonicalisation removed only one RFC 1034 root-label dot, so a dNSName, rfc822Name host or URI host carrying more than one matched no constraint at all (PR #2436). Initial implementation of the lookup of indirect CRL entries by issuer and serial number, having found that the certification path validators compared only the issuer of the first entry carrying a certificate's serial number (PR #2464).
536536
- suraj0208 \<https://github.com/suraj0208\> - initial work on auto-detecting private key reader (JcaPrivateKeyReader).
537537
- liamgilligan \<https://github.com/liamgilligan\> - noticing the BIP-340 step numbering in the BIP340Signer signing comments was incorrect (PR #2340).
538538
- digi-scrypt \<https://github.com/digi-scrypt\> - disabling DTD and external-entity resolution in KMIPInputStream to close an XXE (local file disclosure / SSRF) exposure in KMIP XML parsing (PR #2315).

‎ant/jdk13.xml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,7 @@
477477
<exclude name="**/cert/ocsp/test/AllTests.java"/>
478478
<exclude name="**/cert/plants/**/*.java"/>
479479
<exclude name="**/cert/test/X509CertificateReviewerTest.java"/>
480+
<exclude name="**/cert/test/IndirectCRLSerialCollisionTest.java"/>
480481
<exclude name="**/pkix/test/QcStatementReviewerTest.java"/>
481482
<exclude name="**/cert/ocsp/test/OCSPExceptionalSignatureRejectionTest.java"/>
482483
<exclude name="**/cert/ocsp/test/PKIXOcspRevocationCheckerTest.java"/>
@@ -521,6 +522,8 @@
521522
<fileset dir="mail/src/main/jdk1.4" includes="**/*.java" />
522523
<fileset dir="prov/src/main/jdk1.4" includes="**/*.java" >
523524
<exclude name="**/LDAP*.java" />
525+
<!-- jdk1.4 validator bridge: the jdk1.3 validators and CRL entries have overlays of their own -->
526+
<exclude name="**/asymmetric/x509/X509CRLEntryIssuer.java" />
524527
<exclude name="**/X509LDAP*.java" />
525528
<exclude name="**/rsa/PSSParamSpec.java" />
526529
</fileset>

‎ant/jdk14.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,7 @@
238238
<exclude name="**/cert/ocsp/test/AllTests.java"/>
239239
<exclude name="**/cert/plants/**/*.java"/>
240240
<exclude name="**/cert/test/X509CertificateReviewerTest.java"/>
241+
<exclude name="**/cert/test/IndirectCRLSerialCollisionTest.java"/>
241242
<exclude name="**/pkix/test/QcStatementReviewerTest.java"/>
242243
<exclude name="**/pkix/test/RevocationTest.java"/>
243244
<exclude name="**/TimeStampTokenInfoUnitTest.java"/>

‎docs/releasenotes.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ Date: 2026, TBD
2828
- An ERSDataGroup recomputed its hash on every request rather than taking it from the cache ERSCachingData exists to provide: the group overrode getHash(), which left the calculateHash() the cache calls unreachable - and wrong, as it copied the member hashes with a loop bounded by the size of the empty list it was copying into, so it would have digested nothing had anything reached it. The computation is back in calculateHash() and the override is gone, so a group's hash is computed once per digest algorithm and previous-chain hash, as every other ERSData's is. The value itself is unchanged.
2929
- ERSArchiveTimeStampGenerator rebuilt its reduced hash tree from the data objects on every call, so the usual generateTimeStampRequest() followed by generateArchiveTimeStamp() or generateArchiveTimeStamps() built it twice. The leaves are now built once and dropped when data or a previous chain is added. A Set of the data groups it had been given, which nothing ever read, has gone with it.
3030
- Grain-128AEAD returned corrupted plaintext from a decryption driven in chunks. The stream cipher data operator splits a processBytes() call that spans the buffered authentication tag into two output segments, the bytes released from the tag buffer and then the bytes taken straight from the caller's input, and wrote the second segment at the caller's output offset instead of after the first, so the second segment overwrote the head of the first and the tail of the reported output was never written at all. The call still returned the full byte count, and because the engine's state update depends on the input and the keystream rather than on where the output lands, the tag still verified: the wrong plaintext came back with no error raised. Any chunk after the first that carried more than the 8 byte tag length was affected. Grain-128AEAD is the only engine that uses this operator, and one shot decryption, the encryption path and every other AEAD engine were unaffected. The second segment is now written at the advanced offset, matching the equivalent step of the general decryption path (github PR #2447).
31+
- The certificate path validators looked a certificate up in an indirect CRL by serial number alone and then compared the issuer of whichever entry came first, so where two issuers had each revoked the same serial number - an indirect CRL (RFC 5280 sec. 5.2.5) lists certificates from more than one issuer, and a serial number is only unique within its issuer - a certificate whose own entry followed the other issuer's was reported as not revoked. The BC provider's PKIX validator, the pkix X509RevocationChecker and the legacy org.bouncycastle.x509 validator now check every entry carrying the serial number against the issuer it applies to, the one named by its certificateIssuer extension or inherited from the entries before it (sec. 5.3.3), and the provider's X509CRL implements getRevokedCertificate(X509Certificate), the JDK's lookup for indirect CRLs, the same way rather than inheriting the default that assumes a single issuer. CRL.isRevoked() on the provider's CRLs had the same fault, stopping at the first entry carrying the serial number, and now does the same. The validators also missed, whatever the serial numbers, any revocation listed for an issuer other than the CRL issuer when the CRL had been parsed by the JDK's own provider, whose getRevokedCertificate(BigInteger) looks only at the CRL issuer's entries; they now look further whenever that lookup cannot be conclusive. In the jdk1.4 distribution the two provider validators read an entry's certificate issuer through a cast to a class the provider's own CRLs do not use, so an indirect CRL carrying an entry with the certificate's serial number failed validation with no reason given, whether or not that entry was the certificate's; they now share the lookup above.
3132
- The AEAD stream cipher data operator, which Grain-128AEAD alone uses, wrote its output without first checking that the caller's buffer was long enough, so a short output buffer surfaced as an ArrayIndexOutOfBoundsException from inside the engine rather than as the OutputLengthException the general path reports for every other AEAD engine. Both directions of processBytes(), and processByte(), now check before anything is written or buffered, and only when the call releases output, as the general path does.
3233
- The RFC 9709 content-encryption AlgorithmIdentifier, which carries the real algorithm inside the parameters of an outer id-alg-cek-hkdf-sha256, was unwrapped at only one of the points where a CMS recipient makes a decision about it. Key-size validation was corrected for plain key transport in 1.86, but the same call in the KEK, RSA-KTS and KEM recipients, and in the key-transport recipient's own ORI-KEM branch, still compared the recovered key against the outer identifier, which registers no key size, so setKeySizeValidation(true) silently checked nothing there; the setAllowedContentAlgorithms allow-list and the setMinimumTagSize floor were applied to the outer identifier on every recipient family, including the one already corrected, so neither constrained an RFC 9709 message. The unwrap now happens once for the key-size check and once for the two policy checks, and every recipient polices and validates the content-encryption algorithm the message actually carries. A recipient with no allow-list, no tag floor and no key-size validation configured behaves exactly as before; a caller who listed id-alg-cek-hkdf-sha256 in an allow-list in order to admit RFC 9709 messages must now list the content-encryption algorithms themselves (github PR #2446).
3334
- A CMS message whose EncryptedContentInfo named the RFC 9709 key derivation but carried no readable content-encryption AlgorithmIdentifier in its parameters was reported as a NullPointerException, or as an IllegalArgumentException from the ASN.1 decoder, out of methods declared to throw CMSException, RecipientInformation.getContent() among them. The four places that resolve the wrapper - the CEK derivation, the content cipher selection, the key-size check, and the recipient's allowed-algorithm and tag-size checks - now share one resolver, which reports an absent or unreadable inner algorithm as a CMSException.

‎pkix/src/main/java/org/bouncycastle/pkix/jcajce/CertPathValidatorUtilities.java‎

Lines changed: 78 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -403,36 +403,12 @@ protected static void getCertStatus(
403403

404404
if (isIndirect)
405405
{
406-
crl_entry = crl.getRevokedCertificate(getSerialNumber(cert));
406+
crl_entry = getIndirectCRLEntry(crl, getSerialNumber(cert), getEncodedIssuerPrincipal(cert));
407407

408408
if (crl_entry == null)
409409
{
410410
return;
411411
}
412-
413-
X500Principal certIssuer;
414-
try
415-
{
416-
certIssuer = crl_entry.getCertificateIssuer();
417-
}
418-
catch (RuntimeException e)
419-
{
420-
// getCertificateIssuer() builds a new X500Principal from the entry's certificateIssuer
421-
// DN, which can throw an unchecked IllegalArgumentException on a name that decodes
422-
// structurally but is semantically invalid. Fail closed with the checked contract type
423-
// rather than let it escape (or swallow it to null, which would fail revocation open).
424-
throw new AnnotatedException("CRL entry certificate issuer could not be parsed.", e);
425-
}
426-
427-
if (certIssuer == null)
428-
{
429-
certIssuer = getIssuerPrincipal(crl);
430-
}
431-
432-
if (!getEncodedIssuerPrincipal(cert).equals(certIssuer))
433-
{
434-
return;
435-
}
436412
}
437413
else if (!getEncodedIssuerPrincipal(cert).equals(getIssuerPrincipal(crl)))
438414
{
@@ -479,6 +455,83 @@ else if (!getEncodedIssuerPrincipal(cert).equals(getIssuerPrincipal(crl)))
479455
}
480456
}
481457

458+
/**
459+
* Find the entry of an indirect CRL for the certificate with the given serial number and issuer.
460+
* An indirect CRL (RFC 5280 sec. 5.2.5) lists certificates from more than one issuer and a serial
461+
* number is only unique within its issuer, so the entry cannot be located by serial number alone:
462+
* an entry carrying the serial number is the certificate's only if it applies to the certificate's
463+
* issuer, the one named by its certificateIssuer extension or inherited from the entries before it
464+
* (sec. 5.3.3). The lookup by serial number is tried first, as it avoids building the set of all
465+
* entries, and every entry carrying the serial number is checked when that is not conclusive.
466+
*
467+
* @return the entry naming the certificate, or null if the CRL carries none.
468+
*/
469+
private static X509CRLEntry getIndirectCRLEntry(X509CRL crl, BigInteger serialNumber, X500Principal certIssuer)
470+
throws AnnotatedException
471+
{
472+
X509CRLEntry entry = crl.getRevokedCertificate(serialNumber);
473+
if (entry == null ? isIssuerBlindLookup(crl) : certIssuer.equals(getEntryIssuer(crl, entry)))
474+
{
475+
return entry;
476+
}
477+
478+
Set entries = crl.getRevokedCertificates();
479+
if (entries == null)
480+
{
481+
return null;
482+
}
483+
484+
for (Iterator it = entries.iterator(); it.hasNext();)
485+
{
486+
entry = (X509CRLEntry)it.next();
487+
if (serialNumber.equals(entry.getSerialNumber()) && certIssuer.equals(getEntryIssuer(crl, entry)))
488+
{
489+
return entry;
490+
}
491+
}
492+
493+
return null;
494+
}
495+
496+
/**
497+
* The BC CRL's getRevokedCertificate(BigInteger) returns the first entry with the serial number
498+
* whichever issuer it applies to, so a null from it means no entry carries the serial number. The
499+
* JDK's looks only at the entries of the CRL issuer, so a null from it says nothing about those of
500+
* the other issuers of an indirect CRL.
501+
*/
502+
private static boolean isIssuerBlindLookup(X509CRL crl)
503+
{
504+
return crl.getClass().getName().startsWith("org.bouncycastle.jcajce.provider.asymmetric.x509.");
505+
}
506+
507+
/**
508+
* Return the issuer of the certificate an entry of an indirect CRL applies to.
509+
*/
510+
private static X500Principal getEntryIssuer(X509CRL crl, X509CRLEntry entry)
511+
throws AnnotatedException
512+
{
513+
X500Principal certificateIssuer;
514+
try
515+
{
516+
certificateIssuer = entry.getCertificateIssuer();
517+
}
518+
catch (RuntimeException e)
519+
{
520+
// getCertificateIssuer() builds a new X500Principal from the entry's certificateIssuer
521+
// DN, which can throw an unchecked IllegalArgumentException on a name that decodes
522+
// structurally but is semantically invalid. Fail closed with the checked contract type
523+
// rather than let it escape (or swallow it to null, which would fail revocation open).
524+
throw new AnnotatedException("CRL entry certificate issuer could not be parsed.", e);
525+
}
526+
527+
if (certificateIssuer == null)
528+
{
529+
return getIssuerPrincipal(crl);
530+
}
531+
532+
return certificateIssuer;
533+
}
534+
482535
/**
483536
* Return the next working key inheriting DSA parameters if necessary.
484537
* <p>

‎pkix/src/test/java/org/bouncycastle/cert/test/AllTests.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ public void testSimpleTests()
3535
new GOST3410_2012CMSTest(),
3636
new GOSTR3410_2012_256GenerateCertificate(),
3737
new IndirectCRLSignerTest(),
38+
new IndirectCRLSerialCollisionTest(),
3839
new MLDSACredentialsTest(),
3940
new PKCS10Test(),
4041
new RelatedCertificateDescriptorTest(),

0 commit comments

Comments
 (0)