Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ECDSA signatures falsely reject points with leading zeros #290

Closed
mkeeter opened this issue Aug 29, 2024 · 1 comment
Closed

ECDSA signatures falsely reject points with leading zeros #290

mkeeter opened this issue Aug 29, 2024 · 1 comment

Comments

@mkeeter
Copy link
Contributor

mkeeter commented Aug 29, 2024

According to RFD 5656, an ecdsa_signature_blob is a pair of mpint values (r, s).

The encoding for mpint is specified in RFC 4251. Here's a notable requirement:

Unnecessary leading bytes with the value 0 or 255 MUST NOT be included.

This means that it's possible to see "short" mpint values, e.g. this is a valid ecdsa_signature_blob for the p256 curve (which normally has 32-byte coordinates):

0000002056A3EF43B4008E74169481F44C0BAB56EE651EDFDA4847DB37F992517A603544
0000001F01B02CDFB08568C910D7E7BB8942637D51ED48440E4E1F149CC483027F3D3F

ssh_key::Signature::decode does not currently handle short mpint values, returning encoding::Error::Length.


I haven't managed to generate such signatures when using ssh_key::PrivateKey::sign (not sure why!), but they can be generated using ssh-agent, e.g. when using a Yubikey to sign things.

I have an example program at mkeeter/ssh-agent-test which repeatedly signs random blobs using ssh-agent. It fails about 0.7% percent of the time, which is almost exactly the probability that at least one of the mpint values has a leading 0 byte (1 - (255/256)**2 = 0.0077).

Looking through the code, there are two places where things go wrong:

  • ecdsa_sig_size checks that the mpint be exactly the curve length; it should also accept shorter values
  • p256_signature_from_openssh_bytes (and equivalent) calls p256::FieldBytes::try_from(..) on Mpint::positive_bytes, which fails if the Mpint is short; it should instead pad with leading zeros
@mkeeter
Copy link
Contributor Author

mkeeter commented Aug 29, 2024

Fixed by #291

tarcieri pushed a commit that referenced this issue Sep 5, 2024
Leading zeros must be stripped from mpint values (per RFC 4251), meaning it's
possible to see "short" `mpint` values when using them to construct signatures.

These values must be padded with leading zeros before being used in
`pXYZ::ecdsa::Signature`, which expect exact-size arrays.

See #290 for details.
@mkeeter mkeeter closed this as completed Sep 5, 2024
tarcieri pushed a commit that referenced this issue Oct 15, 2024
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

No branches or pull requests

1 participant