guard minimum length in sm2 decrypt and gost/dstu/desede/rc2 unwrap - #2359
Open
rootvector2 wants to merge 4084 commits into
Open
guard minimum length in sm2 decrypt and gost/dstu/desede/rc2 unwrap#2359rootvector2 wants to merge 4084 commits into
rootvector2 wants to merge 4084 commits into
Conversation
The earlier ArmoredOutputStream CR/LF fix hardened only the Builder; the deprecated setHeader/addHeader, the ArmoredOutputStream(out, Hashtable) constructor and the core PemWriter still emitted a header name/value verbatim, so a CR or LF could inject an extra header line or terminate the header block early with the remainder parsed as base64 body (header injection). Guard the single sink every path funnels through (ArmoredOutputStream.writeHeaderEntry, PemWriter.writeObject), throwing IllegalArgumentException; the Builder's pre-split values are unaffected.
…ost label ProvX509TrustManager.checkEndpointID dispatched the HTTPS algorithm to HostnameUtil.checkHostname with all-labels wildcard matching, so a dNSName SAN such as "foo.*.com" matched "foo.evil.com" and "*.*.com" matched any two-label .com host -- more permissive than SunJSSE (HostnameChecker.matchLeftmostWildcard for TYPE_TLS) and contrary to RFC 6125 sec. 6.4.3 / RFC 9525 sec. 6.3, which require the wildcard to be the complete left-most label. The HTTPS path now uses left-most-label-only matching, like the already-correct LDAP/LDAPS path.
- implement encodeTo directly
BCJSSE: restrict HTTPS endpoint-identification wildcards to the left-most label (hostname-verification hardening) See merge request root/bc-java!230
Cross-validated against the BCFIPS SecureHash.SM3_HMAC implementation; full-length 32-byte output guards against MAC truncation regressions.
…ms, refusing others with CMSAlgorithmNotAllowedException
…multiply-add
NTRUPlusEngine.finalizeWithAddition (the h*r + m step of encapsulation) applied the
Montgomery factor R as the unreduced literal 2^16 instead of its representative R mod q
(-147, the reference's NTRUPLUS_R). The two are congruent modulo q, so output stayed
correct mod q and the KAT vectors were unaffected, but 2^16 is ~445x larger, so for a
message coefficient near +/-(q-1)/2 the term c*2^16 pushed montgomery_reduce's input past
its valid range (+/- q*2^15). montgomery_reduce then returned a value outside (-Q, Q),
which poly_tobytes (single +Q fold) packed incorrectly, corrupting that ciphertext
coefficient and making ~0.2% of encapsulations decapsulate to the wrong shared secret
("unable to extract KTS secret: checksum failed" on key unwrap). Apply R mod q, matching
the NTRU+ reference. NTRU+ KAT output is byte-for-byte unchanged. Adds testRandomRoundTrips.
…tectedPart, AttributeTypeAndValue, POPOSigningKeyInput, IssuerAndSerialNumber, OriginatorPublicKey) with a lower-bound size check
…2-byte ML-DSA seed with IOException, not an IllegalArgumentException from Arrays.copyOfRange
…nt with a clean IOException in SExpression.parseExpression
…ay against the bytes remaining before allocating, matching the readOpaque/readAll guard
…rter tags with CMSTagLengthException
…ognising AES, ARIA and SM4 GCM/CCM
…fix ARIA-CCM cipher alias
… MAC, relates to github bcgit#287.
…AnchorID<1..2^8-1> length bound in TrustAnchorIDs: logId/landmarkId/landmarkGroupId now validate the caId input and reject a built ID outside 1..255 bytes, and fromDottedDecimal rejects an over-255-byte result, rather than relying on the downstream MTCSignature / MTCCosignedMessage opaque<1..255> checks to catch an out-of-range trust anchor ID only at serialization time.
…ureSpi: engineSign generated a 32-byte r that was never used (processPreHashedMessage was always called with null, matching draft-ietf-lamps-pq-composite-sigs sec. 2.2 which has no randomizer in M' = Prefix || Label || len(ctx) || ctx || PH(M)); dropped the unused r parameter, the SecureRandom field and its now-unused imports, and stale commented-out r-handling. Behaviour is unchanged - the message representative was already built without r.
… version=0 rule in the CMS KEMRecipientInfo / CMSORIforKEMOtherInfo ASN.1 types, which previously only checked the 65535 upper bound and accepted a non-zero KEMRecipientInfo version on parse; adds KEMRecipientInfoTest cases for a zero kekLength (constructor and parse paths) and a non-zero version.
…en-style -sources.jar/-javadoc.jar files for the jdk1.4 and jdk15to18 Ant distributions from the src.zip and javadoc artifacts the Ant build leaves behind (bcprov-ext reuses bcprov, bctest is sources-only), with a JDKDIR-parameterised script.
…lease.debug is true: jdk14.xml now loads bc-build[.user].properties so it can see release.debug and sets target.prefix to debug-jdk14 (else jdk14) via a condition, so target.name and every <module>-${target.name} artifact picks up the debug label; default (release.debug=false) output is unchanged.
…when release.debug is true, mirroring the jdk14.xml change: jdk15+.xml now loads bc-build[.user].properties so it can see release.debug and sets target.prefix to debug-jdk15to18 (else jdk15to18) via a condition; default (release.debug=false) output is unchanged.
…ars left at the top level (e.g. bctest-*.jar) cannot be accidentally staged and committed.
…t-build step reaches debug jars: the fixed 'bcprov-jdk15to18*.jar' / 'bcprov-ext-jdk15to18*.jar' globs did not match a release.debug build's 'bcprov-debug-jdk15to18-*.jar' names, so debug provider jars shipped without the JCA auto-registration file. Replaced the tar/mv/rm dance with a loop over 'bcprov*jdk15to18*.jar' (matches prov and ext, debug and non-debug) using jar uf -C prov/src/main/resources META-INF.
… @Override/@FunctionalInterface source strip that jdk15+.xml lacked (ant/jdk14.xml preprocesses it away), exclude the tests using JDK 7/8-only APIs (PKIXRevocationChecker, Cipher.updateAAD, java.nio.file.Files, JSSE SNI/ExtendedSSLSession, PipedInputStream(,int), cert.plants generics) with matching jdk1.5 tsp AllTests / jce RegressionTest overlays, and route three guarded Java 8 test calls (CertTest verify(pub,Provider), operator GCMParameterSpec, BouncyCastleProviderTest String.join) through 1.5-safe code
… the public X509Certificate type, not cert.getClass(): the BC cert impl (X509CertificateImpl/X509CertificateObject) is package-private, so a Method obtained from the runtime class failed invoke() with IllegalAccessException and broke cert.test.AllTests testSimpleTests after the jdk15to18 reflective-call change.
… a re-initialised cipher does not carry the previous operation's associated data into the authentication tag, incorporating github PR bcgit#2349 (converting its tab indentation to spaces) and adding a DSTU7624Test regression case that a re-init matches a freshly constructed cipher; relates to github bcgit#2349.
… (or -debug-jdk15to18- on a Java <= 8 daemon) and compile with debug symbols when release.debug is true, mirroring the jdk14.xml / jdk15+.xml debug-label change: build.gradle reads release.debug (via -Prelease.debug or gradle.properties, default false) and prefixes the shared vmrange, so every module's jar archiveBaseName/appendix and Maven artifactId pick up the debug label, and options.debug follows the flag; default (release.debug=false) output is unchanged.
…Cipher (DSTU 7624 CCM), incorporating github PR bcgit#2350: init zero-extends a short nonce to the block size and processAssociatedText zero-pads a trailing partial AAD block instead of throwing "padding not supported". Corrects the PR's AAD MAC loop, whose bound byteIndex < authLen - assocOff double-subtracted the offset (authLen already equals aadLen - assocOff) and skipped every AAD block after the first, silently changing the MAC for multi-block associated data; the bound is byteIndex < authLen. Adds DSTU7624Test cases locking the multi-block AAD MAC to its pre-PR value and round-tripping partial AAD and short nonces; relates to github bcgit#2350.
…Debug builds subsection under Building explaining that -Prelease.debug=true (or release.debug=true in gradle.properties) labels every module's jars/artifacts <module>-debug-jdk18on-<version> and compiles with debug symbols, with clean build / copyJars examples.
run_mtt.sh extracted the release version with `fgrep version`, an unanchored substring match. The debug-build comment added to gradle.properties in 10056f5 contains "version" (in <version>), so fgrep matched it too and $version became a two-line string, garbling every -scan jar path and failing the CI module-dependency step with a spurious "File not found: .../bcjmail-jdk18on-1.85\n# A debug build...". Match only the line starting with version= instead.
…overlay resync (partial: the provider does not yet build clean) Reviving the long-rotted jdk1.3 legacy build. A genuine 32-bit Sun JDK 1.3.1 no longer runs on a modern host, so the build-jdk13-docker skill runs it inside a debian/eol:etch (i386, glibc 2.3.6) container: glibc < 2.4 avoids the PTR_MANGLE pointer-mangling that segfaults the green-threads Classic VM, forcing -classic (green threads) avoids the extinct libstdc++-libc6.1-1.so.2 the native launcher needs, a uname->i686 shim works around the pre-x86-64 launch wrapper, and ORO is supplied to ant via -lib for <replaceregexp> (no java.util.regex before 1.4). Build/overlay fixes made so far: - ant/jdk13.xml: add the @deprecated strip pass (previously only in jdk14.xml, so @deprecated survived into 1.3 javac as "illegal character"); exclude the algorithm families added since the tree was last synced, matching jdk14.xml (hash2curve, crypto/bls + BLS*, Ascon, asymmetric/compositekem, CompositeKEMs, jcajce/PKCS12LoadStoreParameter, drbg Entropy{Daemon,Gatherer}). - initCause() is a Java 1.4 API absent on 1.3: message-only jdk1.3 overlays for util/Exceptions, jcajce/provider/util/SecurityExceptions and asn1/DEREncodingException (1.3 has no getCause, so the dropped cause is unobservable; message text kept verbatim and signatures kept in lockstep with base). - asn1/StreamUtil overlay: add the checkLength(long,long) overload base callers use. - jce/src: backport javax.security.auth.Destroyable and DestroyFailedException (Java 1.4, absent from the JDK 1.3.1 rt.jar; javax.* is loadable from the class path where java.* is not) - fixes ~32 key classes at once. - Relocate the picnic overlays from pqc/crypto/picnic to pqc/legacy/picnic to match the base package move (removes a duplicate-class error). Known remaining before bcprov-jdk13 builds (real source floor is Java 1.4): logging overlays for the java.util.logging users (SM2Signer, PKCS12KeyStoreSpi, PKCS12PBMAC1KeyStoreSpi, BouncyCastleProvider); refresh the stale X509CertificateObject overlays for the new hasFriendlyName(); the symmetric AES/Camellia/SEED/Serpent/LEA unresolved symbol; and jdk1.4-style overlays (or excludes) for Falcon, SDitH and SQIsign.
…-SNAPSHOT (release.suffix/version 1.85.99), and the BC and BCPQC provider info strings to v1.86-SNAPSHOT with provider version 1.8599 across the base, jdk1.1 and jdk1.4 copies
…i-release overlays and normalise OpenSSH KeyFactory error handling The edec KeyFactorySpi jdk1.11/jdk1.15 overlays had drifted from base on the OpenSSH private-key path: they used the no-passphrase parsePrivateKeyBlob overload (so passphrase-encrypted openssh-key-v1 Ed25519 keys failed on JDK 11+), left a raw RuntimeException escape a malformed blob, and jdk1.11 threw IllegalStateException rather than InvalidKeySpecException. Both overlays now match base. Across rsa/dsa/ec/edec the OpenSSH wrong-type and decode-failure paths now consistently throw InvalidKeySpecException and wrap the public-key parse, and the RSA private path's "public key is not RSA private key" message was corrected. Adds OpenSSHKeyFactoryMRTest under src/test/jdk1.11 and jdk1.15 (wired into AllTests11/AllTests15) so test11/test15 run the OpenSSH key specs against the multi-release jar and catch overlay drift.
…erage for overlaid behaviour, in docs/claude/architecture.md
IANAObjectIdentifiers was consolidated to a single home in core (shipped in bcprov) in 1.85 (github bcgit#2176), but ant/bc+-build.xml's build-util target still copied org/bouncycastle/asn1/iana/** into bcutil, so the Ant distributions (bcutil-jdk15to18, bcutil-jdk14) duplicated the class already present in bcprov and an Android/R8 build depending on both failed with "Duplicate class org.bouncycastle.asn1.iana.IANAObjectIdentifiers". The iana arc is the only package in the build-util include list also shipped publicly by core, and no util class references it, so dropping the include removes the duplicate while leaving the class in bcprov. The Gradle jdk18on jars were already correct. Relates to github bcgit#2356.
ligefeiBouncycastle
self-requested a review
July 23, 2026 04:25
Collaborator
|
Thank you for the PR, nice catch. A few days ago I had identified the same issue in SM2 and GOST28147, and your PR also covers DSTU7624, DESede and RC2, so this is a good opportunity to combine the fixes. I'll merge them together. |
Contributor
Author
|
sounds good, makes sense to combine them. thanks for picking it up. |
Contributor
Author
|
sounds good. |
hubot
pushed a commit
that referenced
this pull request
Jul 26, 2026
…ht engines Follow-up to the previous commit (PR #2359, Naveed Khan). SM2Engine.decrypt rejected a short ciphertext with "ciphertext too short"; every other engine and AEAD mode in the tree uses "data too short" for this condition (CCMBlockCipher, GCMBlockCipher, EAXBlockCipher, OCBBlockCipher, KCCMBlockCipher, KGCMBlockCipher, ChaCha20Poly1305, AEADBaseEngine, the Ascon engines), so use that wording and update the ShortWrapCipherTextTest assertion in lockstep. Also hoists c1Len into a local so the length check and the following new byte[c1Len] cannot drift apart, and expands the two guard comments to record which unchecked exception each guard prevents. No functional change beyond the message.
hubot
pushed a commit
that referenced
this pull request
Jul 26, 2026
… / DESede / RC2 unwrap minimum-length guards (PR #2359)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SM2Engine.decryptand theGOST28147/DSTU7624/DESede/RC2key-wrapunwrapengines sized their output asnew byte[inLen - overhead]without first checkinginLencovers that overhead, so a short attacker-supplied ciphertext threwNegativeArraySizeException/ArrayIndexOutOfBoundsExceptioninstead of the declaredInvalidCipherTextException; found auditing these against the already-guardedIESEngine/RFC3394WrapEngine/RFC5649WrapEngineand adds the same minimum-length check.