Skip to content

Conversation

@tarcieri
Copy link
Member

We've had a couple request to make signature parsing infallible. Presently it checks that the s scalar component of the signature has its three highest bits clear, which will reject some but not all signatures where s overflows the curve's order.

However, this check will not reject all such invalid signatures (at least as presently implemented), and the same check will be performed at the time the signature is verified (which will reject all such invalid signatures).

Though we already shipped ed25519 v2.0.0 and this is a breaking change, that release has been yanked so we can get this change in last minute.

We've had a couple request to make signature parsing infallible.
Presently it checks that the `s` scalar component of the signature has
its three highest bits clear, which will reject some but not all
signatures where `s` overflows the curve's order.

However, this check will not reject *all* such invalid signatures (at
least as presently implemented), and the same check will be performed at
the time the signature is verified (which *will* reject all such invalid
signatures).

Though we already shipped `ed25519` v2.0.0 and this is a breaking
change, that release has been yanked so we can get this change in last
minute.
@rozbb
Copy link

rozbb commented Jan 21, 2023

LGTM. Maybe a doc comment somewhere (either on the struct or on all the methods) that the contents is a dumb byte representation and there is no guarantee it represents a well-formed signature (ie scalar might exceed modulus). And, if exists, a pointer to a function that will let you know if there's an issue with the sig

@tarcieri
Copy link
Member Author

Added a comment about the signature being a dumb byte representation in a5512a0.

And, if exists, a pointer to a function that will let you know if there's an issue with the sig

In the comment I noted signature verifiers are expected to perform these checks.

This commit removes the only logic this crate previously had for performing such checks, which as noted was incomplete at best.

@tarcieri tarcieri merged commit 64b75e5 into master Jan 21, 2023
@tarcieri tarcieri deleted the ed25519/infallible-signature-parsing branch January 21, 2023 16:44
@rozbb
Copy link

rozbb commented Jan 21, 2023

and does not necessarily represent well-formed elements of the respective elliptic curve fields.

Better: and does not necessarily represent well-formed field or curve elements

It's still not entirely true because any byte sequence is a valid compressed Edwards Y representation. A more pendantic thing would be:

s does not necessarily represent a well-formed field element

@tarcieri tarcieri mentioned this pull request Jan 21, 2023
@tarcieri
Copy link
Member Author

@rozbb updated the wording in #624

@rozbb
Copy link

rozbb commented Jan 21, 2023

Looks good!

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.

3 participants