Skip to content

Outbound clients never validate the address a hostname resolves to #340

Description

@beardthelion

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:

  1. Reuse the address checks we already have rather than adding a dependency. Extract the IP-classification half of is_public_http_url into a function over IpAddr and call it from both the existing literal path and the resolver. IpAddr::is_global is still nightly-only, so std is not an option at our MSRV, and our hand-rolled ranges are already covered by tests.
  2. Both clients, not one. build_http_client (crates/gitlawb-node/src/main.rs:1173) is the named shared builder, and sync.rs:149 builds a second client of its own. Both set redirect::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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    crate:nodegitlawb-node — the serving node and REST APIkind:securityVulnerability fix or hardeningsev:criticalData loss, exploitable security, or crash loopsubsystem:apiNode REST API request/response surfacesubsystem:peersPeer announce, discovery, and registrysubsystem:replicationMirror, replica, and cross-node sync

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions