Skip to content

Fix DoS on negative scalar input to mulPointEscalar + EdDSA bounds hardening#38

Open
simonmasson wants to merge 2 commits into
iden3:mainfrom
simonmasson:main
Open

Fix DoS on negative scalar input to mulPointEscalar + EdDSA bounds hardening#38
simonmasson wants to merge 2 commits into
iden3:mainfrom
simonmasson:main

Conversation

@simonmasson
Copy link
Copy Markdown

Problem

BabyJub.mulPointEscalar(P, e) assumed e >= 0. Passing a negative BigInt made the while (!Scalar.isZero(rem)) loop spin forever -- right-shifting a negative scalar never reaches zero.

This was reachable through EdDSA verification: the verify* functions checked S >= subOrder but did not check S >= 0, so a malformed signature with a raw negative S made verifyPoseidon / verifyMiMC / verifyMiMCSponge / verifyPedersen hang inside scalar mult instead of returning false. Verifier DoS, not a forgery.

Fix

Two layers:

1. Make the primitive total over (src/babyjub.js).
If e < 0, normalize to [-e]·(-P) before the double-and-add loop. On twisted Edwards, -(x, y) = (-x, y), so the rewrite is one field negation and is universally correct (works for any point, in or out of subgroup -- important because inSubgroup itself calls mulPointEscalar on potentially-cofactor points, where reducing e mod L would silently give the wrong answer).

2. Reject malformed signatures at the API boundary (src/eddsa.js).
Symmetric lower-bound check if (Scalar.lt(sig.S, Scalar.e(0))) return false; added to all four verify* functions, paired with the existing S >= subOrder upper-bound check. Negative S is rejected before any scalar mult runs.

Layer 1 closes the foot-gun for every caller of mulPointEscalar (current and future). Layer 2 is defense-in-depth at the EdDSA boundary -- faster rejection and clearer intent.

Test

Added "Reject a signature with a negative S" in test/eddsa.js, covering both:

  • Modular negation: S' = subOrder - S -- stays in [0, subOrder) and passes the bounds check, so it goes through scalar mult; verification correctly returns false because the equation doesn't hold.
  • Raw negative: S' = -S -- must be rejected by the new bounds check before mulPointEscalar is called. The test enforces this by monkey-patching eddsa.babyJub.mulPointEscalar to throw; if the bounds check ever regresses and verification falls through, the test fails loudly.

All existing tests still pass.

Notes

  • The existing upper-bound check uses a mix of Scalar.geq (Pedersen) and native >= (others). The new lower-bound check uses Scalar.lt consistently. Happy to also normalize the upper-bound style if preferred.
  • A small amount of whitespace/formatting cleanup is included in the same diff. Can be split into a separate commit on request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant