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

* Enforce ECDSA low-S canonicalization to block signature malleability

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

* Simplify low-S ECDSA normalization

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

* Close the three review-gap holes in ECDSA malleability tests

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

* Simplify gap-coverage tests

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

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: brody-0125

src/main/java/work/brodykim/signet/credential/CredentialSigner.java

@@ -41,6 +41,21 @@
  */
 public class CredentialSigner {
 
+    // P-256 (secp256r1) curve order n, per SEC 2 / FIPS 186-5.
+    // Used to enforce low-S canonicalization on ECDSA signatures and reject
+    // malleable high-S variants (defense-in-depth against signature malleability
+    // where (r, s) and (r, n - s) are both mathematically valid).
+    static final BigInteger P256_N = new BigInteger(
+            "FFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551", 16);
+    static final BigInteger P256_HALF_N = P256_N.shiftRight(1);
+    static final int P256_COMPONENT_LEN = 32;
+
+    // JCA algorithm names. P1363 (raw r||s) is the W3C VC-DI-ECDSA wire format;
+    // the DER path is a fallback for JREs without the P1363 algorithm alias.
+    // Package-private so SelectiveDisclosure can reuse them.
+    static final String ALGO_ECDSA_P1363 = "SHA256withECDSAinP1363Format";
+    static final String ALGO_ECDSA_DER = "SHA256withECDSA";
+
     private final ObjectMapper objectMapper;
     private final JsonLdProcessor jsonLdProcessor;
 
@@ -287,41 +302,59 @@ private PublicKey decodeEd25519PublicKey(byte[] rawKey) throws GeneralSecurityEx
     /**
      * Sign data with ECDSA P-256 in IEEE P1363 format (r || s, 64 bytes).
      * The W3C Data Integrity ECDSA spec requires P1363 format, not DER.
+     *
+     * <p>The returned signature is always in low-S canonical form
+     * (s &lt;= n/2) to avoid signature malleability. JDK's SunEC provider
+     * does not enforce low-S, so we normalize the output here.
      */
     private byte[] signEcdsaP256Raw(byte[] data, ECKey privateKey) throws GeneralSecurityException {
         try {
             ECPrivateKey ecPrivateKey = privateKey.toECPrivateKey();
+            byte[] p1363;
             try {
-                Signature sig = Signature.getInstance("SHA256withECDSAinP1363Format");
+                Signature sig = Signature.getInstance(ALGO_ECDSA_P1363);
                 sig.initSign(ecPrivateKey);
                 sig.update(data);
-                return sig.sign();
+                p1363 = sig.sign();
             } catch (java.security.NoSuchAlgorithmException e) {
-                // Fallback: use DER format and convert to P1363
-                Signature sig = Signature.getInstance("SHA256withECDSA");
+                Signature sig = Signature.getInstance(ALGO_ECDSA_DER);
                 sig.initSign(ecPrivateKey);
                 sig.update(data);
                 byte[] derSig = sig.sign();
-                return derToP1363(derSig, 32);
+                p1363 = derToP1363(derSig, P256_COMPONENT_LEN);
             }
+            return normalizeToLowS(p1363);
         } catch (com.nimbusds.jose.JOSEException e) {
             throw new GeneralSecurityException("Failed to extract EC private key from JWK", e);
         }
     }
 
     private boolean verifyEcdsaP256Raw(byte[] data, byte[] signature, ECKey publicKey)
             throws GeneralSecurityException {
+        // Enforce low-S canonical form and reject out-of-range (r, s) before
+        // delegating to the underlying verifier. This prevents signature
+        // malleability: without this check, both (r, s) and (r, n - s) would
+        // verify as valid for the same message, breaking any system that
+        // treats the signature bytes as a stable identifier (revocation,
+        // tracking, deduplication, etc.).
+        if (signature == null || signature.length != P256_COMPONENT_LEN * 2) {
+            return false;
+        }
+        BigInteger r = new BigInteger(1, Arrays.copyOfRange(signature, 0, P256_COMPONENT_LEN));
+        BigInteger s = new BigInteger(1, Arrays.copyOfRange(signature, P256_COMPONENT_LEN, signature.length));
+        if (r.signum() <= 0 || r.compareTo(P256_N) >= 0) return false;
+        if (s.signum() <= 0 || s.compareTo(P256_HALF_N) > 0) return false;
+
         try {
             ECPublicKey ecPublicKey = publicKey.toECPublicKey();
             try {
-                Signature sig = Signature.getInstance("SHA256withECDSAinP1363Format");
+                Signature sig = Signature.getInstance(ALGO_ECDSA_P1363);
                 sig.initVerify(ecPublicKey);
                 sig.update(data);
                 return sig.verify(signature);
             } catch (java.security.NoSuchAlgorithmException e) {
-                // Fallback: convert P1363 to DER and verify
                 byte[] derSig = p1363ToDer(signature);
-                Signature sig = Signature.getInstance("SHA256withECDSA");
+                Signature sig = Signature.getInstance(ALGO_ECDSA_DER);
                 sig.initVerify(ecPublicKey);
                 sig.update(data);
                 return sig.verify(derSig);
@@ -331,6 +364,26 @@ private boolean verifyEcdsaP256Raw(byte[] data, byte[] signature, ECKey publicKe
         }
     }
 
+    /**
+     * Normalize an IEEE P1363 ECDSA P-256 signature to low-S canonical form.
+     * If s &gt; n/2, replaces s with n - s (which is mathematically equivalent
+     * as an ECDSA signature). Otherwise returns the input unchanged.
+     *
+     * <p>Package-private so {@link SelectiveDisclosure} can reuse it.
+     */
+    static byte[] normalizeToLowS(byte[] p1363Sig) {
+        int half = p1363Sig.length / 2;
+        BigInteger s = new BigInteger(1, Arrays.copyOfRange(p1363Sig, half, p1363Sig.length));
+        if (s.compareTo(P256_HALF_N) <= 0) {
+            return p1363Sig;
+        }
+        byte[] sBytes = P256_N.subtract(s).toByteArray();
+        byte[] result = p1363Sig.clone();
+        Arrays.fill(result, half, result.length, (byte) 0);
+        copyToFixed(sBytes, result, half, half);
+        return result;
+    }
+
     /**
      * Convert a DER-encoded ECDSA signature to IEEE P1363 format (r || s).
      */

src/main/java/work/brodykim/signet/credential/SelectiveDisclosure.java

@@ -235,18 +235,20 @@ private List<Integer> resolveMandatoryIndexes(List<String> quads,
     private byte[] signEcdsaP256(byte[] data, ECKey privateKey) throws GeneralSecurityException {
         try {
             ECPrivateKey ecPrivateKey = privateKey.toECPrivateKey();
+            byte[] p1363;
             try {
-                Signature sig = Signature.getInstance("SHA256withECDSAinP1363Format");
+                Signature sig = Signature.getInstance(CredentialSigner.ALGO_ECDSA_P1363);
                 sig.initSign(ecPrivateKey);
                 sig.update(data);
-                return sig.sign();
+                p1363 = sig.sign();
             } catch (java.security.NoSuchAlgorithmException e) {
-                Signature sig = Signature.getInstance("SHA256withECDSA");
+                Signature sig = Signature.getInstance(CredentialSigner.ALGO_ECDSA_DER);
                 sig.initSign(ecPrivateKey);
                 sig.update(data);
                 byte[] derSig = sig.sign();
-                return CredentialSigner.derToP1363(derSig, 32);
+                p1363 = CredentialSigner.derToP1363(derSig, CredentialSigner.P256_COMPONENT_LEN);
             }
+            return CredentialSigner.normalizeToLowS(p1363);
         } catch (com.nimbusds.jose.JOSEException e) {
             throw new GeneralSecurityException("Failed to extract EC private key", e);
         }

src/test/java/work/brodykim/signet/CredentialSignerTest.java

@@ -3,12 +3,15 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import work.brodykim.signet.credential.CredentialSigner;
 import work.brodykim.signet.credential.KeyPairManager;
+import work.brodykim.signet.credential.Multibase;
 import work.brodykim.signet.jsonld.CachedDocumentLoader;
 import work.brodykim.signet.jsonld.JsonLdProcessor;
 import com.nimbusds.jose.jwk.ECKey;
 import com.nimbusds.jose.jwk.OctetKeyPair;
 import org.junit.jupiter.api.Test;
 
+import java.math.BigInteger;
+import java.util.Arrays;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -177,6 +180,147 @@ void shouldFailEcdsaVerificationWithTamperedDocument() {
         assertFalse(valid, "Should fail ECDSA verification when document is tampered");
     }
 
+    // ── ECDSA signature malleability (low-S enforcement) regression tests ───
+
+    /** P-256 curve order n. */
+    private static final BigInteger P256_N = new BigInteger(
+            "FFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551", 16);
+    private static final BigInteger P256_HALF_N = P256_N.shiftRight(1);
+
+    @Test
+    @SuppressWarnings("unchecked")
+    void ecdsaSignaturesShouldAlwaysBeInLowSForm() {
+        // Regression: JDK SunEC produces high-S signatures roughly half the time.
+        // After normalization every signature must have s <= n/2. 20 iterations
+        // gives P(all happen to be low-S anyway) ≈ 2^-20 ≈ 1e-6 — low enough
+        // that a regression won't be masked by luck.
+        ECKey keyPair = KeyPairManager.generateP256KeyPair();
+
+        for (int i = 0; i < 20; i++) {
+            Map<String, Object> credential = buildSampleCredential();
+            credential.put("id", "https://example.com/cred/" + i);
+
+            Map<String, Object> signed = signer.signWithEcdsaDataIntegrity(
+                    credential, keyPair, "https://example.com/issuers/1#key-1");
+
+            Map<String, Object> proof = (Map<String, Object>) signed.get("proof");
+            String proofValue = (String) proof.get("proofValue");
+            byte[] sig = Multibase.decodeBase58Btc(proofValue);
+            assertEquals(64, sig.length, "P-256 P1363 signature must be 64 bytes");
+
+            BigInteger s = new BigInteger(1, Arrays.copyOfRange(sig, 32, 64));
+            assertTrue(s.compareTo(P256_HALF_N) <= 0,
+                    "s must be <= n/2 (low-S canonical form) on iteration " + i
+                            + ", got s=" + s.toString(16));
+            assertTrue(s.signum() > 0, "s must be positive");
+        }
+    }
+
+    @Test
+    @SuppressWarnings("unchecked")
+    void ecdsaVerificationShouldRejectMalleableHighSVariant() {
+        // Malleability attack: given a valid signature (r, s), compute (r, n-s).
+        // Without low-S enforcement, this would also verify — allowing the same
+        // credential to be represented by two distinct proofValues, breaking
+        // revocation/tracking systems that key off the signature bytes.
+        ECKey keyPair = KeyPairManager.generateP256KeyPair();
+        Map<String, Object> credential = buildSampleCredential();
+
+        Map<String, Object> signed = signer.signWithEcdsaDataIntegrity(
+                credential, keyPair, "https://example.com/issuers/1#key-1");
+
+        // Sanity: the original signature verifies.
+        assertTrue(signer.verifyEcdsaDataIntegrity(signed, keyPair.toPublicJWK()));
+
+        byte[] origSig = Multibase.decodeBase58Btc(
+                (String) ((Map<String, Object>) signed.get("proof")).get("proofValue"));
+        byte[] forged = flipS(origSig, P256_N);
+
+        // Preconditions: r is unchanged, s has actually been flipped to high-S.
+        assertArrayEquals(Arrays.copyOfRange(origSig, 0, 32),
+                Arrays.copyOfRange(forged, 0, 32),
+                "forgery should leave r untouched");
+        BigInteger forgedS = new BigInteger(1, Arrays.copyOfRange(forged, 32, 64));
+        assertTrue(forgedS.compareTo(P256_HALF_N) > 0,
+                "forged signature must be in high-S region to exercise malleability");
+
+        assertFalse(verifyWithForgedSignature(signed, keyPair, forged),
+                "Verifier must reject the malleable high-S twin of a valid signature");
+    }
+
+    @Test
+    @SuppressWarnings("unchecked")
+    void ecdsaVerificationShouldRejectMalformedSignatureLength() {
+        ECKey keyPair = KeyPairManager.generateP256KeyPair();
+        Map<String, Object> credential = buildSampleCredential();
+
+        Map<String, Object> signed = signer.signWithEcdsaDataIntegrity(
+                credential, keyPair, "https://example.com/issuers/1#key-1");
+
+        byte[] origSig = Multibase.decodeBase58Btc(
+                (String) ((Map<String, Object>) signed.get("proof")).get("proofValue"));
+        byte[] truncated = Arrays.copyOfRange(origSig, 0, 32);
+
+        assertFalse(verifyWithForgedSignature(signed, keyPair, truncated),
+                "Verifier must reject signatures that are not 64 bytes");
+    }
+
+    @Test
+    @SuppressWarnings("unchecked")
+    void ecdsaVerificationShouldRejectOutOfRangeR() {
+        // Locks in the r ∈ [1, n-1] range check. The verifier pre-rejects r == 0
+        // and r >= n before delegating to the underlying ECDSA verifier — without
+        // this, a subsequent implementation change that skipped the range check
+        // would go unnoticed.
+        ECKey keyPair = KeyPairManager.generateP256KeyPair();
+        Map<String, Object> credential = buildSampleCredential();
+
+        Map<String, Object> signed = signer.signWithEcdsaDataIntegrity(
+                credential, keyPair, "https://example.com/issuers/1#key-1");
+        byte[] origSig = Multibase.decodeBase58Btc(
+                (String) ((Map<String, Object>) signed.get("proof")).get("proofValue"));
+
+        // Forge r = 0 (zero out the first 32 bytes).
+        byte[] zeroR = origSig.clone();
+        Arrays.fill(zeroR, 0, 32, (byte) 0);
+        assertFalse(verifyWithForgedSignature(signed, keyPair, zeroR),
+                "Verifier must reject r == 0");
+
+        // Forge r = n (encode the curve order directly in the r slot).
+        byte[] rEqualsN = origSig.clone();
+        byte[] nBytes = P256_N.toByteArray();
+        // P256_N.toByteArray() may return 33 bytes (leading 0x00 sign byte) since n > 2^255.
+        System.arraycopy(nBytes, nBytes.length - 32, rEqualsN, 0, 32);
+        assertFalse(verifyWithForgedSignature(signed, keyPair, rEqualsN),
+                "Verifier must reject r == n (r must be strictly less than n)");
+    }
+
+    @SuppressWarnings("unchecked")
+    private boolean verifyWithForgedSignature(Map<String, Object> signed, ECKey keyPair, byte[] forgedSig) {
+        Map<String, Object> proof = new LinkedHashMap<>((Map<String, Object>) signed.get("proof"));
+        proof.put("proofValue", Multibase.encodeBase58Btc(forgedSig));
+        Map<String, Object> tampered = new LinkedHashMap<>(signed);
+        tampered.put("proof", proof);
+        return signer.verifyEcdsaDataIntegrity(tampered, keyPair.toPublicJWK());
+    }
+
+    /** Flip s to n - s in an IEEE P1363 (r || s) signature, keeping r intact. */
+    private static byte[] flipS(byte[] p1363, BigInteger n) {
+        int half = p1363.length / 2;
+        BigInteger s = new BigInteger(1, Arrays.copyOfRange(p1363, half, p1363.length));
+        BigInteger sPrime = n.subtract(s);
+        byte[] sBytes = sPrime.toByteArray();
+        byte[] out = p1363.clone();
+        Arrays.fill(out, half, out.length, (byte) 0);
+        // Right-align sBytes into the s region, stripping any BigInteger sign byte.
+        if (sBytes.length > half) {
+            System.arraycopy(sBytes, sBytes.length - half, out, half, half);
+        } else {
+            System.arraycopy(sBytes, 0, out, half + (half - sBytes.length), sBytes.length);
+        }
+        return out;
+    }
+
     // ── Helper ──────────────────────────────────────────────────────────────
 
     private Map<String, Object> buildSampleCredential() {