fix: avoid panic on invalid signature input#29
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the HTTP Signature header parsing path against attacker-controlled inputs by removing two panic conditions and converting them into normal parse errors, preventing trivial denial-of-service crashes during verification.
Changes:
- Make
created/expiresparsing reject negative SFV integers without panicking by using fallibleu64conversion. - Replace derived component parsing (
@...) frompanic!-based lookup toTryFrom<&str>returningHttpSigError::InvalidComponentName. - Add regression tests ensuring malformed
Signature-Inputvalues return errors instead of panicking.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| httpsig/src/signature_params.rs | Replaces unwrap() timestamp conversion with fallible conversion and error propagation. |
| httpsig/src/signature_base.rs | Adds regression tests covering negative timestamps and unknown derived components. |
| httpsig/src/message_component/component_name.rs | Converts derived component parsing to return InvalidComponentName instead of panicking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _ => { | ||
| return Err(HttpSigError::InvalidComponentName(format!( | ||
| "Invalid derived component: {val}" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Thanks for the fix, and sorry for the long delay in reviewing!
I've verified this locally against develop:
- Both panics reproduce on
developas described —created=-1/expires=-1hits thetry_into().unwrap()insignature_params.rs, and an unknown@-prefixed component hits thepanic!incomponent_name.rs. Since both are reachable from attacker-controlled headers before any signature check, this is indeed a DoS vector. cargo test --all-featurespasses on this branch (36 + 35 tests, including the two new regression tests), andcargo fmt --checkis clean.- The only call site of
DerivedComponentName::from(&str)was the one updated here, so nothing is left behind.
One note: replacing From<&str> with TryFrom<&str> on the publicly exported DerivedComponentName is technically a breaking API change, but that's acceptable at 0.0.x. I'll note it in the release.
Nit: the comment in signature_params.rs has a trailing ;. typo ("non-negative integers;."). Not blocking.
LGTM.
Fixes two denial-of-service bugs in the signature verification path.
The library parses the Signature and Signature-Input headers of incoming
requests before any cryptographic check runs, so the bytes it sees are fully
attacker-controlled. Two of those parse steps could panic on malformed input:
A negative "created" or "expires" timestamp. These are valid Structured
Field integers but were converted to u64 with .unwrap(), so a value like
created=-1 panicked.
An undefined @-prefixed derived component name. The
name lookup called panic! on anything it didn't recognize.
Either one lets an unauthenticated client crash the verifier just by sending a
crafted header.
The fix makes both paths return a normal parse error instead of panicking.