Accept ECDSA signatures shorter than 71 bytes#69
Conversation
| let sighash_type = | ||
| EcdsaSighashType::from_consensus(encoded_signature[signature_length - 1] as u32); |
There was a problem hiding this comment.
| let sighash_type = | |
| EcdsaSighashType::from_consensus(encoded_signature[signature_length - 1] as u32); | |
| let sighash_type = | |
| EcdsaSighashType::from_standard(encoded_signature[signature_length - 1] as u32); |
Thank you for your work on this.
I think from_standard is a better fit here. from_standard matches the byte strictly and errors on anything non-standard, whereas from_consensus forces unknown bytes to All. Open to your thought.
There was a problem hiding this comment.
Good call. from_standard is the right strictness for a verifier; from_consensus would fold non-standard bytes to All and accept them. It also lines up with how the taproot path parses its flag.
One detail: from_standard returns Result<_, NonStandardSighashTypeError>, which doesn't match the existing SigHashTypeInvalid variant (typed to InvalidSighashTypeError), so I added a SigHashTypeNonStandard variant and .context()'d onto it, mirroring the taproot handling. Pushed. Thanks for the review!
aagbotemi
left a comment
aagbotemi
left a comment
There was a problem hiding this comment.
ACK 742487f
@johnzilla could you also sign your commits, to match the convention used by the rest of the codebase?
ECDSA signatures are not fixed length: a low-S DER signature is shorter than the usual 71 or 72 bytes when r or s serialize to fewer than 32 bytes (roughly 1% of signatures). verify_full_p2wpkh matched the encoded signature length against `71 | 72` and rejected everything else with Error::SignatureLength, so a valid signature that sign_simple itself can produce was rejected by verify_simple. Split the trailing sighash flag off and let from_der validate the remainder rather than gating on length. The empty case is guarded so the slice index cannot panic.
from_consensus folds unrecognized sighash bytes to All, so a signature with a non-standard flag would pass the == All check. Parse with from_standard instead, which errors on anything that isn't a standard sighash type, matching the strictness of the taproot path. Adds a SigHashTypeNonStandard error variant for the parse failure. Co-authored-by: aagbotemi <63763418+aagbotemi@users.noreply.github.com>
johnzilla
force-pushed
the
fix-ecdsa-signature-length
branch
from
742487f to
d33307e
Compare
|
Heads up: I force-pushed to sign both commits with my SSH key (they show as Verified now). No content change from what you reviewed, just the signatures added. |
Co-authored-by: Abiodun Awoyemi <63763418+aagbotemi@users.noreply.github.com>
2 participants
verify_full_p2wpkhgated the encoded ECDSA signature length on71 | 72and rejected everything else withError::SignatureLength. ECDSA signatures aren't fixed length — a low-S DER signature is shorter than 71 bytes whenrorsserialize to fewer than 32 bytes (~1% of signatures) — so valid signatures thatsign_simpleitself produces were rejected byverify_simple.This peels the trailing sighash flag off and lets
from_dervalidate the remainder instead of gating on length, with an empty-input guard so the slice index can't panic. The taproot path is unchanged.Adds a P2WPKH regression vector (a 70-byte signature); it fails on
masterand passes with this change.cargo fmt --check,cargo clippy --all --all-targets -- --deny warnings, andcargo test --allare green.Fixes #68.