Skip to content

Commit 9eadfc9

Browse files
committed
The seven OpenPGP signature subpackets whose body RFC 9580 defines as a fixed length now all enforce that length where the subpacket is parsed rather than deferring it to an accessor, only Signature Expiration Time having done so, since SignatureSubpacketInputStream.readPacket validated a declared body length for range and for the case where it overran the subpacket area but handed a body whose declared length matched the octets it carried to the constructor whatever that length was, so Signature Creation Time and Key Expiration Time reported a body that was not the four octets of a time field, Exportable Certification, Revocable and Primary User ID a body that was not the single zero or one octet of a flag, and Issuer Key ID one shorter than a key ID, as an unchecked IllegalStateException out of org.bouncycastle.bcpg.sig.Utils or an IllegalArgumentException out of FingerprintUtil.readKeyID when the value was read, which matters because such a body inside a self-signature is content the signer signed over, so a certificate carrying one verified as valid and failed only later in ordinary reader code - PGPPublicKey.getValidSeconds(), PGPSignatureSubpacketVector.getKeyExpirationTime() and isExportable(), OpenPGPCertificate.getExpirationTime(), and the keyserver re-export path PGPPublicKeyRing.encode(out, true), which declares IOException - and the length and flag-value checks now sit as shared statics beside the accessors they protect, with Signature Expiration Time's own check folded into them, a malformed subpacket refused at parse time as a MalformedPacketException the way Features, Trust Signature and Notation Data already were, and the parser's tolerance of a fixed-length field whose declared length overruns the subpacket area preserved, BytesBooleansTest being wired into the openpgp suite it had never been registered in, relates to github #2426.
1 parent a5e172b commit 9eadfc9

13 files changed

Lines changed: 198 additions & 28 deletions

File tree

‎CONTRIBUTORS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -542,7 +542,7 @@ We also wish to acknowledge financial and collaborative support from [CISCO](htt
542542
- jmeeder \<https://github.com/jmeeder\> - reporting that RFC 4998 evidence-record generation rejected time stamps from an authority naming the digest with NULL parameters where BC names it with them absent, both of which RFC 5754 requires a receiver to accept (issue #2379).
543543
- rimuln \<https://github.com/rimuln\> - diagnosis and fix for PKCS12 getCertificateAlias returning the alias of an unrelated certificate, tracing it to the alias and certificate enumerations of the keystore's certs table diverging in order once keys() enumerated a copy (issue #2384, PR #2385).
544544
- Yu Bao \<yubao&#064;paypal.com\> - reporting an API gap, on behalf of the PayPal Cyber Security Team, that the high-level OpenPGP message API (OpenPGPMessageProcessor / OpenPGPMessageInputStream) gave a caller no way to bound how far a compressed data packet expands, where the low-level PGPCompressedData it wraps has carried a bounded getDataStream(long) overload all along, and that OpenPGPPolicy exposed no equivalent property to set. Suggesting a protocol whitelist for CRL Distribution Point fetching, which is now the org.bouncycastle.x509.CRLDP_protocols property, and suggesting that an OCSP response was read up to the length the responder declared for itself, now capped by org.bouncycastle.ocsp.max_response_size, and suggesting bounds on the OpenPGP ASCII armor headers, now capped by org.bouncycastle.openpgp.max_armor_header_length and org.bouncycastle.openpgp.max_armor_headers.
545-
- Arpan Sharma \<https://github.com/Arpan0995\> - initial audit of BCPQC provider consistency starting with HQC, which led to the exposure of a number of issues in the JCA provider service interfaces for other BCPQC algorithms. In-depth auditing of PQC signature algorithms in the provider leading to the correction of a number of JCA API compliance issues. Initial implementation of the guard that lets a signature context be set on the composite ML-DSA services before initSign / initVerify, the composite counterpart of the base-engine fix for issue #2396 (issue #2412). Audit of the LMS / HSS stateful private key decoder, establishing that the level count, the one-time index and the persisted tree cache were all accepted without validation (issue #2414); that issue was also the inspiration for the further investigation which found the HSS and XMSS^MT decoders accepting a private key whose declared index disagreed with the traversal state stored beside it, allowing a one-time key to be used twice. Audit of certificate evaluation in the high-level OpenPGP API, which found a signing subkey escaping the cross-certification requirement and a version 6 key being used without the Direct Key signature RFC 9580 requires. Audit of the recipient side of the CMS RFC 9629 KEMRecipientInfo path, which found three sender-controlled fields leaving methods declared to throw CMSException as unchecked exceptions and the kekLength never being confirmed against the wrap algorithm, and which led to the same translation being added to the neighbouring AuthEnvelopedData constructor and the two streaming parsers (issue #2422). Audit of RSA-PSS parameter handling in the provider, which found Signature.setParameter reporting a rejected trailer field as an unchecked exception and leaving the rejected spec half-applied, so that the parameters reported for a signature were not the ones it had been made with (issue #2421). Audit of the PKCS#12 key store entry-setting boundary, which found setKeyEntry reporting a certificate whose public key the provider cannot resolve as an unchecked exception rather than the KeyStoreException it declares, and which led to the same guard being applied to setCertificateEntry and to the RFC 9579 PBMAC1 store, and to the rejected entry no longer being left behind in the store (issue #2419).
545+
- Arpan Sharma \<https://github.com/Arpan0995\> - initial audit of BCPQC provider consistency starting with HQC, which led to the exposure of a number of issues in the JCA provider service interfaces for other BCPQC algorithms. In-depth auditing of PQC signature algorithms in the provider leading to the correction of a number of JCA API compliance issues. Initial implementation of the guard that lets a signature context be set on the composite ML-DSA services before initSign / initVerify, the composite counterpart of the base-engine fix for issue #2396 (issue #2412). Audit of the LMS / HSS stateful private key decoder, establishing that the level count, the one-time index and the persisted tree cache were all accepted without validation (issue #2414); that issue was also the inspiration for the further investigation which found the HSS and XMSS^MT decoders accepting a private key whose declared index disagreed with the traversal state stored beside it, allowing a one-time key to be used twice. Audit of the high-level OpenPGP API (org.bouncycastle.openpgp, org.bouncycastle.openpgp.api) and the bcpg packet layer beneath it, spanning certificate evaluation, message verification, decryption and signature subpacket parsing, which found certificates being used without the certifications RFC 9580 requires of them, malformed but signed content reaching ordinary reader code as unchecked exceptions, a truncated message being read as a clean end of message, and unverified plaintext being released ahead of the integrity check (issues #2417, #2424, #2426). Audit of the recipient side of the CMS RFC 9629 KEMRecipientInfo path, which found three sender-controlled fields leaving methods declared to throw CMSException as unchecked exceptions and the kekLength never being confirmed against the wrap algorithm, and which led to the same translation being added to the neighbouring AuthEnvelopedData constructor and the two streaming parsers (issue #2422). Audit of RSA-PSS parameter handling in the provider, which found Signature.setParameter reporting a rejected trailer field as an unchecked exception and leaving the rejected spec half-applied, so that the parameters reported for a signature were not the ones it had been made with (issue #2421). Audit of the PKCS#12 key store entry-setting boundary, which found setKeyEntry reporting a certificate whose public key the provider cannot resolve as an unchecked exception rather than the KeyStoreException it declares, and which led to the same guard being applied to setCertificateEntry and to the RFC 9579 PBMAC1 store, and to the rejected entry no longer being left behind in the store (issue #2419).
546546
- Flowdalic \<https://github.com/Flowdalic\> - initial implementation of an AnimalSniffer-based Android API-level compatibility check for the Gradle build (PR #336).
547547
- hannesa2 \<https://github.com/hannesa2\> - initial Dependabot configuration for the Gradle and GitHub Actions ecosystems (PR #883).
548548
- vladhuma \<https://github.com/vladhuma\> - initial implementation of server-side OCSP stapling for the BCJSSE provider, on behalf of Thales Group (PR #1740).

‎docs/releasenotes.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,8 @@ Date: 2026, TBD
174174

175175
- The three X509CRL accessors let an unchecked exception escape when an indirect CRL carried an entry whose certificateIssuer extension did not decode to a directoryName. getRevokedCertificates(), getRevokedCertificate(BigInteger) and isRevoked(Certificate) each re-parsed the extension with GeneralNames.getInstance(...).getNames()[0], which raises ArrayIndexOutOfBoundsException for an empty GeneralNames and IllegalArgumentException for a first GeneralName that is not a directoryName, neither of them declared by java.security.cert.X509CRL, so a caller guarding only the CRLException the API names did not catch them; a CRL is routinely fetched and parsed before its signature is checked, so the bytes reaching these methods are not necessarily trusted. The same three sites appear again in the deprecated org.bouncycastle.jce.provider.X509CRLObject, which the registered X509StreamParser.CRL service returns, and in the jdk1.1 and jdk1.3 overlays of both classes. This was not only a robustness problem: because the parse assumed the directoryName was first, a CRL naming its entry issuer as a directoryName after some other GeneralName - a well-formed encoding, GeneralNames being a SEQUENCE OF with no ordering requirement - was refused as well, while X509CRLEntryObject, which iterates for the directoryName inside a try/catch, read the same CRL correctly. All six call sites now use that sibling's logic, which has been promoted to a package-private static in each of the two packages rather than left as a fourth hand-written copy, so the accessors and the entry objects can no longer disagree about the same CRL. An entry whose certificateIssuer names no directoryName is treated as having no certificate issuer of its own, which java.security.cert.X509CRLEntry.getCertificateIssuer() defines as the CRL issuer's, so isRevoked reports such an entry against the CRL issuer rather than throwing (github #2425).
176176

177+
- Six OpenPGP signature subpackets whose body RFC 9580 defines as a fixed length did not enforce it when the subpacket was parsed, deferring the check to an accessor that reported the wrong length as an unchecked IllegalStateException. Signature Creation Time (sec. 5.2.3.11) and Key Expiration Time (sec. 5.2.3.13) carry a 4-octet time field, Issuer Key ID (sec. 5.2.3.12) an 8-octet key ID, and Exportable Certification (sec. 5.2.3.19), Revocable (sec. 5.2.3.20) and Primary User ID (sec. 5.2.3.27) a single octet that is a zero or a one; only Signature Expiration Time already checked its length. SignatureSubpacketInputStream validated the declared body length for range and for the case where it overran the subpacket area, so a body whose declared length matched the octets it actually carried was handed to the subpacket constructor whatever that length was, and org.bouncycastle.bcpg.sig.Utils then threw from timeFromBytes or booleanFromByteArray when the value was read. Because such a body inside a self-signature is content the signer signed over, a certificate carrying one verified as valid and only failed later, in ordinary reader code: PGPPublicKey.getValidSeconds(), PGPSignatureSubpacketVector.getKeyExpirationTime() and isExportable(), OpenPGPCertificate.getExpirationTime(), and the keyserver re-export path PGPPublicKeyRing.encode(out, true), which declares IOException rather than an unchecked exception. The six constructors now apply the check the way Signature Expiration Time, Features, Trust Signature and Notation Data already did, so a malformed subpacket is refused at parse time as a MalformedPacketException and the accessors cannot throw on a subpacket that exists, with the parser's existing tolerance of a fixed-length field whose declared length overruns the subpacket area preserved (github #2426).
178+
177179

178180
### 2.1.3 Additional Features and Functionality
179181

‎pg/src/main/java/org/bouncycastle/bcpg/sig/Exportable.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ public Exportable(
2020
boolean isLongLength,
2121
byte[] data)
2222
{
23-
super(SignatureSubpacketTags.EXPORTABLE, critical, isLongLength, data);
23+
super(SignatureSubpacketTags.EXPORTABLE, critical, isLongLength, Utils.checkBooleanData(data));
2424
}
2525

2626
public Exportable(

‎pg/src/main/java/org/bouncycastle/bcpg/sig/IssuerKeyID.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public IssuerKeyID(
3131
boolean isLongLength,
3232
byte[] data)
3333
{
34-
super(SignatureSubpacketTags.ISSUER_KEY_ID, critical, isLongLength, data);
34+
super(SignatureSubpacketTags.ISSUER_KEY_ID, critical, isLongLength, verifyData(data));
3535
}
3636

3737
public IssuerKeyID(
@@ -41,6 +41,17 @@ public IssuerKeyID(
4141
super(SignatureSubpacketTags.ISSUER_KEY_ID, critical, false, keyIDToBytes(keyID));
4242
}
4343

44+
// RFC 9580 5.2.3.12: the Issuer Key ID body is an 8-octet key ID, which is what
45+
// FingerprintUtil.readKeyID requires of the body getKeyID() hands it.
46+
private static byte[] verifyData(byte[] data)
47+
{
48+
if (data.length < 8)
49+
{
50+
throw new IllegalArgumentException("Truncated issuer key-ID subpacket");
51+
}
52+
return data;
53+
}
54+
4455
public long getKeyID()
4556
{
4657
return FingerprintUtil.readKeyID(data);

‎pg/src/main/java/org/bouncycastle/bcpg/sig/KeyExpirationTime.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ public KeyExpirationTime(
2929
boolean isLongLength,
3030
byte[] data)
3131
{
32-
super(SignatureSubpacketTags.KEY_EXPIRE_TIME, critical, isLongLength, data);
32+
super(SignatureSubpacketTags.KEY_EXPIRE_TIME, critical, isLongLength, Utils.checkTimeData(data));
3333
}
3434

3535
public KeyExpirationTime(

‎pg/src/main/java/org/bouncycastle/bcpg/sig/PrimaryUserID.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ public PrimaryUserID(
1919
boolean isLongLength,
2020
byte[] data)
2121
{
22-
super(SignatureSubpacketTags.PRIMARY_USER_ID, critical, isLongLength, data);
22+
super(SignatureSubpacketTags.PRIMARY_USER_ID, critical, isLongLength, Utils.checkBooleanData(data));
2323
}
2424

2525
public PrimaryUserID(

‎pg/src/main/java/org/bouncycastle/bcpg/sig/Revocable.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ public Revocable(
1919
boolean isLongLength,
2020
byte[] data)
2121
{
22-
super(SignatureSubpacketTags.REVOCABLE, critical, isLongLength, data);
22+
super(SignatureSubpacketTags.REVOCABLE, critical, isLongLength, Utils.checkBooleanData(data));
2323
}
2424

2525
public Revocable(

‎pg/src/main/java/org/bouncycastle/bcpg/sig/SignatureCreationTime.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public SignatureCreationTime(
3131
boolean isLongLength,
3232
byte[] data)
3333
{
34-
super(SignatureSubpacketTags.CREATION_TIME, critical, isLongLength, data);
34+
super(SignatureSubpacketTags.CREATION_TIME, critical, isLongLength, Utils.checkTimeData(data));
3535
}
3636

3737
public SignatureCreationTime(

‎pg/src/main/java/org/bouncycastle/bcpg/sig/SignatureExpirationTime.java‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ public SignatureExpirationTime(
2929
boolean isLongLength,
3030
byte[] data)
3131
{
32-
super(SignatureSubpacketTags.EXPIRE_TIME, critical, isLongLength, verifyData(data));
32+
super(SignatureSubpacketTags.EXPIRE_TIME, critical, isLongLength, Utils.checkTimeData(data));
3333
}
3434

3535
public SignatureExpirationTime(
@@ -38,15 +38,6 @@ public SignatureExpirationTime(
3838
{
3939
super(SignatureSubpacketTags.EXPIRE_TIME, critical, false, Utils.timeToBytes(seconds));
4040
}
41-
42-
private static byte[] verifyData(byte[] data)
43-
{
44-
if (data.length != 4)
45-
{
46-
throw new IllegalArgumentException("Malformed data length. Expected 4, got " + data.length);
47-
}
48-
return data;
49-
}
5041

5142
/**
5243
* return time in seconds before signature expires after creation time.

‎pg/src/main/java/org/bouncycastle/bcpg/sig/Utils.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,42 @@ else if (bytes[0] == 1)
5050
}
5151
}
5252

53+
/**
54+
* Check that the given subpacket body is the four octets RFC 9580 defines a time field as, so that
55+
* {@link #timeFromBytes(byte[])} cannot fail once the subpacket has been constructed.
56+
*
57+
* @param data subpacket body
58+
* @return data
59+
*/
60+
static byte[] checkTimeData(byte[] data)
61+
{
62+
if (data.length != 4)
63+
{
64+
throw new IllegalArgumentException("Malformed data length. Expected 4, got " + data.length);
65+
}
66+
return data;
67+
}
68+
69+
/**
70+
* Check that the given subpacket body is the single "zero or one" octet RFC 9580 defines a flag
71+
* field as, so that {@link #booleanFromByteArray(byte[])} cannot fail once the subpacket has been constructed.
72+
*
73+
* @param data subpacket body
74+
* @return data
75+
*/
76+
static byte[] checkBooleanData(byte[] data)
77+
{
78+
if (data.length != 1)
79+
{
80+
throw new IllegalArgumentException("Malformed data length. Expected 1, got " + data.length);
81+
}
82+
if (data[0] != 0 && data[0] != 1)
83+
{
84+
throw new IllegalArgumentException("Unexpected byte value for boolean encoding: " + data[0]);
85+
}
86+
return data;
87+
}
88+
5389
static long timeFromBytes(byte[] bytes)
5490
{
5591
if (bytes.length != 4)

0 commit comments

Comments
 (0)