Skip to content

Commit f17d2fe

Browse files
committed
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
1 parent 48807a2 commit f17d2fe

3 files changed

Lines changed: 22 additions & 20 deletions

File tree

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

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,12 @@ public class CredentialSigner {
5050
static final BigInteger P256_HALF_N = P256_N.shiftRight(1);
5151
static final int P256_COMPONENT_LEN = 32;
5252

53+
// JCA algorithm names. P1363 (raw r||s) is the W3C VC-DI-ECDSA wire format;
54+
// the DER path is a fallback for JREs without the P1363 algorithm alias.
55+
// Package-private so SelectiveDisclosure can reuse them.
56+
static final String ALGO_ECDSA_P1363 = "SHA256withECDSAinP1363Format";
57+
static final String ALGO_ECDSA_DER = "SHA256withECDSA";
58+
5359
private final ObjectMapper objectMapper;
5460
private final JsonLdProcessor jsonLdProcessor;
5561

@@ -306,19 +312,18 @@ private byte[] signEcdsaP256Raw(byte[] data, ECKey privateKey) throws GeneralSec
306312
ECPrivateKey ecPrivateKey = privateKey.toECPrivateKey();
307313
byte[] p1363;
308314
try {
309-
Signature sig = Signature.getInstance("SHA256withECDSAinP1363Format");
315+
Signature sig = Signature.getInstance(ALGO_ECDSA_P1363);
310316
sig.initSign(ecPrivateKey);
311317
sig.update(data);
312318
p1363 = sig.sign();
313319
} catch (java.security.NoSuchAlgorithmException e) {
314-
// Fallback: use DER format and convert to P1363
315-
Signature sig = Signature.getInstance("SHA256withECDSA");
320+
Signature sig = Signature.getInstance(ALGO_ECDSA_DER);
316321
sig.initSign(ecPrivateKey);
317322
sig.update(data);
318323
byte[] derSig = sig.sign();
319324
p1363 = derToP1363(derSig, P256_COMPONENT_LEN);
320325
}
321-
return normalizeToLowS(p1363, P256_N);
326+
return normalizeToLowS(p1363);
322327
} catch (com.nimbusds.jose.JOSEException e) {
323328
throw new GeneralSecurityException("Failed to extract EC private key from JWK", e);
324329
}
@@ -343,14 +348,13 @@ private boolean verifyEcdsaP256Raw(byte[] data, byte[] signature, ECKey publicKe
343348
try {
344349
ECPublicKey ecPublicKey = publicKey.toECPublicKey();
345350
try {
346-
Signature sig = Signature.getInstance("SHA256withECDSAinP1363Format");
351+
Signature sig = Signature.getInstance(ALGO_ECDSA_P1363);
347352
sig.initVerify(ecPublicKey);
348353
sig.update(data);
349354
return sig.verify(signature);
350355
} catch (java.security.NoSuchAlgorithmException e) {
351-
// Fallback: convert P1363 to DER and verify
352356
byte[] derSig = p1363ToDer(signature);
353-
Signature sig = Signature.getInstance("SHA256withECDSA");
357+
Signature sig = Signature.getInstance(ALGO_ECDSA_DER);
354358
sig.initVerify(ecPublicKey);
355359
sig.update(data);
356360
return sig.verify(derSig);
@@ -361,22 +365,20 @@ private boolean verifyEcdsaP256Raw(byte[] data, byte[] signature, ECKey publicKe
361365
}
362366

363367
/**
364-
* Normalize an IEEE P1363 ECDSA signature to low-S canonical form.
368+
* Normalize an IEEE P1363 ECDSA P-256 signature to low-S canonical form.
365369
* If s > n/2, replaces s with n - s (which is mathematically equivalent
366370
* as an ECDSA signature). Otherwise returns the input unchanged.
367371
*
368372
* <p>Package-private so {@link SelectiveDisclosure} can reuse it.
369373
*/
370-
static byte[] normalizeToLowS(byte[] p1363Sig, BigInteger n) {
374+
static byte[] normalizeToLowS(byte[] p1363Sig) {
371375
int half = p1363Sig.length / 2;
372376
BigInteger s = new BigInteger(1, Arrays.copyOfRange(p1363Sig, half, p1363Sig.length));
373-
if (s.compareTo(n.shiftRight(1)) <= 0) {
377+
if (s.compareTo(P256_HALF_N) <= 0) {
374378
return p1363Sig;
375379
}
376-
BigInteger sPrime = n.subtract(s);
377-
byte[] sBytes = sPrime.toByteArray();
380+
byte[] sBytes = P256_N.subtract(s).toByteArray();
378381
byte[] result = p1363Sig.clone();
379-
// Zero out the s region, then right-align sPrime into it.
380382
Arrays.fill(result, half, result.length, (byte) 0);
381383
copyToFixed(sBytes, result, half, half);
382384
return result;

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -237,19 +237,18 @@ private byte[] signEcdsaP256(byte[] data, ECKey privateKey) throws GeneralSecuri
237237
ECPrivateKey ecPrivateKey = privateKey.toECPrivateKey();
238238
byte[] p1363;
239239
try {
240-
Signature sig = Signature.getInstance("SHA256withECDSAinP1363Format");
240+
Signature sig = Signature.getInstance(CredentialSigner.ALGO_ECDSA_P1363);
241241
sig.initSign(ecPrivateKey);
242242
sig.update(data);
243243
p1363 = sig.sign();
244244
} catch (java.security.NoSuchAlgorithmException e) {
245-
Signature sig = Signature.getInstance("SHA256withECDSA");
245+
Signature sig = Signature.getInstance(CredentialSigner.ALGO_ECDSA_DER);
246246
sig.initSign(ecPrivateKey);
247247
sig.update(data);
248248
byte[] derSig = sig.sign();
249249
p1363 = CredentialSigner.derToP1363(derSig, CredentialSigner.P256_COMPONENT_LEN);
250250
}
251-
// Normalize to low-S canonical form to prevent signature malleability.
252-
return CredentialSigner.normalizeToLowS(p1363, CredentialSigner.P256_N);
251+
return CredentialSigner.normalizeToLowS(p1363);
253252
} catch (com.nimbusds.jose.JOSEException e) {
254253
throw new GeneralSecurityException("Failed to extract EC private key", e);
255254
}

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -191,11 +191,12 @@ void shouldFailEcdsaVerificationWithTamperedDocument() {
191191
@SuppressWarnings("unchecked")
192192
void ecdsaSignaturesShouldAlwaysBeInLowSForm() {
193193
// Regression: JDK SunEC produces high-S signatures roughly half the time.
194-
// After our normalization, every signature must have s <= n/2. Run many
195-
// iterations so the probability of missing a high-S path is negligible.
194+
// After normalization every signature must have s <= n/2. 20 iterations
195+
// gives P(all happen to be low-S anyway) ≈ 2^-20 ≈ 1e-6 — low enough
196+
// that a regression won't be masked by luck.
196197
ECKey keyPair = KeyPairManager.generateP256KeyPair();
197198

198-
for (int i = 0; i < 50; i++) {
199+
for (int i = 0; i < 20; i++) {
199200
Map<String, Object> credential = buildSampleCredential();
200201
credential.put("id", "https://example.com/cred/" + i);
201202

0 commit comments

Comments
 (0)