HttpSignature::parse locates the component list with two independent searches and then slices
between them (crates/gitlawb-core/src/http_sig.rs:60-67):
let open = rest
.find('(')
.ok_or_else(|| Error::HttpSignature("missing '(' in Signature-Input".into()))?;
let close = rest
.find(')')
.ok_or_else(|| Error::HttpSignature("missing ')' in Signature-Input".into()))?;
let components_str = &rest[open + 1..close];
Nothing requires open < close. When ) appears first the range is reversed and the slice panics.
Note the function is otherwise careful: every other malformed shape returns a typed Error and the
caller turns it into a 400. This one input class walks straight past that.
Measured
The indexing above, extracted verbatim into a standalone binary (a yanked transitive dep blocks
building a scratch crate against gitlawb-core itself, so this is the five cited lines copied exactly
rather than a link against the crate):
reversed parens -> PANIC
close-before-open w/ params -> PANIC
well-formed (control) -> Ok(("\"@method\"", ";keyid=\"k\""))
missing paren (control) -> Err(missing '(')
bad prefix (control) -> Err(must start with 'sig1=')
The three controls are the point: the panic is specific to the reversed-range condition, not a
blanket failure of malformed input.
Reachability
Pre-authentication, and the caller needs no credentials of any kind.
crates/gitlawb-node/src/auth/mod.rs:88 calls it inside require_signature, before any key is
resolved or checked, which is what makes this a parse of fully untrusted bytes. And
optional_signature (auth/mod.rs:251-257) delegates to require_signature whenever the request
merely carries signature headers:
let has_signature_headers = request.headers().contains_key("signature-input")
|| request.headers().contains_key("signature");
if has_signature_headers {
return require_signature(request, next).await;
}
So the panic is reachable on routes that are otherwise anonymous, by sending:
Signature-Input: sig1=)(
Signature: sig1=:AAAA:
Blast radius, stated honestly
grep -rn "CatchPanic\|catch_unwind" crates/gitlawb-node/src/ finds only a test helper in
api/repos.rs, so there is no panic-catching layer on the router, and the release profile
(Cargo.toml:59-61) sets lto and strip but not panic = abort. So this unwinds and kills the
request task rather than the process: the caller gets a dropped connection instead of the 400 the
code intends, plus a logged backtrace per attempt.
That is why I am filing this medium rather than high. It is trivially scriptable and it is pre-auth,
but it costs one request per attempt and corrupts no state, which puts it alongside #317 (anonymous
malformed limit yielding a raw DB error) rather than alongside a resource-exhaustion issue. I have
NOT driven this end to end through a running node; the panic and the call chain are established by
execution and by reading respectively, and the "axum does not catch it" step is inferred from the
absence of a catch layer.
Fix direction
Require open < close before slicing, and return the existing Error::HttpSignature when it does
not hold, which keeps every malformed input on the one path the caller already handles. A regression
test should drive sig1=)( and assert a 400 rather than a panic, since a test that only asserts
is_err() would pass today by aborting the test thread.
Worth noting for whoever picks this up: PRs #306 and #261 both touch http_sig.rs and both add tests
around parse, but neither changes this indexing, so the panic survives both. Coordinating with them
is probably worth more than the fix itself, which is one comparison.
HttpSignature::parselocates the component list with two independent searches and then slicesbetween them (
crates/gitlawb-core/src/http_sig.rs:60-67):Nothing requires
open < close. When)appears first the range is reversed and the slice panics.Note the function is otherwise careful: every other malformed shape returns a typed
Errorand thecaller turns it into a 400. This one input class walks straight past that.
Measured
The indexing above, extracted verbatim into a standalone binary (a yanked transitive dep blocks
building a scratch crate against
gitlawb-coreitself, so this is the five cited lines copied exactlyrather than a link against the crate):
The three controls are the point: the panic is specific to the reversed-range condition, not a
blanket failure of malformed input.
Reachability
Pre-authentication, and the caller needs no credentials of any kind.
crates/gitlawb-node/src/auth/mod.rs:88calls it insiderequire_signature, before any key isresolved or checked, which is what makes this a parse of fully untrusted bytes. And
optional_signature(auth/mod.rs:251-257) delegates torequire_signaturewhenever the requestmerely carries signature headers:
So the panic is reachable on routes that are otherwise anonymous, by sending:
Blast radius, stated honestly
grep -rn "CatchPanic\|catch_unwind" crates/gitlawb-node/src/finds only a test helper inapi/repos.rs, so there is no panic-catching layer on the router, and the release profile(
Cargo.toml:59-61) setsltoandstripbut notpanic = abort. So this unwinds and kills therequest task rather than the process: the caller gets a dropped connection instead of the 400 the
code intends, plus a logged backtrace per attempt.
That is why I am filing this medium rather than high. It is trivially scriptable and it is pre-auth,
but it costs one request per attempt and corrupts no state, which puts it alongside #317 (anonymous
malformed
limityielding a raw DB error) rather than alongside a resource-exhaustion issue. I haveNOT driven this end to end through a running node; the panic and the call chain are established by
execution and by reading respectively, and the "axum does not catch it" step is inferred from the
absence of a catch layer.
Fix direction
Require
open < closebefore slicing, and return the existingError::HttpSignaturewhen it doesnot hold, which keeps every malformed input on the one path the caller already handles. A regression
test should drive
sig1=)(and assert a 400 rather than a panic, since a test that only assertsis_err()would pass today by aborting the test thread.Worth noting for whoever picks this up: PRs #306 and #261 both touch
http_sig.rsand both add testsaround
parse, but neither changes this indexing, so the panic survives both. Coordinating with themis probably worth more than the fix itself, which is one comparison.