Skip to content

ES256: return errors instead of panicking on invalid byte conversions#3722

Open
jsdanielh wants to merge 1 commit intoalbatrossfrom
jsdanielh/es256
Open

ES256: return errors instead of panicking on invalid byte conversions#3722
jsdanielh wants to merge 1 commit intoalbatrossfrom
jsdanielh/es256

Conversation

@jsdanielh
Copy link
Copy Markdown
Member

Replace the From<[u8; N]> / From<&[u8; N]> impls on ES256PublicKey and ES256Signature with TryFrom, matching the existing Commitment precedent in the multisig module. The previous impls called from_bytes(...).expect(...), which panics on inputs that p256 rejects (invalid SEC1 tag byte for the public key; zero or out-of-range r/s scalars for the signature). No current call site feeds untrusted bytes through these impls, but the TryFrom form removes the footgun.

Also remove the Default impl on ES256Signature: it attempted to construct a signature from all-zero bytes, which p256 rejects (panicking with a misleading "Invalid slice length" message). The impl had no callers.

Pull request checklist

  • All tests pass. The project builds and runs.
  • I have resolved any merge conflicts.
  • I have resolved all clippy and rustfmt warnings.

Replace the `From<[u8; N]>` / `From<&[u8; N]>` impls on `ES256PublicKey`
and `ES256Signature` with `TryFrom`, matching the existing `Commitment`
precedent in the multisig module. The previous impls called
`from_bytes(...).expect(...)`, which panics on inputs that `p256`
rejects (invalid SEC1 tag byte for the public key; zero or
out-of-range `r`/`s` scalars for the signature). No current call site
feeds untrusted bytes through these impls, but the `TryFrom` form
removes the footgun.

Also remove the `Default` impl on `ES256Signature`: it attempted to
construct a signature from all-zero bytes, which `p256` rejects
(panicking with a misleading "Invalid slice length" message). The
impl had no callers.
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