Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ public static CRLDistPoint fromExtensions(Extensions extensions)
private CRLDistPoint(
ASN1Sequence seq)
{
if (seq.size() < 1)
{
throw new IllegalArgumentException("sequence may not be empty");
}

this.seq = seq;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ public CertificatePolicies(
private CertificatePolicies(
ASN1Sequence seq)
{
if (seq.size() < 1)
{
throw new IllegalArgumentException("sequence may not be empty");
}

this.policyInformation = new PolicyInformation[seq.size()];

for (int i = 0; i != seq.size(); i++)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ public ExtendedKeyUsage(
private ExtendedKeyUsage(
ASN1Sequence seq)
{
if (seq.size() < 1)
{
throw new IllegalArgumentException("sequence may not be empty");
}

this.seq = seq;

Enumeration e = seq.getObjects();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ public static PolicyMappings getInstance(Object obj)
*/
private PolicyMappings(ASN1Sequence seq)
{
if (seq.size() < 1)
{
throw new IllegalArgumentException("sequence may not be empty");
}

this.seq = seq;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ public static SubjectDirectoryAttributes getInstance(
*/
private SubjectDirectoryAttributes(ASN1Sequence seq)
{
if (seq.size() < 1)
{
throw new IllegalArgumentException("sequence may not be empty");
}

Enumeration e = seq.getObjects();

while (e.hasMoreElements())
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
package org.bouncycastle.asn1.test;

import java.io.IOException;
import java.util.Vector;

import org.bouncycastle.asn1.ASN1Encodable;
import org.bouncycastle.asn1.ASN1ObjectIdentifier;
import org.bouncycastle.asn1.DERSequence;
import org.bouncycastle.asn1.DERSet;
import org.bouncycastle.asn1.DERUTF8String;
import org.bouncycastle.asn1.x509.AccessDescription;
import org.bouncycastle.asn1.x509.Attribute;
import org.bouncycastle.asn1.x509.AuthorityInformationAccess;
import org.bouncycastle.asn1.x509.CRLDistPoint;
import org.bouncycastle.asn1.x509.CertPolicyId;
import org.bouncycastle.asn1.x509.CertificatePolicies;
import org.bouncycastle.asn1.x509.DistributionPoint;
import org.bouncycastle.asn1.x509.DistributionPointName;
import org.bouncycastle.asn1.x509.ExtendedKeyUsage;
import org.bouncycastle.asn1.x509.GeneralName;
import org.bouncycastle.asn1.x509.GeneralNames;
import org.bouncycastle.asn1.x509.KeyPurposeId;
import org.bouncycastle.asn1.x509.PolicyInformation;
import org.bouncycastle.asn1.x509.PolicyMappings;
import org.bouncycastle.asn1.x509.SubjectDirectoryAttributes;
import org.bouncycastle.util.test.SimpleTest;

/**
* The X.509 extension types whose RFC 5280 syntax is SEQUENCE SIZE (1..MAX)
* must reject an empty SEQUENCE on parse, matching AuthorityInformationAccess.
*/
public class EmptyExtensionSequenceTest
extends SimpleTest
{
private static final ASN1ObjectIdentifier anyPolicy = new ASN1ObjectIdentifier("2.5.29.32.0");

public String getName()
{
return "EmptyExtensionSequence";
}

public void performTest()
throws IOException
{
emptyRejected();
nonEmptyParses();
}

private void emptyRejected()
{
DERSequence empty = new DERSequence();

rejectEmpty("CertificatePolicies", new Parse()
{
public void parse() { CertificatePolicies.getInstance(empty); }
});
rejectEmpty("PolicyMappings", new Parse()
{
public void parse() { PolicyMappings.getInstance(empty); }
});
rejectEmpty("ExtendedKeyUsage", new Parse()
{
public void parse() { ExtendedKeyUsage.getInstance(empty); }
});
rejectEmpty("CRLDistPoint", new Parse()
{
public void parse() { CRLDistPoint.getInstance(empty); }
});
rejectEmpty("SubjectDirectoryAttributes", new Parse()
{
public void parse() { SubjectDirectoryAttributes.getInstance(empty); }
});
// already enforced before this change - guards the family stays consistent
rejectEmpty("AuthorityInformationAccess", new Parse()
{
public void parse() { AuthorityInformationAccess.getInstance(empty); }
});
}

private void rejectEmpty(String name, Parse p)
{
try
{
p.parse();
fail("empty " + name + " accepted");
}
catch (IllegalArgumentException e)
{
isTrue("unexpected message for " + name + ": " + e.getMessage(),
e.getMessage().indexOf("sequence may not be empty") >= 0);
}
}

private void nonEmptyParses()
{
// each built non-empty and re-parsed through the ASN1Sequence constructor
CertificatePolicies.getInstance(
new CertificatePolicies(new PolicyInformation(anyPolicy)).toASN1Primitive());

PolicyMappings.getInstance(
new PolicyMappings(CertPolicyId.getInstance(anyPolicy),
CertPolicyId.getInstance(anyPolicy)).toASN1Primitive());

ExtendedKeyUsage.getInstance(
new ExtendedKeyUsage(KeyPurposeId.id_kp_serverAuth).toASN1Primitive());

DistributionPoint dp = new DistributionPoint(
new DistributionPointName(new GeneralNames(
new GeneralName(GeneralName.uniformResourceIdentifier, "http://crl.example/c.crl"))),
null, null);
CRLDistPoint.getInstance(
new CRLDistPoint(new DistributionPoint[]{ dp }).toASN1Primitive());

AuthorityInformationAccess.getInstance(
new AuthorityInformationAccess(AccessDescription.id_ad_caIssuers,
new GeneralName(GeneralName.uniformResourceIdentifier, "http://ca.example/ca")).toASN1Primitive());

Vector attrs = new Vector();
attrs.addElement(new Attribute(new ASN1ObjectIdentifier("2.5.4.3"),
new DERSet(new DERUTF8String("name"))));
SubjectDirectoryAttributes.getInstance(
new SubjectDirectoryAttributes(attrs).toASN1Primitive());
}

private interface Parse
{
void parse();
}

public static void main(String[] args)
{
runTest(new EmptyExtensionSequenceTest());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public class RegressionTest
new TargetInformationTest(),
new SubjectKeyIdentifierTest(),
new AuthorityKeyIdentifierTest(),
new EmptyExtensionSequenceTest(),
new TBSCertListTest(),
new TBSCertificateIssuerTest(),
new AttributeCertificateInfoIssuerTest(),
Expand Down
1 change: 1 addition & 0 deletions docs/releasenotes.html
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ <h3>2.1.2 Defects Fixed</h3>
<li>The composite (draft-ietf-lamps-pq-composite-kem / -sig) KEM and signature parsers split attacker-controlled key and signature bytes at fixed component offsets without first checking the input was long enough. A crafted composite public key, private key or signature with a truncated body therefore raised an uncaught NegativeArraySizeException / ArrayIndexOutOfBoundsException / IllegalArgumentException out of KeyFactory.generatePublic / generatePrivate (reachable via BouncyCastleProvider.getPublicKey / getPrivateKey when parsing a certificate or PKCS#8) and out of Signature.verify -- a parse-time denial of service on untrusted input that escaped the methods' declared IOException / SignatureException contracts. compositekem.KeyFactorySpi (public- and private-key paths), compositesignatures.SignatureSpi.engineVerify and CompositeMLKEMEngine decapsulation now bound-check the input length first and reject a too-short body with a diagnosable IOException / SignatureException, matching the strict-parse hardening applied elsewhere (e.g. the CVE-2024-0727 PKCS#12 fix).</li>
<li>Three further secret-comparison sites were switched from the variable-time Arrays.areEqual to the constant-time Arrays.constantTimeAreEqual, matching the bc-java convention for secret-bearing comparisons: the DSTU 7624 (Kalyna) key-unwrap integrity check in DSTU7624WrapEngine (the only wrap engine still using the variable-time compare; RFC3394WrapEngine / RFC5649WrapEngine / DESedeWrapEngine / RC2WrapEngine / RFC3211WrapEngine already use the constant-time form), and the legacy stateful-hash PQC private-key equals() implementations in BCXMSSPrivateKey, BCXMSSMTPrivateKey and BCSphincs256PrivateKey (the un-migrated siblings of the ML-DSA / ML-KEM / SLH-DSA private-key equals() hardened earlier in this release). The boolean / thrown results are unchanged.</li>
<li>X509CertificateHolder and X509CRLHolder, when constructed from a byte[] / InputStream, caught only ClassCastException and IllegalArgumentException from the ASN.1 decode and wrapped them in a CertIOException; other RuntimeExceptions thrown on malformed input -- ASN1ParsingException / IllegalStateException from the lazy-sequence and tagged-object decoders, and a NullPointerException reachable in the certificate path -- escaped a constructor that declares only IOException, so a caller catching IOException on untrusted input could still receive an uncaught RuntimeException (a parse-time robustness failure surfaced by fuzzing). Both parse helpers now treat any RuntimeException from the decode as malformed input and wrap it in a CertIOException carrying the same "malformed data: ..." message. Valid encodings are unaffected (80,000 mutated CRL/certificate inputs all surfaced as IOException).</li>
<li>The X.509 extension types whose RFC 5280 syntax is SEQUENCE SIZE (1..MAX) -- CertificatePolicies (sec. 4.2.1.4), PolicyMappings (sec. 4.2.1.5), ExtendedKeyUsage (sec. 4.2.1.12), CRLDistPoint / FreshestCRL (sec. 4.2.1.13, 5.2.6) and SubjectDirectoryAttributes (sec. 4.2.1.8) -- accepted an empty SEQUENCE on parse, decoding a structurally invalid extension from an untrusted certificate as a valid but content-free extension instead of rejecting it. AuthorityInformationAccess (and RelatedCertificateDescriptor) already rejected this case with "sequence may not be empty"; the getInstance parse path of these sibling types now applies the same check, so an empty extension fails fast with an IllegalArgumentException. Non-empty extensions are unaffected.</li>
<li>PKCS12KeyStoreSpi.engineLoad and its RFC 9579 PBMAC1 SPI pair (PKCS12PBMAC1KeyStoreSpi) parsed the (untrusted) PKCS#12 safe contents -- AuthenticatedSafe / SafeBag / CertBag decode, ASN1OctetString.getInstance, embedded certificate generation -- without catching the RuntimeExceptions those ASN.1 operations throw on malformed input, so a crafted .p12 could surface an uncaught IllegalArgumentException (e.g. "illegal object in getInstance") or a bare RuntimeException out of KeyStore.load, escaping the method's declared IOException contract (a parse-time robustness failure for callers catching IOException; the same class as the X509CertificateHolder / X509CRLHolder fix above). The safe-contents processing in both SPIs is now wrapped so any RuntimeException from the decode is re-thrown via Exceptions.ioException as an IOException. Valid keystores are unaffected.</li>
<li>BCPBEKey.destroy() -- the JCE PBEKey returned by the BC SecretKeyFactory PBE / PBKDF2 / scrypt key factories -- zeroized the password and salt but left the derived key bytes, the actual secret, in memory: the derived key is held in the CipherParameters param field (a KeyParameter, or a KeyParameter wrapped in ParametersWithIV) and was never cleared, so a heap dump taken after a caller had dutifully called destroy() still contained the derived key. destroy() now also overwrites the derived key held in param (recursing through ParametersWithIV to reach the wrapped KeyParameter), honouring the Destroyable contract; behaviour before destroy() and the post-destroy IllegalStateException from getEncoded()/getParam() are unchanged.</li>
<li>Three provider / cache data races were closed. BouncyCastleProvider.getKeyInfoConverter and BouncyCastlePQCProvider.getKeyInfoConverter read the shared static keyInfoConverters HashMap with no synchronization, while addKeyInfoConverter and the sibling getAsymmetricKeyInfoConverter both hold the map's monitor; a getKeyInfoConverter call racing an addKeyInfoConverter (for example a second provider construction, which re-runs the converter registration, concurrent with a key decode on another thread) could observe a partially-rehashed table and return a wrong or null converter. Both reads now take the same monitor as the write. Separately, OcspCache.getOcspResponse mutated its per-responder inner HashMap (get / put / remove) with no synchronization, so two threads validating certificates from the same OCSP responder could structurally corrupt that map; the method is now static synchronized, matching its sibling CrlCache.getCrl. No behavioural change for single-threaded callers.</li>
Expand Down