Skip to content

fix(ecc): reduce ECDSA x-coordinate - #935

Open
thevilledev wants to merge 2 commits into
Legrandin:masterfrom
thevilledev:fix/ecdsa-large-x
Open

thevilledev wants to merge 2 commits into
Legrandin:masterfrom
thevilledev:fix/ecdsa-large-x

Conversation

@thevilledev

Copy link
Copy Markdown

Summary

ECDSA verification compared the recomputed affine x-coordinate directly with r. Valid ECDSA verification requires comparing x mod n with r, where n is the curve order.

As a result, valid signatures with a recomputed x-coordinate greater than or equal to n were rejected. This is a correctness and interoperability issue. It causes false negatives but does not allow invalid signatures or forgeries.

SEC 1: Elliptic Curve Cryptography, Version 2.0 Section 4.1.4, steps 7–8, specifies that ECDSA verification must compute v = xR mod n and compare v with r.

Changes

  • Reduce the recomputed x-coordinate modulo the curve order before comparing it with r.
  • Add a P-256/SHA-256 regression test based on a valid Project Wycheproof vector. The regression is covered both directly and by the extended Wycheproof suite.
  • Remove the existing Wycheproof exception that silently skipped signatures labeled k*G has a large x-coordinate.

The regression vector is tcId 285 in the Wycheproof corpus bundled with PyCryptodome. The same vector is tcId 350 in the current C2SP Wycheproof corpus.

Add a P-256 vector whose verification point has an x-coordinate
larger than the subgroup order. The valid signature captures the
interoperability failure reported by Wycheproof.

Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
Reduce the reconstructed verification point's x-coordinate modulo the
curve order before comparing it with r, as required by ECDSA.

This accepts valid signatures with x greater than or equal to n and lets
the previously skipped Wycheproof cases run normally.

Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>

This branch has not been deployed

No deployments
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