diff --git a/docs/releasenotes.md b/docs/releasenotes.md index 3983bd59b6..3d3cf3aebc 100644 --- a/docs/releasenotes.md +++ b/docs/releasenotes.md @@ -15,6 +15,7 @@ Date: 2026, TBD ### 2.1.2 Defects Fixed +- The CMS content-type decoders SignedData, EnvelopedData, AuthenticatedData, AuthEnvelopedData and EncryptedData read their mandatory fields by index or enumeration with no lower-bound size check, so a ContentInfo whose inner content was an empty or too-short SEQUENCE - malformed but DER-parseable - left the decode as an unchecked NoSuchElementException or ArrayIndexOutOfBoundsException rather than the CMSException the CMSSignedData, CMSEnvelopedData, CMSAuthenticatedData and CMSAuthEnvelopedData byte[]/InputStream constructors declare (the IllegalArgumentException CMSEncryptedData documents). Each now rejects a short sequence with IllegalArgumentException before reading, as the sibling content types CompressedData and DigestedData already did, so the wrappers surface it as their declared exception; the case that claims a leading OPTIONAL field and then truncates the mandatory ones is covered too. - A KeyAgreement asked for its shared secret before doPhase returned data rather than refusing. javax.crypto.KeyAgreement specifies IllegalStateException for that state, but nothing in the provider tracked it, so each SPI handed back whatever its result field held: for Diffie-Hellman that was the private value itself - engineInit seeded result with x, so generateSecret() returned the private exponent padded to the prime's length and generateSecret("AES") an all-zero key taken from that padding - while ECDH returned null and its named-algorithm overload raised NullPointerException. BaseAgreementSpi now records whether a doPhase has completed the agreement since the last init and refuses the request with an IllegalStateException naming the algorithm, so every family in the provider - DH, ECDH and ECMQV, the SM2 exchange, both ECGOST families, XDH, SM9 and NewHope - answers the same way, and the DH SPI no longer holds the private value in that field at all. - Mac.getInstance and KeyGenerator.getInstance by the HMAC SHA-512/224 and SHA-512/256 object identifiers (1.2.840.113549.2.12 and .13) failed, although the same algorithms resolved by name and the matching SecretKeyFactory aliases were registered: the SHA512 mappings called addHMACAlgorithm for the two truncated variants without the addHMACAlias that registers their OIDs against Mac and KeyGenerator. Both are now aliased, as every other HMAC in that class already was. - A KTSParameterSpec naming an HKDF key-derivation function with a parameters field - a form the provider does not service - was accepted at Cipher init and then failed out of wrap or unwrap with an unchecked IllegalStateException neither method declares. The KTS key-wrapping Ciphers (ML-KEM, Classic McEliece, FrodoKEM, the composite KEM and RSA-KEM) now validate the spec's KDF when they take it, reporting an unserviceable one as the InvalidAlgorithmParameterException engineInit declares, which is what the javax.crypto.KEM services already did through KdfUtil.resolveKemSpec. diff --git a/util/src/main/java/org/bouncycastle/asn1/cms/AuthEnvelopedData.java b/util/src/main/java/org/bouncycastle/asn1/cms/AuthEnvelopedData.java index 0390627021..e6fd5e67dc 100644 --- a/util/src/main/java/org/bouncycastle/asn1/cms/AuthEnvelopedData.java +++ b/util/src/main/java/org/bouncycastle/asn1/cms/AuthEnvelopedData.java @@ -91,6 +91,11 @@ public AuthEnvelopedData( private AuthEnvelopedData( ASN1Sequence seq) { + if (seq.size() < 4) + { + throw new IllegalArgumentException("Bad sequence size: " + seq.size()); + } + int index = 0; // "It MUST be set to 0." @@ -118,10 +123,19 @@ private AuthEnvelopedData( tmp = seq.getObjectAt(index++).toASN1Primitive(); authEncryptedContentInfo = EncryptedContentInfo.getInstance(tmp); + if (seq.size() <= index) + { + throw new IllegalArgumentException("Bad sequence size: " + seq.size()); + } + tmp = seq.getObjectAt(index++).toASN1Primitive(); if (tmp instanceof ASN1TaggedObject) { authAttrs = ASN1Set.getInstance((ASN1TaggedObject)tmp, false); + if (seq.size() <= index) + { + throw new IllegalArgumentException("Bad sequence size: " + seq.size()); + } tmp = seq.getObjectAt(index++).toASN1Primitive(); } else diff --git a/util/src/main/java/org/bouncycastle/asn1/cms/AuthenticatedData.java b/util/src/main/java/org/bouncycastle/asn1/cms/AuthenticatedData.java index c8e21d7008..a797fc00f0 100644 --- a/util/src/main/java/org/bouncycastle/asn1/cms/AuthenticatedData.java +++ b/util/src/main/java/org/bouncycastle/asn1/cms/AuthenticatedData.java @@ -84,6 +84,11 @@ public AuthenticatedData( private AuthenticatedData( ASN1Sequence seq) { + if (seq.size() < 5) + { + throw new IllegalArgumentException("Bad sequence size: " + seq.size()); + } + int index = 0; version = (ASN1Integer)seq.getObjectAt(index++); @@ -104,16 +109,29 @@ private AuthenticatedData( if (tmp instanceof ASN1TaggedObject) { digestAlgorithm = AlgorithmIdentifier.getInstance((ASN1TaggedObject)tmp, false); + if (seq.size() <= index) + { + throw new IllegalArgumentException("Bad sequence size: " + seq.size()); + } tmp = seq.getObjectAt(index++); } encapsulatedContentInfo = ContentInfo.getInstance(tmp); + if (seq.size() <= index) + { + throw new IllegalArgumentException("Bad sequence size: " + seq.size()); + } + tmp = seq.getObjectAt(index++); if (tmp instanceof ASN1TaggedObject) { authAttrs = ASN1Set.getInstance((ASN1TaggedObject)tmp, false); + if (seq.size() <= index) + { + throw new IllegalArgumentException("Bad sequence size: " + seq.size()); + } tmp = seq.getObjectAt(index++); } diff --git a/util/src/main/java/org/bouncycastle/asn1/cms/EncryptedData.java b/util/src/main/java/org/bouncycastle/asn1/cms/EncryptedData.java index 9422ed6732..a37c277c3e 100644 --- a/util/src/main/java/org/bouncycastle/asn1/cms/EncryptedData.java +++ b/util/src/main/java/org/bouncycastle/asn1/cms/EncryptedData.java @@ -69,6 +69,11 @@ public EncryptedData(EncryptedContentInfo encInfo, ASN1Set unprotectedAttrs) private EncryptedData(ASN1Sequence seq) { + if (seq.size() < 2) + { + throw new IllegalArgumentException("Bad sequence size: " + seq.size()); + } + this.version = ASN1Integer.getInstance(seq.getObjectAt(0)); this.encryptedContentInfo = EncryptedContentInfo.getInstance(seq.getObjectAt(1)); diff --git a/util/src/main/java/org/bouncycastle/asn1/cms/EnvelopedData.java b/util/src/main/java/org/bouncycastle/asn1/cms/EnvelopedData.java index b555b34d23..7b63bc088e 100644 --- a/util/src/main/java/org/bouncycastle/asn1/cms/EnvelopedData.java +++ b/util/src/main/java/org/bouncycastle/asn1/cms/EnvelopedData.java @@ -65,6 +65,11 @@ public EnvelopedData( private EnvelopedData( ASN1Sequence seq) { + if (seq.size() < 3) + { + throw new IllegalArgumentException("Bad sequence size: " + seq.size()); + } + int index = 0; version = (ASN1Integer)seq.getObjectAt(index++); @@ -79,6 +84,11 @@ private EnvelopedData( recipientInfos = ASN1Set.getInstance(tmp); + if (seq.size() <= index) + { + throw new IllegalArgumentException("Bad sequence size: " + seq.size()); + } + encryptedContentInfo = EncryptedContentInfo.getInstance(seq.getObjectAt(index++)); if (seq.size() > index) diff --git a/util/src/main/java/org/bouncycastle/asn1/cms/SignedData.java b/util/src/main/java/org/bouncycastle/asn1/cms/SignedData.java index 98039dfe81..2a07b2fe7e 100644 --- a/util/src/main/java/org/bouncycastle/asn1/cms/SignedData.java +++ b/util/src/main/java/org/bouncycastle/asn1/cms/SignedData.java @@ -246,6 +246,11 @@ private boolean checkForVersion3(ASN1Set signerInfs) private SignedData( ASN1Sequence seq) { + if (seq.size() < 3) + { + throw new IllegalArgumentException("Bad sequence size: " + seq.size()); + } + Enumeration e = seq.getObjects(); version = ASN1Integer.getInstance(e.nextElement()); diff --git a/util/src/test/java/org/bouncycastle/asn1/cms/test/ContentTypeSequenceSizeTest.java b/util/src/test/java/org/bouncycastle/asn1/cms/test/ContentTypeSequenceSizeTest.java new file mode 100644 index 0000000000..e422852774 --- /dev/null +++ b/util/src/test/java/org/bouncycastle/asn1/cms/test/ContentTypeSequenceSizeTest.java @@ -0,0 +1,138 @@ +package org.bouncycastle.asn1.cms.test; + +import org.bouncycastle.asn1.ASN1Encodable; +import org.bouncycastle.asn1.ASN1Integer; +import org.bouncycastle.asn1.ASN1ObjectIdentifier; +import org.bouncycastle.asn1.ASN1Primitive; +import org.bouncycastle.asn1.DEROctetString; +import org.bouncycastle.asn1.DERSequence; +import org.bouncycastle.asn1.DERSet; +import org.bouncycastle.asn1.DERTaggedObject; +import org.bouncycastle.asn1.cms.AuthEnvelopedData; +import org.bouncycastle.asn1.cms.AuthenticatedData; +import org.bouncycastle.asn1.cms.CMSObjectIdentifiers; +import org.bouncycastle.asn1.cms.ContentInfo; +import org.bouncycastle.asn1.cms.EncryptedData; +import org.bouncycastle.asn1.cms.EnvelopedData; +import org.bouncycastle.asn1.cms.SignedData; +import org.bouncycastle.asn1.x509.AlgorithmIdentifier; +import org.bouncycastle.util.test.SimpleTest; + +/** + * Confirms the CMS content-type decoders SignedData, EnvelopedData, AuthenticatedData, + * AuthEnvelopedData and EncryptedData reject a too-short SEQUENCE with IllegalArgumentException + * rather than reading their mandatory fields past the end and leaking a NoSuchElementException / + * ArrayIndexOutOfBoundsException - the sibling content types CompressedData and DigestedData + * already guard their size the same way. + */ +public class ContentTypeSequenceSizeTest + extends SimpleTest +{ + private static final ASN1ObjectIdentifier DATA = CMSObjectIdentifiers.data; + private static final AlgorithmIdentifier ALG = + new AlgorithmIdentifier(new ASN1ObjectIdentifier("2.16.840.1.101.3.4.1.2")); + + public String getName() + { + return "ContentTypeSequenceSizeTest"; + } + + public void performTest() + throws Exception + { + // a minimal well-formed EncryptedContentInfo: contentType, contentEncryptionAlgorithm + DERSequence encContentInfo = new DERSequence(new ASN1Encodable[]{ DATA, ALG }); + + // well-formed instances still parse + SignedData.getInstance(new DERSequence(new ASN1Encodable[]{ + new ASN1Integer(1), new DERSet(), new ContentInfo(DATA, null), new DERSet() })); + EnvelopedData.getInstance(new DERSequence(new ASN1Encodable[]{ + new ASN1Integer(0), new DERSet(), encContentInfo })); + EncryptedData.getInstance(new DERSequence(new ASN1Encodable[]{ + new ASN1Integer(0), encContentInfo })); + AuthenticatedData.getInstance(new DERSequence(new ASN1Encodable[]{ + new ASN1Integer(0), new DERSet(), ALG, new ContentInfo(DATA, null), + new DEROctetString(new byte[]{ 1, 2, 3, 4 }) })); + AuthEnvelopedData.getInstance(new DERSequence(new ASN1Encodable[]{ + new ASN1Integer(0), new DERSet(new DERSequence()), encContentInfo, + new DEROctetString(new byte[]{ 1, 2, 3, 4 }) })); + + // empty SEQUENCE is rejected by every content type + expectReject("SignedData empty", new SignedDataParse(), new DERSequence()); + expectReject("EnvelopedData empty", new EnvelopedDataParse(), new DERSequence()); + expectReject("AuthenticatedData empty", new AuthenticatedDataParse(), new DERSequence()); + expectReject("AuthEnvelopedData empty", new AuthEnvelopedDataParse(), new DERSequence()); + expectReject("EncryptedData empty", new EncryptedDataParse(), new DERSequence()); + + // one element short of the mandatory fields + expectReject("SignedData short", new SignedDataParse(), new DERSequence(new ASN1Encodable[]{ + new ASN1Integer(1), new DERSet() })); + expectReject("EnvelopedData short", new EnvelopedDataParse(), new DERSequence(new ASN1Encodable[]{ + new ASN1Integer(0), new DERSet() })); + expectReject("EncryptedData short", new EncryptedDataParse(), new DERSequence(new ASN1Encodable[]{ + new ASN1Integer(0) })); + + // a leading OPTIONAL field claimed but the mandatory fields then truncated - the case a + // single top-of-sequence size check would miss + expectReject("EnvelopedData originatorInfo truncated", new EnvelopedDataParse(), + new DERSequence(new ASN1Encodable[]{ + new ASN1Integer(0), new DERTaggedObject(false, 0, new DERSequence()), new DERSet() })); + expectReject("AuthenticatedData originatorInfo truncated", new AuthenticatedDataParse(), + new DERSequence(new ASN1Encodable[]{ + new ASN1Integer(0), new DERTaggedObject(false, 0, new DERSequence()), + new DERSet(), ALG })); + expectReject("AuthEnvelopedData originatorInfo truncated", new AuthEnvelopedDataParse(), + new DERSequence(new ASN1Encodable[]{ + new ASN1Integer(0), new DERTaggedObject(false, 0, new DERSequence()), + new DERSet(new DERSequence()), encContentInfo })); + } + + private void expectReject(String label, Parse parse, DERSequence malformed) + { + try + { + parse.run(malformed); + fail("malformed " + label + " not rejected"); + } + catch (IllegalArgumentException e) + { + // expected - the getInstance contract, not a leaked + // NoSuchElementException / ArrayIndexOutOfBoundsException + } + } + + private interface Parse + { + void run(ASN1Primitive seq); + } + + private static class SignedDataParse implements Parse + { + public void run(ASN1Primitive seq) { SignedData.getInstance(seq); } + } + + private static class EnvelopedDataParse implements Parse + { + public void run(ASN1Primitive seq) { EnvelopedData.getInstance(seq); } + } + + private static class AuthenticatedDataParse implements Parse + { + public void run(ASN1Primitive seq) { AuthenticatedData.getInstance(seq); } + } + + private static class AuthEnvelopedDataParse implements Parse + { + public void run(ASN1Primitive seq) { AuthEnvelopedData.getInstance(seq); } + } + + private static class EncryptedDataParse implements Parse + { + public void run(ASN1Primitive seq) { EncryptedData.getInstance(seq); } + } + + public static void main(String[] args) + { + runTest(new ContentTypeSequenceSizeTest()); + } +} diff --git a/util/src/test/java/org/bouncycastle/asn1/util/test/RegressionTest.java b/util/src/test/java/org/bouncycastle/asn1/util/test/RegressionTest.java index b7fa7778ad..31728d0684 100644 --- a/util/src/test/java/org/bouncycastle/asn1/util/test/RegressionTest.java +++ b/util/src/test/java/org/bouncycastle/asn1/util/test/RegressionTest.java @@ -38,6 +38,7 @@ import org.bouncycastle.asn1.cmp.test.PollReqContentTest; import org.bouncycastle.asn1.cms.test.AttributeTableUnitTest; import org.bouncycastle.asn1.cms.test.CMSTest; +import org.bouncycastle.asn1.cms.test.ContentTypeSequenceSizeTest; import org.bouncycastle.asn1.cms.test.SignerInfoTest; import org.bouncycastle.asn1.crmf.test.DhSigStaticTest; import org.bouncycastle.asn1.crmf.test.PKIPublicationInfoTest; @@ -110,6 +111,7 @@ public class RegressionTest new AttributeTableUnitTest(), new CMSTest(), new SignerInfoTest(), + new ContentTypeSequenceSizeTest(), new DhSigStaticTest(), new PKIPublicationInfoTest(), new CommitmentTypeIndicationUnitTest(),