Skip to content

Enforce low-S canonicalization on ECDSA signatures to prevent malleability#3

Merged
brody-0125 merged 5 commits intomainfrom
claude/fix-ecdsa-malleability-Qgdqb
Apr 16, 2026
Merged

Enforce low-S canonicalization on ECDSA signatures to prevent malleability#3
brody-0125 merged 5 commits intomainfrom
claude/fix-ecdsa-malleability-Qgdqb

Conversation

@brody-0125
Copy link
Copy Markdown
Owner

@brody-0125 brody-0125 commented Apr 16, 2026

Summary

This PR adds defense-in-depth against ECDSA signature malleability by enforcing low-S canonical form on all P-256 signatures. The JDK's SunEC provider does not enforce low-S normalization, allowing mathematically equivalent signatures (r, s) and (r, n-s) to both verify. This breaks systems that rely on signature bytes as stable identifiers (revocation, tracking, deduplication).

Key Changes

  • Signature normalization on signing: Added normalizeToLowS() method that converts any high-S signature to its low-S equivalent (s ≤ n/2) before returning. Applied in both CredentialSigner and SelectiveDisclosure.

  • Strict validation on verification: Enhanced verifyEcdsaP256Raw() to reject signatures that:

    • Are not exactly 64 bytes (P-256 P1363 format)
    • Have r or s outside the valid range [1, n)
    • Have s in the high-S region (s > n/2)

  • Constants extraction: Moved P-256 curve parameters (P256_N, P256_HALF_N, P256_COMPONENT_LEN) and algorithm names (ALGO_ECDSA_P1363, ALGO_ECDSA_DER) to static fields in CredentialSigner for reuse across the codebase.

  • Comprehensive regression tests: Added three new test cases:

    • ecdsaSignaturesShouldAlwaysBeInLowSForm(): Verifies 20 consecutive signatures are all normalized to low-S (P(false positive) ≈ 2^-20)
    • ecdsaVerificationShouldRejectMalleableHighSVariant(): Confirms the verifier rejects the high-S twin of a valid signature
    • ecdsaVerificationShouldRejectMalformedSignatureLength(): Ensures truncated signatures are rejected

Implementation Details

  • The low-S normalization is mathematically equivalent: if s > n/2, replacing s with n-s produces a signature that verifies identically but in canonical form.
  • Verification now performs early rejection before delegating to the underlying JCA provider, preventing any possibility of accepting malleable variants.
  • Both signing paths (P1363 native and DER fallback) are normalized consistently.

https://claude.ai/code/session_011UnXmEtWXkFn8sz5bnNkUT

claude added 3 commits April 15, 2026 09:28
@claude
Enforce ECDSA low-S canonicalization to block signature malleability
48807a2 
JDK's SunEC ECDSA provider emits both low-S and high-S signatures, and
its verifier accepts either. That makes (r, s) and (r, n - s) both valid
for the same message, so any system that treats the signature bytes as
a stable identifier (revocation lists, credential tracking, dedup) can
be bypassed by swapping in the malleable twin.

This change:
- Normalizes every P-256 signature emitted by signEcdsaP256Raw and
  SelectiveDisclosure.signEcdsaP256 to low-S (s <= n/2).
- Rejects high-S and out-of-range (r, s) values up front in
  verifyEcdsaP256Raw before delegating to the JDK verifier.
- Adds regression tests covering the malleability attack (forged
  high-S twin must be rejected), the low-S invariant on fresh
  signatures, and malformed signature lengths.

https://claude.ai/code/session_011UnXmEtWXkFn8sz5bnNkUT
@claude
Simplify low-S ECDSA normalization
f17d2fe 
Apply review feedback on top of the malleability fix:
- Drop the speculative BigInteger parameter from normalizeToLowS; it
  was always called with P256_N and now uses the existing P256_HALF_N
  constant directly instead of recomputing n >> 1 on each call.
- Extract the two JCA algorithm name strings into shared constants on
  CredentialSigner, removed four duplicated string literals across
  CredentialSigner and SelectiveDisclosure.
- Remove a stale WHAT-style comment and cut the low-S regression loop
  from 50 to 20 iterations (2^-20 false-negative probability is already
  well past the flaky-test threshold, and the JSON-LD canonicalization
  per iteration was dominating test time).

https://claude.ai/code/session_011UnXmEtWXkFn8sz5bnNkUT
@claude
Close the three review-gap holes in ECDSA malleability tests
b2a4e20 
Self-review flagged three blind spots in the original malleability
regression suite:

- No test exercised the r ∈ [1, n-1] bounds check on verify — only
  the s-side high-S rejection was covered. Added
  ecdsaVerificationShouldRejectOutOfRangeR to forge r == 0 and r == n
  via byte-level tampering and confirm both are rejected.

- The DER fallback path in signEcdsaP256Raw/verifyEcdsaP256Raw is
  effectively dead on modern JDKs, leaving derToP1363/p1363ToDer
  untested. Added a direct round-trip test against a real JDK DER
  signature plus a crafted high-bit-set input that exercises the
  DER sign-extension branch of p1363ToDer.

- SelectiveDisclosure.signEcdsaP256 shares the low-S normalization
  contract with CredentialSigner but had no test locking it in.
  Added selectiveDisclosureBaseSignatureIsAlwaysLowS, which reads
  the 64-byte base signature out of the fixed CBOR prefix emitted
  by CborEncoder and asserts s <= n/2 across 20 iterations.

The new tests live in src/test/java/work/brodykim/signet/credential/
so they can reach the package-private helpers (derToP1363,
p1363ToDer, normalizeToLowS, P256_* constants) without widening
production visibility.

https://claude.ai/code/session_011UnXmEtWXkFn8sz5bnNkUT
brody-0125 pushed a commit that referenced this pull request Apr 16, 2026
@claude
fix(sd-2023): align proof header + multibase with W3C VC-DI-ECDSA spec
bc9da19 
Addresses five review findings against the W3C VC Data Integrity ECDSA
Cryptosuites 1.0 spec and Open Badges 3.0 (which defers to VC-DI):

A. Base proof CBOR header corrected from 0xd9 0x5d 0x02 to 0xd9 0x5d 0x00
   (spec §3.5.2). The digitalbazaar reference implementation confirms
   CBOR_PREFIX_BASE = [0xd9, 0x5d, 0x00].

B. proofValue multibase prefix corrected from 'z' (base58btc) to 'u'
   (base64url-no-pad) for both base and derived proofs (spec §3.5.2,
   §3.5.3, §3.5.7, §3.5.8 — "If the proofValue string does not start
   with `u`, an error MUST be raised"). base58btc is retained for the
   other cryptosuites (eddsa-rdfc-2022, ecdsa-rdfc-2022) that legitimately
   require it. Adds Multibase.encode/decodeBase64UrlNoPad().

#1. verifyDerivedProof now fails fast on a non-ecdsa-sd-2023 cryptosuite
    before attempting CBOR decode (spec §2.2.1).

#3. Javadoc on verifyDerivedProof documents the public-key trust contract:
    this primitive only verifies signatures under the proof-embedded
    publicKey and does NOT resolve verificationMethod / check controller
    binding — callers MUST layer VC-DI §2.6 issuer/controller validation
    before trusting the boolean result.

#5. verifyDerivedProof rejects mandatoryIndexes that are null, negative,
    or ≥ quadList.size() explicitly, giving a clean failure path instead
    of relying on incidental downstream hash mismatch.

Regression tests added: base-proof header/prefix compliance, derived-proof
header/prefix compliance, cryptosuite-mismatch rejection, and
base-proof-fed-to-derived-verifier rejection. Existing tests updated for
the new 'u' prefix. 89/89 tests pass.

Known remaining interop gap (NOT addressed here): the labelMap is still
serialized as CBOR text→text rather than the spec's compressed
int→bytes form (§3.5.5), so cross-verification with
@digitalbazaar/ecdsa-sd-2023-cryptosuite still fails on the labelMap
element. This will be a separate follow-up PR.
claude added 2 commits April 16, 2026 08:07
@claude
Merge remote-tracking branch 'origin/main' into claude/fix-ecdsa-mall…
8cad5cf 
…eability-Qgdqb
@claude
Simplify gap-coverage tests
5a89f6f 
Apply review feedback on the ECDSA test additions:
- Reuse the new verifyWithForgedSignature helper from the two earlier
  forgery tests (malleable high-S variant, truncated signature),
  dropping the proof-wrapping boilerplate that was duplicated three
  times.
- Replace two hand-rolled fill loops in p1363ToDerHandlesHighBitSetOn
  BothComponents with Arrays.fill.
- Narrow the derToP1363RoundTripsForRealJdkDerSignature throws clause
  from Exception to the three specific checked exceptions the body
  can throw (NoSuchAlgorithmException, InvalidKeyException,
  SignatureException) plus JOSEException from key extraction.
- Extract CBOR_BASE_SIG_OFFSET and CBOR_BASE_SIG_END constants so the
  intent of the bare `cbor[6..70]` slice is self-documenting.

https://claude.ai/code/session_011UnXmEtWXkFn8sz5bnNkUT
@brody-0125 brody-0125 merged commit 6b2c52e into main Apr 16, 2026
2 checks passed
brody-0125 added a commit that referenced this pull request Apr 16, 2026
@brody-0125 @claude
Implement W3C ecdsa-sd-2023 disclosure proof derivation and verificat…
6cc24dc 
…ion (#5)

* fix(sd-2023): strip HMAC key from presented proofs via derivation step

Adds the missing W3C ecdsa-sd-2023 disclosure-proof derivation and
verification path. Previously only createBaseProof was implemented, so
the issuer's HMAC key was embedded in every proofValue presented to
verifiers — letting any verifier brute-force the small canonical-label
space (_:c14n0, _:c14n1, ...) to reverse blank-node masking and
dictionary-attack per-quad signatures to recover undisclosed claims.

- SelectiveDisclosure.deriveProof: holder-side step that decodes the
  base proof, re-canonicalizes with the issuer's HMAC, and emits a
  derived proof (CBOR tag 0xd95d01) carrying baseSignature, publicKey,
  per-quad signatures, a c14n→HMAC labelMap, and mandatoryIndexes —
  but NOT the HMAC key itself.
- SelectiveDisclosure.verifyDerivedProof: verifier-side reconstruction
  of the signed canonical form via labelMap (no HMAC key needed).
- CborDecoder (new) + CborEncoder.encodeDerivedProofValue: wire format
  support for both base (0x5d02) and derived (0x5d01) proof variants.
- SelectiveDisclosureTest: round-trip, tag/shape, tamper-detection, and
  the central security assertion that the HMAC key bytes do not appear
  in any derived proofValue.

Scope note: this implementation reveals the full credential (no claim
subsetting yet). Subset-by-JSON-Pointer is a follow-up; the security
fix — HMAC key never leaves the holder — is delivered now.

* fix(sd-2023): align proof header + multibase with W3C VC-DI-ECDSA spec

Addresses five review findings against the W3C VC Data Integrity ECDSA
Cryptosuites 1.0 spec and Open Badges 3.0 (which defers to VC-DI):

A. Base proof CBOR header corrected from 0xd9 0x5d 0x02 to 0xd9 0x5d 0x00
   (spec §3.5.2). The digitalbazaar reference implementation confirms
   CBOR_PREFIX_BASE = [0xd9, 0x5d, 0x00].

B. proofValue multibase prefix corrected from 'z' (base58btc) to 'u'
   (base64url-no-pad) for both base and derived proofs (spec §3.5.2,
   §3.5.3, §3.5.7, §3.5.8 — "If the proofValue string does not start
   with `u`, an error MUST be raised"). base58btc is retained for the
   other cryptosuites (eddsa-rdfc-2022, ecdsa-rdfc-2022) that legitimately
   require it. Adds Multibase.encode/decodeBase64UrlNoPad().

#1. verifyDerivedProof now fails fast on a non-ecdsa-sd-2023 cryptosuite
    before attempting CBOR decode (spec §2.2.1).

#3. Javadoc on verifyDerivedProof documents the public-key trust contract:
    this primitive only verifies signatures under the proof-embedded
    publicKey and does NOT resolve verificationMethod / check controller
    binding — callers MUST layer VC-DI §2.6 issuer/controller validation
    before trusting the boolean result.

#5. verifyDerivedProof rejects mandatoryIndexes that are null, negative,
    or ≥ quadList.size() explicitly, giving a clean failure path instead
    of relying on incidental downstream hash mismatch.

Regression tests added: base-proof header/prefix compliance, derived-proof
header/prefix compliance, cryptosuite-mismatch rejection, and
base-proof-fed-to-derived-verifier rejection. Existing tests updated for
the new 'u' prefix. 89/89 tests pass.

Known remaining interop gap (NOT addressed here): the labelMap is still
serialized as CBOR text→text rather than the spec's compressed
int→bytes form (§3.5.5), so cross-verification with
@digitalbazaar/ecdsa-sd-2023-cryptosuite still fails on the labelMap
element. This will be a separate follow-up PR.

* refactor(sd-2023): simplify deriveProof + verifyDerivedProof

Review-driven cleanup targeting only code introduced in this PR:

- deriveProof: drop redundant replaceBlankNodesWithHmac call. The HMAC'd
  quads were only ever passed to resolveMandatoryIndexes, which matches
  against predicate IRIs — unaffected by blank-node relabelling. Skipping
  the rewrite halves the HMAC/regex work in this code path.
- deriveProof: drop the "new ArrayList<>(mandatoryIdx)" copy; the local
  list is not reused anywhere.
- verifyDerivedProof: drop null check on Integer indexes (CBOR decoder
  never emits null elements) and replace java.util.Set/HashSet FQNs with
  proper imports.
- buildLabelMap: replace per-byte String.format("%02x", ...) hex loop
  with java.util.HexFormat.formatHex (JDK 17, already on build classpath).
- decompressP256PublicKey: cache the P-256 ECParameterSpec in a static
  final field so per-verification AlgorithmParameters construction is a
  one-shot class-init cost.
- Extract CRYPTOSUITE_ECDSA_SD_2023 constant for the two new usages
  (guard + verifier-side proofConfig rebuild). Existing createBaseProof
  string literals left untouched to keep the diff narrow.

Existing public API, wire format, and 89/89 tests pass unchanged.

---------

Co-authored-by: Claude <noreply@anthropic.com>
brody-0125 pushed a commit that referenced this pull request Apr 16, 2026
@claude
Merge origin/main: resolve conflicts with low-S and ecdsa-sd-2023 work
279f44d 
Conflicts in CredentialSigner.signEcdsaP256Raw and
SelectiveDisclosure.signEcdsaP256 came from PR #3 (low-S canonicalization)
and PR #5 (ecdsa-sd-2023 derivation) landing on main alongside this
branch's key-zeroization rewrite of the same methods.

Resolution:
- CredentialSigner.signEcdsaP256Raw: keep ecPrivateKey hoisted out of
  the inner try (needed by our finally{tryDestroy}) and add the byte[]
  p1363 staging variable from main so normalizeToLowS can run on the
  result before return.
- SelectiveDisclosure.signEcdsaP256: keep our pre-extracted ECPrivateKey
  parameter shape (avoids re-running toECPrivateKey per quad) and adopt
  main's algorithm-name constants (CredentialSigner.ALGO_ECDSA_*),
  P256_COMPONENT_LEN, and final normalizeToLowS step. The outer JOSE
  try/catch is dropped because the JOSE call is now in the caller.

Drive-by: EcdsaInternalsTest.selectiveDisclosureBaseSignatureIsAlwaysLowS
was broken on plain main — PR #5 changed createBaseProof to W3C-required
multibase-base64url-no-pad ('u' prefix) and updated the CBOR base-proof
tag low byte from 0x02 to 0x00, but PR #3's test still called
decodeBase58Btc and asserted 0x02. Switch the test to
decodeBase64UrlNoPad and the 0x00 tag so this branch's CI is green.

https://claude.ai/code/session_01Tjyxx8f3ZhfXdNEFMP1avf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@brody-0125 @claude