Conversation
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
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.
ITSExplicitCertificateBuilder.buildpicks the certificate'sSignatureCHOICE by switching aVerificationKeyIndicatorCHOICE, the certificate's own when self-signing and otherwise the issuer's, against thePublicVerificationKeycurve constants (ITSExplicitCertificateBuilder.java:65-75,:91-104). The two CHOICE spaces are unrelated:verificationKey = 0andreconstructionValue = 1(VerificationKeyIndicator.java:25-26) indexecdsaNistP256andecdsaBrainpoolP256r1(PublicVerificationKey.java:24-26), so the signing key's curve never enters the decision:ecdsaNistP256Signature, and the verifier picked by that tag (ITSCertificate.java:57) refuses it withOperatorCreationException: wrong verifier for algorithm: 0(BcITSContentVerifierProvider.java:83-86);ECDSAEncoder.java:60to a 384-bit r, and 25 of 25 self-signed attempts threwIllegalArgumentException: standard length exceeded for value. A brainpoolP384r1 subject key under a P-256 issuer is unaffected;reconstructionValue = 1: a secp256r1 signature is silently mislabelledecdsaBrainpoolP256r1Signature, a brainpoolP256r1 one happens to be right, and a brainpoolP384r1 signer throws as above.Reproduced on the released bcpkix-jdk18on-1.86.jar, on current
origin/main(94270ff1cb) and on the byte-identical 1.87-SNAPSHOT beta (1.87.0.20719), with the same output and the re-decoded OER carrying the same tag.buildtakes no curve argument, so there is no workaround; every path is fail-closed.ETSISignedDataBuilderreads the curve from the signer already (ETSISignedDataBuilder.java:104,:131,:153).This change:
ECDSAEncoder.toITS(signer.getCurveID(), signer.getSignature()), dropping two locals and three imports. Both shipped signers reject a fourth curve at construction withIllegalArgumentException: unknown key type(BcITSContentSigner.java:64,JcaITSContentSigner.java:101); a third-partyITSContentSignerreporting one now getsIllegalArgumentException: unknown curveID(ECDSAEncoder.java:76) rather than the removed arm'sIllegalStateException.JcaITSExplicitCertificateBuilderinherits the correctedbuild; andtestBuildSelfSignedCurvesandtestBuildUnderImplicitIssuertoITSBasicTest: the first builds a self-signed CA and an end-entity certificate under it on all three curves, checking the emitted CHOICE and that the verifier accepts each; the second pins the implicit-issuer case. Both fail without the change (ITSBasicTest.java:366,:452) and pass with it; the package's 21 pre-existing tests pass either way.Base tree only, no overlays and no
module-infochange (org.bouncycastle.itsis already exported). A release-note entry is included.