Repository navigation
Outbound clients never validate the address a hostname resolves to #340
Description
is_public_http_url (crates/gitlawb-node/src/api/peers.rs:109) classifies a URL by its host string. When the host is an IP literal that works. When it is a name, nothing resolves it, so a hostname whose A or AAAA record points at 127.0.0.1, 169.254.169.254, or RFC1918 space passes every check we have.
POST /api/v1/peers/announce is unauthenticated, and the resulting http_url is used for outbound sync-notify fan-out, so the caller who chooses the name is not required to be anyone in particular.
There is a second half. Validation and connection resolve independently, so even a name that resolves to public space at announce time can resolve somewhere else when the request is actually made. A syntactic gate cannot see that by construction, which is why OWASP's SSRF guidance treats hostname-only validation as insufficient rather than as a partial win.
Why this is the unfinished third step
We have closed SSRF here twice, both times at the syntactic layer. #78 added the host-class gate and the peer prune; #93 and #140 stopped the gossip path following redirects into private space. Neither addressed what a name resolves to. Nothing in the tree records a decision that syntax-only is sufficient, and #333's description states the same conclusion from the other direction, deferring resolved-address validation to the fetch worker that will consume upstream_url.
Fix direction
reqwest::ClientBuilder::dns_resolver takes an Arc<R: Resolve>, and reqwest::dns carries no feature gate, so it is available in our default-features = false build of 0.12.28. A resolver that applies the address policy and drops non-public results closes the name case and the rebinding case together, because the check then happens on the addresses actually used to connect.
Two things to get right:
- Reuse the address checks we already have rather than adding a dependency. Extract the IP-classification half of
is_public_http_urlinto a function overIpAddrand call it from both the existing literal path and the resolver.IpAddr::is_globalis still nightly-only, so std is not an option at our MSRV, and our hand-rolled ranges are already covered by tests. - Both clients, not one.
build_http_client(crates/gitlawb-node/src/main.rs:1173) is the named shared builder, andsync.rs:149builds a second client of its own. Both setredirect::Policy::none(). A resolver policy attached only to the first leaves the sync worker on the old behavior, which is the same shape as the bug fix(node,gossip): route gossip HTTP through the no-redirect client (#93) #140 fixed.
Keep the string checks. They stay useful as a cheap reject before any lookup happens, and defense in depth is the point; they are just not the boundary.
The narrower predicate gaps in that same function are tracked separately.
Activity