From 621bba00f2d1238ad99dbc3c9d7c685ad0b7daf5 Mon Sep 17 00:00:00 2001 From: Jean Date: Mon, 20 Jul 2026 17:35:25 +0000 Subject: [PATCH] Fix review findings: direction-bind the gossip handshake proof, whitespace-tokenize log redaction, show ranked STATS to viewers, single-pass render, code-before-message email templating --- api/src/email.rs | 6 ++- api/src/lib.rs | 24 +++++++++-- modules/gameserv/src/lib.rs | 8 ++-- src/gossip.rs | 83 ++++++++++++++++++++++++------------- src/link.rs | 17 ++++---- 5 files changed, 92 insertions(+), 46 deletions(-) diff --git a/api/src/email.rs b/api/src/email.rs index a41beba..99916ba 100644 --- a/api/src/email.rs +++ b/api/src/email.rs @@ -24,13 +24,17 @@ fn brand_mark(brand: &str, accent: &str, logo: &str) -> String { } fn render(brand: &str, accent: &str, logo: &str, title: &str, message: &str, code: &str, note: &str) -> String { + // `message` carries the account name (attacker-influenceable). Substitute every + // OTHER slot first and `{{message}}` LAST, so a message that literally contains + // `{{code}}`/`{{note}}` (an account named that) is inserted verbatim rather than + // splicing in the real code. BASE.replace("{{brand_mark}}", &brand_mark(brand, accent, logo)) .replace("{{brand}}", &escape(brand)) .replace("{{accent}}", &escape(accent)) .replace("{{title}}", &escape(title)) - .replace("{{message}}", &escape(message)) .replace("{{code}}", &escape(code)) .replace("{{note}}", &escape(note)) + .replace("{{message}}", &escape(message)) } pub fn reset(brand: &str, accent: &str, logo: &str, account: &str, code: &str, lang: &str) -> Mail { diff --git a/api/src/lib.rs b/api/src/lib.rs index 131d2d9..e93b8c2 100644 --- a/api/src/lib.rs +++ b/api/src/lib.rs @@ -497,13 +497,29 @@ pub fn render(lang: &str, msgid: &str, args: &[(&str, String)]) -> String { .and_then(|m| m.get(msgid)) .map(String::as_str) .unwrap_or(msgid); - if args.is_empty() { + if args.is_empty() || !template.contains('{') { return template.to_string(); } - let mut out = template.to_string(); - for (name, val) in args { - out = out.replace(&format!("{{{name}}}"), val); + // Single left-to-right pass: a substituted value is copied to the output and + // never re-scanned, so a value that itself contains `{name}` (e.g. a nick like + // "{game}") can't trigger a second substitution. An unknown `{x}` (no matching + // arg, e.g. the literal `{ON|OFF}`) is left verbatim, as before. + let mut out = String::with_capacity(template.len() + 16); + let mut rest = template; + while let Some(open) = rest.find('{') { + out.push_str(&rest[..open]); + let Some(rel_close) = rest[open..].find('}') else { + break; // unbalanced '{' — emit the remainder verbatim below + }; + let close = open + rel_close; + let name = &rest[open + 1..close]; + match args.iter().find(|(n, _)| *n == name) { + Some((_, val)) => out.push_str(val), + None => out.push_str(&rest[open..=close]), + } + rest = &rest[close + 1..]; } + out.push_str(rest); out } diff --git a/modules/gameserv/src/lib.rs b/modules/gameserv/src/lib.rs index 2b1e020..982c58f 100644 --- a/modules/gameserv/src/lib.rs +++ b/modules/gameserv/src/lib.rs @@ -463,9 +463,11 @@ fn push_stats(counters: &[(String, u64)], account: &str, to_nick: &str, me: &str _ => read_stats(counters, gt, account), }; let payload = format!("GS$ {} {} r={} p={} w={} l={} d={}", account, gt.wire(), s.rank(), s.points(), s.wins, s.losses, s.draws); - // push_stats is only called for a registered side (a_reg/b_reg), so the - // account guard in push_tag always applies here. - push_tag(me, net, to_nick, account, true, &payload, ctx); + // Ranked stats are PUBLIC leaderboard data and are also pushed to a viewer + // who queried someone ELSE (STATS ), so `to_nick` is not the subject + // account — don't apply the board-state account guard here (that guard is + // only for the private mid-game board in push_one). + push_tag(me, net, to_nick, account, false, &payload, ctx); } } diff --git a/src/gossip.rs b/src/gossip.rs index f967b26..0d2ef2b 100644 --- a/src/gossip.rs +++ b/src/gossip.rs @@ -51,12 +51,17 @@ fn make_nonce() -> String { STANDARD.encode(b) } -// Proof of holding the shared secret: HMAC-SHA256(secret, the peer's nonce). The +// Proof of holding the shared secret: HMAC-SHA256(secret, direction ‖ nonce). The // secret itself never crosses the wire — the peer verifies this against the nonce -// it chose, so an eavesdropper (or a peer with the wrong secret) can't reproduce -// it and can't replay it on a later connection (fresh nonce each time). -fn auth_proof(secret: &str, nonce: &str) -> String { +// it chose. `direction` ("L" for the listener's proof, "D" for the dialer's) is +// mixed in so the two sides' proofs are over DIFFERENT inputs: a peer can neither +// reflect our own nonce+proof back to us, nor use a second connection as an oracle +// to have us sign the value it needs — both would need the other direction's +// label, which only the other role ever produces. +fn auth_proof(secret: &str, direction: &str, nonce: &str) -> String { let mut mac = Hmac::::new_from_slice(secret.as_bytes()).expect("HMAC accepts any key length"); + mac.update(direction.as_bytes()); + mac.update(b"\0"); mac.update(nonce.as_bytes()); STANDARD.encode(mac.finalize().into_bytes()) } @@ -141,11 +146,14 @@ async fn listen(bind: String, engine: Shared, secret: String, origin: String, ou tokio::spawn(async move { let _permit = permit; // released when the session ends let served = match acceptor { - Some(acc) => match acc.accept(stream).await { - Ok(tls) => session(tls, engine, secret, origin, outbound).await, - Err(e) => return tracing::debug!(%e, %addr, "gossip tls accept failed"), + // Bound the TLS handshake too, so a peer that opens the socket but + // never finishes negotiating can't pin its session permit forever. + Some(acc) => match tokio::time::timeout(HANDSHAKE_TIMEOUT, acc.accept(stream)).await { + Ok(Ok(tls)) => session(tls, engine, secret, origin, outbound, true).await, + Ok(Err(e)) => return tracing::debug!(%e, %addr, "gossip tls accept failed"), + Err(_) => return tracing::debug!(%addr, "gossip tls accept timed out"), }, - None => session(stream, engine, secret, origin, outbound).await, + None => session(stream, engine, secret, origin, outbound, true).await, }; if let Err(e) = served { tracing::debug!(%e, "gossip session ended"); @@ -163,12 +171,12 @@ async fn dial(peer: Peer, engine: Shared, secret: String, origin: String, outbou let served = match &connector { Some(conn) => match ServerName::try_from(peer.name.clone()) { Ok(name) => match conn.connect(name, stream).await { - Ok(tls) => session(tls, engine.clone(), secret.clone(), origin.clone(), outbound.clone()).await, + Ok(tls) => session(tls, engine.clone(), secret.clone(), origin.clone(), outbound.clone(), false).await, Err(e) => Err(anyhow::anyhow!("tls connect: {e}")), }, Err(e) => Err(anyhow::anyhow!("bad peer TLS name {:?}: {e}", peer.name)), }, - None => session(stream, engine.clone(), secret.clone(), origin.clone(), outbound.clone()).await, + None => session(stream, engine.clone(), secret.clone(), origin.clone(), outbound.clone(), false).await, }; if let Err(e) = served { tracing::debug!(%e, addr = %peer.addr, "gossip session ended"); @@ -227,7 +235,7 @@ async fn read_handshake(reader: &mut R) -> Option } // One peer connection: authenticate, then run anti-entropy until it drops. -async fn session(stream: S, engine: Shared, secret: String, origin: String, outbound: Outbound) -> anyhow::Result<()> +async fn session(stream: S, engine: Shared, secret: String, origin: String, outbound: Outbound, listener: bool) -> anyhow::Result<()> where S: AsyncRead + AsyncWrite + Send + 'static, { @@ -246,10 +254,12 @@ where // Handshake: a mutual challenge-response over the shared secret, so the secret // itself never crosses the wire. Each side sends a random nonce, then proves it - // holds the secret by returning HMAC(secret, the peer's nonce); each verifies - // the peer's proof against the nonce it chose. A wrong secret (or an - // eavesdropper) can't produce a valid proof, and a fresh nonce per link stops - // replay. + // holds the secret by returning HMAC(secret, its-role ‖ the peer's nonce); each + // verifies the peer's proof against the nonce it chose and the peer's role. The + // per-role label (listener "L" vs dialer "D") is what stops a reflection or + // oracle attack — without it, a peer with no secret could bounce our own + // nonce+proof back and authenticate. A fresh nonce per link stops replay. + let (my_dir, peer_dir) = if listener { ("L", "D") } else { ("D", "L") }; let my_nonce = make_nonce(); let _ = send(&tx, &Msg::Hello { origin, nonce: my_nonce.clone() }).await; let peer_nonce = match read_handshake(&mut reader).await { @@ -259,9 +269,15 @@ where anyhow::bail!("peer failed to complete the handshake"); } }; - let _ = send(&tx, &Msg::Auth { proof: auth_proof(&secret, &peer_nonce) }).await; + // A peer echoing our own nonce back has nothing to prove — refuse it outright + // (the direction labels already defeat reflection; this is belt-and-braces). + if peer_nonce == my_nonce { + writer.abort(); + anyhow::bail!("gossip peer reused our nonce"); + } + let _ = send(&tx, &Msg::Auth { proof: auth_proof(&secret, my_dir, &peer_nonce) }).await; match read_handshake(&mut reader).await { - Some(Msg::Auth { proof }) if ct_str_eq(&proof, &auth_proof(&secret, &my_nonce)) => {} + Some(Msg::Auth { proof }) if ct_str_eq(&proof, &auth_proof(&secret, peer_dir, &my_nonce)) => {} _ => { writer.abort(); anyhow::bail!("bad gossip handshake"); @@ -362,6 +378,15 @@ mod tests { use crate::engine::db::Db; use echo_nickserv::NickServ; + // The listener's and the dialer's proof over the same nonce MUST differ, so a + // secretless peer can't reflect one side's proof (or oracle it on a second + // connection) as the other side's expected value. + #[test] + fn handshake_proof_is_direction_separated() { + assert_ne!(auth_proof("s3cret", "L", "abc"), auth_proof("s3cret", "D", "abc")); + assert_eq!(auth_proof("s3cret", "L", "abc"), auth_proof("s3cret", "L", "abc")); + } + fn engine(origin: &str, tag: &str) -> (Shared, Outbound) { let path = std::env::temp_dir().join(format!("echo-gossip-{tag}.jsonl")); let _ = std::fs::remove_file(&path); @@ -382,8 +407,8 @@ mod tests { a.lock().await.test_register("alice"); let (ca, cb) = tokio::io::duplex(64 * 1024); - let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx)); - let sb = tokio::spawn(session(cb, b.clone(), "s3cret".into(), "B".into(), btx)); + let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx, true)); + let sb = tokio::spawn(session(cb, b.clone(), "s3cret".into(), "B".into(), btx, false)); let mut converged = false; for _ in 0..100 { @@ -406,8 +431,8 @@ mod tests { let (b, btx) = engine("B", "push-b"); let (ca, cb) = tokio::io::duplex(64 * 1024); - let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx)); - let sb = tokio::spawn(session(cb, b.clone(), "s3cret".into(), "B".into(), btx)); + let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx, true)); + let sb = tokio::spawn(session(cb, b.clone(), "s3cret".into(), "B".into(), btx, false)); tokio::time::sleep(Duration::from_millis(200)).await; // let both subscribe a.lock().await.test_register("late"); @@ -435,8 +460,8 @@ mod tests { b.lock().await.test_register_pw("alice", "from-b"); let (ca, cb) = tokio::io::duplex(64 * 1024); - let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx)); - let sb = tokio::spawn(session(cb, b.clone(), "s3cret".into(), "B".into(), btx)); + let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx, true)); + let sb = tokio::spawn(session(cb, b.clone(), "s3cret".into(), "B".into(), btx, false)); let mut converged = false; for _ in 0..100 { @@ -462,8 +487,8 @@ mod tests { a.lock().await.test_register_channel("#secret", "alice"); let (ca, cb) = tokio::io::duplex(64 * 1024); - let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx)); - let sb = tokio::spawn(session(cb, b.clone(), "s3cret".into(), "B".into(), btx)); + let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx, true)); + let sb = tokio::spawn(session(cb, b.clone(), "s3cret".into(), "B".into(), btx, false)); // Wait for the account to converge (proves the link works), then assert // the channel never crossed it. @@ -492,8 +517,8 @@ mod tests { a.lock().await.test_register("alice"); let (ca, cb) = tokio::io::duplex(64 * 1024); - let sa = tokio::spawn(session(ca, a.clone(), "right".into(), "A".into(), atx)); - let sb = tokio::spawn(session(cb, b.clone(), "wrong".into(), "B".into(), btx)); + let sa = tokio::spawn(session(ca, a.clone(), "right".into(), "A".into(), atx, true)); + let sb = tokio::spawn(session(cb, b.clone(), "wrong".into(), "B".into(), btx, false)); let _ = tokio::time::timeout(Duration::from_secs(1), async { let _ = sa.await; @@ -538,8 +563,8 @@ mod tests { let (sside, cside) = tokio::io::duplex(64 * 1024); let name = ServerName::try_from("echo").unwrap(); let (server, client) = tokio::join!(acceptor.accept(sside), connector.connect(name, cside)); - let sa = tokio::spawn(session(server.expect("tls accept"), a.clone(), "s3cret".into(), "A".into(), atx)); - let sb = tokio::spawn(session(client.expect("tls connect"), b.clone(), "s3cret".into(), "B".into(), btx)); + let sa = tokio::spawn(session(server.expect("tls accept"), a.clone(), "s3cret".into(), "A".into(), atx, true)); + let sb = tokio::spawn(session(client.expect("tls connect"), b.clone(), "s3cret".into(), "B".into(), btx, false)); let mut converged = false; for _ in 0..100 { diff --git a/src/link.rs b/src/link.rs index f01234f..6f74014 100644 --- a/src/link.rs +++ b/src/link.rs @@ -55,20 +55,19 @@ fn redact(line: &str) -> Cow<'_, str> { let after = p + kw.len(); if let Some(c) = line[after..].find(" :") { let tstart = after + c + 2; - let mut words = line[tstart..].split(' '); - let first = words.next().unwrap_or(""); - let mut cmd = first.to_ascii_lowercase(); - let mut argstart = tstart + first.len(); + // Tokenise the trailer exactly like the dispatcher (`split_whitespace`), + // so a TAB or a leading/collapsed space can't sneak a credential past + // redaction while still reaching the command handler. + let mut words = line[tstart..].split_whitespace(); + let mut cmd = words.next().unwrap_or("").to_ascii_lowercase(); // "NS ..." / "NICKSERV ..." — unwrap one level. if matches!(cmd.as_str(), "ns" | "nickserv") { - if let Some(w2) = words.next() { - cmd = w2.to_ascii_lowercase(); - argstart += 1 + w2.len(); - } + cmd = words.next().unwrap_or("").to_ascii_lowercase(); } let is_set_password = cmd == "set" && words.next().map(|w| w.eq_ignore_ascii_case("password")).unwrap_or(false); - if (SECRET_CMDS.contains(&cmd.as_str()) || is_set_password) && argstart < line.len() { + // Over-redact the whole trailer when the command is credential-bearing. + if SECRET_CMDS.contains(&cmd.as_str()) || is_set_password { return format!("{} :[REDACTED]", line[..tstart].trim_end_matches(" :")).into(); } }