Skip to content

Commit b2a4e20

Browse files
claudeclaude
committed
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
1 parent f17d2fe commit b2a4e20

2 files changed

Lines changed: 211 additions & 0 deletions

File tree

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,45 @@ void ecdsaVerificationShouldRejectMalformedSignatureLength() {
275275
"Verifier must reject signatures that are not 64 bytes");
276276
}
277277

278+
@Test
279+
@SuppressWarnings("unchecked")
280+
void ecdsaVerificationShouldRejectOutOfRangeR() {
281+
// Locks in the r ∈ [1, n-1] range check. The verifier pre-rejects r == 0
282+
// and r >= n before delegating to the underlying ECDSA verifier — without
283+
// this, a subsequent implementation change that skipped the range check
284+
// would go unnoticed.
285+
ECKey keyPair = KeyPairManager.generateP256KeyPair();
286+
Map<String, Object> credential = buildSampleCredential();
287+
288+
Map<String, Object> signed = signer.signWithEcdsaDataIntegrity(
289+
credential, keyPair, "https://example.com/issuers/1#key-1");
290+
byte[] origSig = Multibase.decodeBase58Btc(
291+
(String) ((Map<String, Object>) signed.get("proof")).get("proofValue"));
292+
293+
// Forge r = 0 (zero out the first 32 bytes).
294+
byte[] zeroR = origSig.clone();
295+
Arrays.fill(zeroR, 0, 32, (byte) 0);
296+
assertFalse(verifyWithForgedSignature(signed, keyPair, zeroR),
297+
"Verifier must reject r == 0");
298+
299+
// Forge r = n (encode the curve order directly in the r slot).
300+
byte[] rEqualsN = origSig.clone();
301+
byte[] nBytes = P256_N.toByteArray();
302+
// P256_N.toByteArray() may return 33 bytes (leading 0x00 sign byte) since n > 2^255.
303+
System.arraycopy(nBytes, nBytes.length - 32, rEqualsN, 0, 32);
304+
assertFalse(verifyWithForgedSignature(signed, keyPair, rEqualsN),
305+
"Verifier must reject r == n (r must be strictly less than n)");
306+
}
307+
308+
@SuppressWarnings("unchecked")
309+
private boolean verifyWithForgedSignature(Map<String, Object> signed, ECKey keyPair, byte[] forgedSig) {
310+
Map<String, Object> proof = new LinkedHashMap<>((Map<String, Object>) signed.get("proof"));
311+
proof.put("proofValue", Multibase.encodeBase58Btc(forgedSig));
312+
Map<String, Object> tampered = new LinkedHashMap<>(signed);
313+
tampered.put("proof", proof);
314+
return signer.verifyEcdsaDataIntegrity(tampered, keyPair.toPublicJWK());
315+
}
316+
278317
/** Flip s to n - s in an IEEE P1363 (r || s) signature, keeping r intact. */
279318
private static byte[] flipS(byte[] p1363, BigInteger n) {
280319
int half = p1363.length / 2;

‎src/test/java/work/brodykim/signet/credential/EcdsaInternalsTest.java‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
package work.brodykim.signet.credential;
2+
3+
import work.brodykim.signet.jsonld.CachedDocumentLoader;
4+
import work.brodykim.signet.jsonld.JsonLdProcessor;
5+
import com.nimbusds.jose.jwk.ECKey;
6+
import org.junit.jupiter.api.Test;
7+
8+
import java.math.BigInteger;
9+
import java.security.Signature;
10+
import java.security.interfaces.ECPrivateKey;
11+
import java.util.Arrays;
12+
import java.util.LinkedHashMap;
13+
import java.util.List;
14+
import java.util.Map;
15+
16+
import static org.junit.jupiter.api.Assertions.*;
17+
18+
/**
19+
* Tests that need package-level access to low-level ECDSA helpers and the
20+
* CBOR-encoded ecdsa-sd-2023 proof value. Package is intentionally
21+
* {@code work.brodykim.signet.credential} to reach the package-private
22+
* {@code derToP1363}, {@code p1363ToDer}, and {@code P256_*} constants in
23+
* {@link CredentialSigner} without widening production visibility.
24+
*/
25+
class EcdsaInternalsTest {
26+
27+
private final JsonLdProcessor jsonLdProcessor = new JsonLdProcessor(new CachedDocumentLoader());
28+
private final SelectiveDisclosure selectiveDisclosure = new SelectiveDisclosure(jsonLdProcessor);
29+
30+
// ── DER ⇄ P1363 round-trip coverage (exercises the DER fallback path) ──
31+
32+
@Test
33+
void derToP1363RoundTripsForRealJdkDerSignature() throws Exception {
34+
// The JDK DER path in signEcdsaP256Raw/verifyEcdsaP256Raw is only reached
35+
// when SHA256withECDSAinP1363Format is absent. Modern JDKs always ship
36+
// it, so the fallback is effectively never exercised at runtime. Test
37+
// the conversion directly against a real DER signature produced by the
38+
// JDK so a regression in derToP1363/p1363ToDer would still be caught.
39+
ECKey jwk = KeyPairManager.generateP256KeyPair();
40+
ECPrivateKey privateKey = jwk.toECPrivateKey();
41+
42+
byte[] data = "payload under test".getBytes();
43+
Signature sig = Signature.getInstance(CredentialSigner.ALGO_ECDSA_DER);
44+
sig.initSign(privateKey);
45+
sig.update(data);
46+
byte[] derSig = sig.sign();
47+
48+
byte[] p1363 = CredentialSigner.derToP1363(derSig, CredentialSigner.P256_COMPONENT_LEN);
49+
assertEquals(64, p1363.length, "P-256 P1363 must be 64 bytes");
50+
51+
byte[] derAgain = CredentialSigner.p1363ToDer(p1363);
52+
53+
// Verifier must accept either form for the same (key, data).
54+
Signature verifyDer = Signature.getInstance(CredentialSigner.ALGO_ECDSA_DER);
55+
verifyDer.initVerify(jwk.toECPublicKey());
56+
verifyDer.update(data);
57+
assertTrue(verifyDer.verify(derAgain),
58+
"DER → P1363 → DER round-trip must produce a DER signature that verifies");
59+
}
60+
61+
@Test
62+
void p1363ToDerHandlesHighBitSetOnBothComponents() {
63+
// DER integer encoding requires a leading 0x00 when the MSB is set
64+
// (otherwise the value would be interpreted as negative). Exercise that
65+
// branch by crafting P1363 bytes with 0x80 in both r and s.
66+
byte[] p1363 = new byte[64];
67+
p1363[0] = (byte) 0x80; // r has MSB set
68+
p1363[1] = 0x01;
69+
p1363[32] = (byte) 0x80; // s has MSB set
70+
p1363[33] = 0x02;
71+
// Fill the rest with non-zero so the values are full-length.
72+
for (int i = 2; i < 32; i++) p1363[i] = (byte) 0x11;
73+
for (int i = 34; i < 64; i++) p1363[i] = (byte) 0x22;
74+
75+
byte[] der = CredentialSigner.p1363ToDer(p1363);
76+
// 0x30 | total-len | 0x02 | 33 | 0x00 | 32 r-bytes | 0x02 | 33 | 0x00 | 32 s-bytes
77+
assertEquals(0x30, der[0] & 0xFF, "DER SEQUENCE marker");
78+
assertEquals(0x02, der[2] & 0xFF, "DER INTEGER marker for r");
79+
assertEquals(33, der[3] & 0xFF, "r must be padded to 33 bytes (leading 0x00)");
80+
assertEquals(0x00, der[4] & 0xFF, "r padding byte");
81+
assertEquals(0x02, der[37] & 0xFF, "DER INTEGER marker for s");
82+
assertEquals(33, der[38] & 0xFF, "s must be padded to 33 bytes");
83+
assertEquals(0x00, der[39] & 0xFF, "s padding byte");
84+
85+
byte[] backToP1363 = CredentialSigner.derToP1363(der, CredentialSigner.P256_COMPONENT_LEN);
86+
assertArrayEquals(p1363, backToP1363,
87+
"DER → P1363 → DER → P1363 must be a no-op for fully-populated inputs");
88+
}
89+
90+
// ── normalizeToLowS direct unit coverage ──────────────────────────────
91+
92+
@Test
93+
void normalizeToLowSFlipsExactlyWhenSExceedsHalfN() {
94+
byte[] lowS = new byte[64];
95+
Arrays.fill(lowS, 0, 32, (byte) 0x11); // arbitrary r
96+
// s = 1 (low)
97+
lowS[63] = 0x01;
98+
assertSame(lowS, CredentialSigner.normalizeToLowS(lowS),
99+
"s already low → must return input unchanged (same reference)");
100+
101+
byte[] highS = new byte[64];
102+
Arrays.fill(highS, 0, 32, (byte) 0x11);
103+
// s = n - 1 (definitely > n/2)
104+
byte[] nMinus1 = CredentialSigner.P256_N.subtract(BigInteger.ONE).toByteArray();
105+
System.arraycopy(nMinus1, nMinus1.length - 32, highS, 32, 32);
106+
107+
byte[] normalized = CredentialSigner.normalizeToLowS(highS);
108+
assertNotSame(highS, normalized, "high-S path must allocate a new array");
109+
BigInteger normalizedS = new BigInteger(1, Arrays.copyOfRange(normalized, 32, 64));
110+
assertEquals(BigInteger.ONE, normalizedS,
111+
"n - (n - 1) must equal 1");
112+
assertArrayEquals(Arrays.copyOfRange(highS, 0, 32), Arrays.copyOfRange(normalized, 0, 32),
113+
"r must be preserved during low-S normalization");
114+
}
115+
116+
// ── ecdsa-sd-2023 base signature low-S invariant ──────────────────────
117+
118+
@Test
119+
void selectiveDisclosureBaseSignatureIsAlwaysLowS() {
120+
// Mirror of the ecdsa-rdfc-2022 low-S invariant, but for ecdsa-sd-2023.
121+
// Without direct access to the signEcdsaP256 helper we read the base
122+
// signature straight out of the CBOR prefix emitted by CborEncoder:
123+
//
124+
// [0..2] = 0xd9 0x5d 0x02 (CBOR tag)
125+
// [3] = 0x85 (array(5) header)
126+
// [4..5] = 0x58 0x40 (byte string, length 64)
127+
// [6..69] = 64-byte base signature
128+
//
129+
// This layout is stable for P-256 base proofs.
130+
ECKey keyPair = KeyPairManager.generateP256KeyPair();
131+
132+
for (int i = 0; i < 20; i++) {
133+
Map<String, Object> credential = buildSampleCredential();
134+
credential.put("id", "https://example.com/cred/" + i);
135+
136+
Map<String, Object> signed = selectiveDisclosure.createBaseProof(
137+
credential, keyPair,
138+
"https://example.com/issuers/1#key-1",
139+
List.of("/issuer"));
140+
141+
@SuppressWarnings("unchecked")
142+
Map<String, Object> proof = (Map<String, Object>) signed.get("proof");
143+
byte[] cbor = Multibase.decodeBase58Btc((String) proof.get("proofValue"));
144+
145+
assertEquals((byte) 0xd9, cbor[0]);
146+
assertEquals((byte) 0x5d, cbor[1]);
147+
assertEquals((byte) 0x02, cbor[2]);
148+
assertEquals((byte) 0x85, cbor[3]);
149+
assertEquals((byte) 0x58, cbor[4]);
150+
assertEquals((byte) 0x40, cbor[5]);
151+
152+
byte[] baseSig = Arrays.copyOfRange(cbor, 6, 70);
153+
BigInteger s = new BigInteger(1, Arrays.copyOfRange(baseSig, 32, 64));
154+
assertTrue(s.signum() > 0, "s must be positive on iteration " + i);
155+
assertTrue(s.compareTo(CredentialSigner.P256_HALF_N) <= 0,
156+
"ecdsa-sd-2023 base signature must be low-S on iteration " + i
157+
+ " (got s=" + s.toString(16) + ")");
158+
}
159+
}
160+
161+
private Map<String, Object> buildSampleCredential() {
162+
Map<String, Object> credential = new LinkedHashMap<>();
163+
credential.put("@context", List.of("https://www.w3.org/ns/credentials/v2",
164+
"https://purl.imsglobal.org/spec/ob/v3p0/context-3.0.3.json"));
165+
credential.put("type", List.of("VerifiableCredential", "OpenBadgeCredential"));
166+
credential.put("id", "https://example.com/cred/1");
167+
credential.put("issuer", Map.of("id", "https://example.com/issuers/1", "type", "Profile", "name", "Test"));
168+
credential.put("validFrom", "2026-01-01T00:00:00Z");
169+
credential.put("name", "Test Badge");
170+
return credential;
171+
}
172+
}

0 commit comments

Comments
 (0)