Skip to content

Commit a8cc33a

Browse files
kevincodex1openclaudeaiclaude
authored
fix(security): reject non-public peer URLs + prune poisoned peers (#78)
* fix(security): reject non-public peer URLs + prune poisoned peers The unauthenticated POST /api/v1/peers/announce only validated the http(s) scheme, so anyone could register loopback/private 'peers' (localhost:5432, localhost:22, internal admin URLs). On every push the node fans out signed sync-notify POSTs to all peers — turning this into an SSRF probe and burying the real peers under junk so node-origin repos stopped replicating. Observed on node.gitlawb.com: 126/194 peer rows were injected junk (fake DIDs z6MkScan…/z6MkAdmin…, localhost targets). - is_public_http_url(): reject non-http(s), loopback, unspecified, private (RFC1918 + ULA), link-local, and localhost/.local/.internal hosts. Applied at announce time. - db.prune_non_public_peers(): boot-time sweep removing already-injected rows, run from main() alongside prune_self_peers — self-heals poisoned tables on deploy. - tests for the validator (accept public, reject loopback/private/internal, incl. IPv6 [::1]). Co-Authored-By: OpenClaude <openclaude@gitlawb.com> * fix(security): address PR #78 review — build, redirect SSRF, validation gaps Resolves the P1/P2 review feedback on the peer-announce SSRF fix: - main.rs: revert the unrelated, incomplete `storage::build()` refactor back to the tigris init from main (the `storage` module never existed, breaking the build). Keeps only the prune_non_public_peers boot call. - Redirect bypass: set redirect::Policy::none() on all peer-facing reqwest clients (shared http_client, sync worker, ping_peer) so a peer cannot answer `302 -> http://127.0.0.1/` and bypass the public-URL check. ping_peer now reuses the shared no-redirect client instead of bare reqwest::get. - Move the public-URL guard into db::upsert_peer so the bootstrap announce-back path (main.rs) can no longer re-poison the table after the prune. - is_public_http_url: strip trailing-dot FQDNs (`localhost.`), fold IPv4-mapped/compatible IPv6 (`::ffff:127.0.0.1`, `::127.0.0.1`) to IPv4 before range checks, and reject CGNAT (100.64.0.0/10). Added tests for each. - prune_non_public_peers: collect bad DIDs then DELETE ... WHERE did = ANY($1) in one statement, so a transient error no longer abandons remaining rows. Follow-up (separate issue): DNS-resolution rebinding guard rejecting peer hostnames that resolve to private IPs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(security): satisfy rustfmt + don't drop redirect guard on builder failure - peers.rs: collapse the CGNAT/private check onto one line (cargo fmt --check). - sync.rs: replace the unwrap_or_else(reqwest::Client::new()) fallback with .expect(). The default client follows redirects, so the fallback silently reintroduced the SSRF vector Policy::none() exists to close (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: OpenClaude <openclaude@gitlawb.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 2202b00 commit a8cc33a

4 files changed

Lines changed: 190 additions & 5 deletions

File tree

‎crates/gitlawb-node/src/api/peers.rs‎

Lines changed: 137 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,78 @@ pub struct PeerResponse {
4040
pub reachable: bool,
4141
}
4242

43+
/// Whether a peer `http_url` is a public http(s) endpoint safe to register.
44+
/// Rejects non-http(s) schemes, loopback/unspecified/private/link-local IPs,
45+
/// and `localhost` / `.local` / `.internal` hostnames. Used at announce time
46+
/// and by the boot-time prune of already-poisoned rows.
47+
pub fn is_public_http_url(raw: &str) -> bool {
48+
let url = match reqwest::Url::parse(raw) {
49+
Ok(u) => u,
50+
Err(_) => return false,
51+
};
52+
if !matches!(url.scheme(), "http" | "https") {
53+
return false;
54+
}
55+
let mut host = match url.host_str() {
56+
Some(h) => h.to_ascii_lowercase(),
57+
None => return false,
58+
};
59+
// Drop a single trailing dot (FQDN root): `localhost.` resolves the same as
60+
// `localhost`, so normalize before the suffix/equality checks below.
61+
if let Some(stripped) = host.strip_suffix('.') {
62+
host = stripped.to_string();
63+
}
64+
if host.is_empty()
65+
|| host == "localhost"
66+
|| host.ends_with(".local")
67+
|| host.ends_with(".internal")
68+
{
69+
return false;
70+
}
71+
// host_str() keeps brackets on IPv6 literals (e.g. "[::1]"); strip them
72+
// before parsing as an IP.
73+
let ip_candidate = host.trim_start_matches('[').trim_end_matches(']');
74+
if let Ok(ip) = ip_candidate.parse::<std::net::IpAddr>() {
75+
// Reject loopback/unspecified on the literal as given — catches `::1`
76+
// and `::` before the IPv4-folding below (`::1`.to_ipv4() would
77+
// otherwise map to a non-loopback `0.0.0.1`).
78+
if ip.is_loopback() || ip.is_unspecified() {
79+
return false;
80+
}
81+
// Fold IPv4-mapped/compatible IPv6 (`::ffff:127.0.0.1`, `::127.0.0.1`)
82+
// down to IPv4 so the v4 range checks catch loopback/private addresses
83+
// smuggled in via an IPv6 literal, then re-check loopback/unspecified.
84+
let ip = match ip {
85+
std::net::IpAddr::V6(v6) => v6
86+
.to_ipv4_mapped()
87+
.or_else(|| v6.to_ipv4())
88+
.map(std::net::IpAddr::V4)
89+
.unwrap_or(ip),
90+
v4 => v4,
91+
};
92+
if ip.is_loopback() || ip.is_unspecified() {
93+
return false;
94+
}
95+
match ip {
96+
std::net::IpAddr::V4(v4) => {
97+
let o = v4.octets();
98+
// RFC1918 private, link-local, or CGNAT (100.64.0.0/10).
99+
if v4.is_private() || v4.is_link_local() || (o[0] == 100 && (o[1] & 0xc0) == 64) {
100+
return false;
101+
}
102+
}
103+
std::net::IpAddr::V6(v6) => {
104+
let s = v6.segments();
105+
// fc00::/7 (unique-local) or fe80::/10 (link-local)
106+
if (s[0] & 0xfe00) == 0xfc00 || (s[0] & 0xffc0) == 0xfe80 {
107+
return false;
108+
}
109+
}
110+
}
111+
}
112+
true
113+
}
114+
43115
/// GET /api/v1/peers
44116
pub async fn list_peers(State(state): State<AppState>) -> Result<Json<serde_json::Value>> {
45117
let peers = state.db.list_peers().await?;
@@ -83,10 +155,14 @@ pub async fn announce(
83155
);
84156
}
85157

86-
// Validate the URL is HTTP/HTTPS
87-
if !req.http_url.starts_with("http://") && !req.http_url.starts_with("https://") {
158+
// Validate the URL is a public http(s) endpoint. The announce route is
159+
// reachable unauthenticated (until all peers sign), so without this an
160+
// attacker can register loopback/private "peers" (localhost:5432, etc.)
161+
// and turn our outbound sync-notify fan-out into an SSRF probe — and bury
162+
// the real peers under junk so node-origin repos stop replicating.
163+
if !is_public_http_url(&req.http_url) {
88164
return Err(AppError::BadRequest(
89-
"http_url must start with http:// or https://".into(),
165+
"http_url must be a public http(s) URL (no loopback, private, or .internal/.local hosts)".into(),
90166
));
91167
}
92168

@@ -311,7 +387,13 @@ pub async fn ping_peer(
311387

312388
// Async ping
313389
let url = format!("{}/health", peer.http_url.trim_end_matches('/'));
314-
let ok = reqwest::get(&url)
390+
// Use the shared no-redirect client: bare `reqwest::get` follows redirects,
391+
// so a peer could answer with `302 -> http://127.0.0.1/` and turn the ping
392+
// into an SSRF probe.
393+
let ok = state
394+
.http_client
395+
.get(&url)
396+
.send()
315397
.await
316398
.map(|r| r.status().is_success())
317399
.unwrap_or(false);
@@ -324,3 +406,54 @@ pub async fn ping_peer(
324406
"reachable": ok,
325407
})))
326408
}
409+
410+
#[cfg(test)]
411+
mod tests {
412+
use super::is_public_http_url;
413+
414+
#[test]
415+
fn accepts_public_https_and_http() {
416+
assert!(is_public_http_url("https://node.gitlawb.com"));
417+
assert!(is_public_http_url("https://manila.gitlawb.com/"));
418+
assert!(is_public_http_url("http://203.0.113.10:7545"));
419+
}
420+
421+
#[test]
422+
fn rejects_loopback_private_and_internal() {
423+
for bad in [
424+
"http://localhost:7545",
425+
"http://127.0.0.1:5432/",
426+
"http://localhost:22/",
427+
"http://0.0.0.0:7545",
428+
"http://10.0.0.5:7545",
429+
"http://192.168.1.10/",
430+
"http://172.16.0.1:7545",
431+
"http://169.254.1.1/",
432+
"http://[::1]:7545",
433+
"http://gitlawb-node.internal:7545",
434+
"http://my-node.local/",
435+
"ftp://node.gitlawb.com",
436+
"not-a-url",
437+
"",
438+
// Trailing-dot FQDN of an internal host.
439+
"http://localhost./",
440+
// CGNAT (100.64.0.0/10).
441+
"http://100.64.0.1:7545",
442+
"http://100.127.255.255/",
443+
// IPv4-mapped / IPv4-compatible IPv6 smuggling private/loopback v4.
444+
"http://[::ffff:127.0.0.1]:7545",
445+
"http://[::ffff:10.0.0.1]/",
446+
"http://[::ffff:192.168.1.1]:7545",
447+
"http://[::127.0.0.1]/",
448+
] {
449+
assert!(!is_public_http_url(bad), "{bad:?} must be rejected");
450+
}
451+
}
452+
453+
#[test]
454+
fn accepts_public_outside_cgnat_range() {
455+
// 100.0.0.0/10 boundary: .63 and .128 are public, only 100.64-127 is CGNAT.
456+
assert!(is_public_http_url("http://100.63.255.255:7545"));
457+
assert!(is_public_http_url("http://100.128.0.1/"));
458+
}
459+
}

‎crates/gitlawb-node/src/db/mod.rs‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1626,6 +1626,14 @@ impl Db {
16261626

16271627
impl Db {
16281628
pub async fn upsert_peer(&self, did: &str, http_url: &str) -> Result<()> {
1629+
// Defense-in-depth at the DB boundary: both writers funnel through here
1630+
// — the announce handler and the bootstrap announce-back in main.rs.
1631+
// The latter has no announce-time check, so validating here is what
1632+
// stops a malicious bootstrap response from re-poisoning the table
1633+
// right after prune_non_public_peers cleaned it.
1634+
if !crate::api::peers::is_public_http_url(http_url) {
1635+
anyhow::bail!("refusing to register non-public peer http_url: {http_url}");
1636+
}
16291637
let now = Utc::now().to_rfc3339();
16301638
sqlx::query(
16311639
"INSERT INTO peers (did, http_url, last_seen, last_ping_ok, announced_at)
@@ -1679,6 +1687,29 @@ impl Db {
16791687
.await?;
16801688
Ok(result.rows_affected())
16811689
}
1690+
1691+
/// Remove peer rows whose `http_url` is not a public http(s) endpoint
1692+
/// (loopback/private/internal hosts injected via the open announce route).
1693+
/// Runs at boot to clean tables poisoned before announce-time validation
1694+
/// existed. Filtering is done in Rust to share one definition of "public"
1695+
/// with the announce handler, then deleted in a single statement so one
1696+
/// transient error can't abandon the remaining poisoned rows mid-loop.
1697+
pub async fn prune_non_public_peers(&self) -> Result<u64> {
1698+
let peers = self.list_peers().await?;
1699+
let bad_dids: Vec<String> = peers
1700+
.into_iter()
1701+
.filter(|p| !crate::api::peers::is_public_http_url(&p.http_url))
1702+
.map(|p| p.did)
1703+
.collect();
1704+
if bad_dids.is_empty() {
1705+
return Ok(0);
1706+
}
1707+
let result = sqlx::query("DELETE FROM peers WHERE did = ANY($1)")
1708+
.bind(&bad_dids)
1709+
.execute(&self.pool)
1710+
.await?;
1711+
Ok(result.rows_affected())
1712+
}
16821713
}
16831714

16841715
// ── Pinned CIDs ───────────────────────────────────────────────────────────────

‎crates/gitlawb-node/src/main.rs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,15 @@ async fn main() -> Result<()> {
100100
}
101101
}
102102

103+
// Prune peer rows with non-public hosts (loopback/private/internal) that
104+
// were injected via the unauthenticated announce route — they poison the
105+
// sync-notify fan-out (SSRF + crowding out real peers).
106+
match db.prune_non_public_peers().await {
107+
Ok(0) => {}
108+
Ok(n) => info!(removed = n, "pruned non-public (poisoned) peer rows"),
109+
Err(e) => warn!(err = %e, "prune_non_public_peers failed (non-fatal)"),
110+
}
111+
103112
// Ensure repos directory exists
104113
std::fs::create_dir_all(&config.repos_dir).context("failed to create repos directory")?;
105114

@@ -135,9 +144,14 @@ async fn main() -> Result<()> {
135144
None
136145
};
137146

147+
// Do not follow redirects: this client fans out to peer-supplied URLs
148+
// (sync trigger, profile/repo fetches). A peer answering `302 Location:
149+
// http://127.0.0.1/` would otherwise bypass the announce-time public-URL
150+
// check and turn the redirect into an SSRF.
138151
let http_client = Arc::new(
139152
reqwest::Client::builder()
140153
.timeout(std::time::Duration::from_secs(10))
154+
.redirect(reqwest::redirect::Policy::none())
141155
.build()?,
142156
);
143157

‎crates/gitlawb-node/src/sync.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,10 +109,17 @@ async fn run(
109109
let machine_id = std::env::var("FLY_MACHINE_ID").ok();
110110
// Bound each peer HTTP call (withheld-paths lookup + replica registration)
111111
// so a stalled peer cannot hang the worker.
112+
// No redirects: peer URLs are attacker-influenceable, so a 3xx to a
113+
// loopback/private address must not be followed (SSRF guard, matching the
114+
// shared http_client and announce-time validation).
115+
// Panic rather than fall back to reqwest::Client::new(): the default
116+
// builder follows redirects, which would silently reintroduce the SSRF
117+
// vector Policy::none() is here to close.
112118
let client = reqwest::Client::builder()
113119
.timeout(std::time::Duration::from_secs(30))
120+
.redirect(reqwest::redirect::Policy::none())
114121
.build()
115-
.unwrap_or_else(|_| reqwest::Client::new());
122+
.expect("failed to build no-redirect sync HTTP client");
116123
info!("sync worker started (auto_sync=true)");
117124
let mut interval = tokio::time::interval(std::time::Duration::from_secs(30));
118125
loop {

0 commit comments

Comments
 (0)