fedserv/echo
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source
This commit is contained in:
Jean Chevronnet 2026-07-20 17:35:25 +00:00
parent 829a3916fd
commit 621bba00f2
No known key found for this signature in database
5 changed files with 92 additions and 46 deletions
Download patch file Download diff file Expand all files Collapse all files

6
api/src/email.rs
View file

@ -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 { 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)) BASE.replace("{{brand_mark}}", &brand_mark(brand, accent, logo))
.replace("{{brand}}", &escape(brand)) .replace("{{brand}}", &escape(brand))
.replace("{{accent}}", &escape(accent)) .replace("{{accent}}", &escape(accent))
.replace("{{title}}", &escape(title)) .replace("{{title}}", &escape(title))
.replace("{{message}}", &escape(message))
.replace("{{code}}", &escape(code)) .replace("{{code}}", &escape(code))
.replace("{{note}}", &escape(note)) .replace("{{note}}", &escape(note))
.replace("{{message}}", &escape(message))
} }
pub fn reset(brand: &str, accent: &str, logo: &str, account: &str, code: &str, lang: &str) -> Mail { pub fn reset(brand: &str, accent: &str, logo: &str, account: &str, code: &str, lang: &str) -> Mail {

24
api/src/lib.rs
View file

@ -497,13 +497,29 @@ pub fn render(lang: &str, msgid: &str, args: &[(&str, String)]) -> String {
.and_then(|m| m.get(msgid)) .and_then(|m| m.get(msgid))
.map(String::as_str) .map(String::as_str)
.unwrap_or(msgid); .unwrap_or(msgid);
if args.is_empty() { if args.is_empty() || !template.contains('{') {
return template.to_string(); return template.to_string();
} }
let mut out = template.to_string(); // Single left-to-right pass: a substituted value is copied to the output and
for (name, val) in args { // never re-scanned, so a value that itself contains `{name}` (e.g. a nick like
out = out.replace(&format!("{{{name}}}"), val); // "{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 out
} }

8
modules/gameserv/src/lib.rs
View file

@ -463,9 +463,11 @@ fn push_stats(counters: &[(String, u64)], account: &str, to_nick: &str, me: &str
_ => read_stats(counters, gt, account), _ => 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); 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 // Ranked stats are PUBLIC leaderboard data and are also pushed to a viewer
// account guard in push_tag always applies here. // who queried someone ELSE (STATS <nick>), so `to_nick` is not the subject
push_tag(me, net, to_nick, account, true, &payload, ctx); // 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);
} }
} }

83
src/gossip.rs
View file

@ -51,12 +51,17 @@ fn make_nonce() -> String {
STANDARD.encode(b) 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 // 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 chose. `direction` ("L" for the listener's proof, "D" for the dialer's) is
// it and can't replay it on a later connection (fresh nonce each time). // mixed in so the two sides' proofs are over DIFFERENT inputs: a peer can neither
fn auth_proof(secret: &str, nonce: &str) -> String { // 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::<Sha256>::new_from_slice(secret.as_bytes()).expect("HMAC accepts any key length"); let mut mac = Hmac::<Sha256>::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()); mac.update(nonce.as_bytes());
STANDARD.encode(mac.finalize().into_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 { tokio::spawn(async move {
let _permit = permit; // released when the session ends let _permit = permit; // released when the session ends
let served = match acceptor { let served = match acceptor {
Some(acc) => match acc.accept(stream).await { // Bound the TLS handshake too, so a peer that opens the socket but
Ok(tls) => session(tls, engine, secret, origin, outbound).await, // never finishes negotiating can't pin its session permit forever.
Err(e) => return tracing::debug!(%e, %addr, "gossip tls accept failed"), 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 { if let Err(e) = served {
tracing::debug!(%e, "gossip session ended"); 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 { let served = match &connector {
Some(conn) => match ServerName::try_from(peer.name.clone()) { Some(conn) => match ServerName::try_from(peer.name.clone()) {
Ok(name) => match conn.connect(name, stream).await { 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!("tls connect: {e}")),
}, },
Err(e) => Err(anyhow::anyhow!("bad peer TLS name {:?}: {e}", peer.name)), 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 { if let Err(e) = served {
tracing::debug!(%e, addr = %peer.addr, "gossip session ended"); tracing::debug!(%e, addr = %peer.addr, "gossip session ended");
@ -227,7 +235,7 @@ async fn read_handshake<R: AsyncBufRead + Unpin>(reader: &mut R) -> Option<Msg>
} }
// One peer connection: authenticate, then run anti-entropy until it drops. // One peer connection: authenticate, then run anti-entropy until it drops.
async fn session<S>(stream: S, engine: Shared, secret: String, origin: String, outbound: Outbound) -> anyhow::Result<()> async fn session<S>(stream: S, engine: Shared, secret: String, origin: String, outbound: Outbound, listener: bool) -> anyhow::Result<()>
where where
S: AsyncRead + AsyncWrite + Send + 'static, S: AsyncRead + AsyncWrite + Send + 'static,
{ {
@ -246,10 +254,12 @@ where
// Handshake: a mutual challenge-response over the shared secret, so the secret // 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 // 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 // holds the secret by returning HMAC(secret, its-role ‖ the peer's nonce); each
// the peer's proof against the nonce it chose. A wrong secret (or an // verifies the peer's proof against the nonce it chose and the peer's role. The
// eavesdropper) can't produce a valid proof, and a fresh nonce per link stops // per-role label (listener "L" vs dialer "D") is what stops a reflection or
// replay. // 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 my_nonce = make_nonce();
let _ = send(&tx, &Msg::Hello { origin, nonce: my_nonce.clone() }).await; let _ = send(&tx, &Msg::Hello { origin, nonce: my_nonce.clone() }).await;
let peer_nonce = match read_handshake(&mut reader).await { let peer_nonce = match read_handshake(&mut reader).await {
@ -259,9 +269,15 @@ where
anyhow::bail!("peer failed to complete the handshake"); 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 { 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(); writer.abort();
anyhow::bail!("bad gossip handshake"); anyhow::bail!("bad gossip handshake");
@ -362,6 +378,15 @@ mod tests {
use crate::engine::db::Db; use crate::engine::db::Db;
use echo_nickserv::NickServ; 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) { fn engine(origin: &str, tag: &str) -> (Shared, Outbound) {
let path = std::env::temp_dir().join(format!("echo-gossip-{tag}.jsonl")); let path = std::env::temp_dir().join(format!("echo-gossip-{tag}.jsonl"));
let _ = std::fs::remove_file(&path); let _ = std::fs::remove_file(&path);
@ -382,8 +407,8 @@ mod tests {
a.lock().await.test_register("alice"); a.lock().await.test_register("alice");
let (ca, cb) = tokio::io::duplex(64 * 1024); let (ca, cb) = tokio::io::duplex(64 * 1024);
let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx)); 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)); let sb = tokio::spawn(session(cb, b.clone(), "s3cret".into(), "B".into(), btx, false));
let mut converged = false; let mut converged = false;
for _ in 0..100 { for _ in 0..100 {
@ -406,8 +431,8 @@ mod tests {
let (b, btx) = engine("B", "push-b"); let (b, btx) = engine("B", "push-b");
let (ca, cb) = tokio::io::duplex(64 * 1024); let (ca, cb) = tokio::io::duplex(64 * 1024);
let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx)); 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)); 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 tokio::time::sleep(Duration::from_millis(200)).await; // let both subscribe
a.lock().await.test_register("late"); a.lock().await.test_register("late");
@ -435,8 +460,8 @@ mod tests {
b.lock().await.test_register_pw("alice", "from-b"); b.lock().await.test_register_pw("alice", "from-b");
let (ca, cb) = tokio::io::duplex(64 * 1024); let (ca, cb) = tokio::io::duplex(64 * 1024);
let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx)); 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)); let sb = tokio::spawn(session(cb, b.clone(), "s3cret".into(), "B".into(), btx, false));
let mut converged = false; let mut converged = false;
for _ in 0..100 { for _ in 0..100 {
@ -462,8 +487,8 @@ mod tests {
a.lock().await.test_register_channel("#secret", "alice"); a.lock().await.test_register_channel("#secret", "alice");
let (ca, cb) = tokio::io::duplex(64 * 1024); let (ca, cb) = tokio::io::duplex(64 * 1024);
let sa = tokio::spawn(session(ca, a.clone(), "s3cret".into(), "A".into(), atx)); 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)); 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 // Wait for the account to converge (proves the link works), then assert
// the channel never crossed it. // the channel never crossed it.
@ -492,8 +517,8 @@ mod tests {
a.lock().await.test_register("alice"); a.lock().await.test_register("alice");
let (ca, cb) = tokio::io::duplex(64 * 1024); let (ca, cb) = tokio::io::duplex(64 * 1024);
let sa = tokio::spawn(session(ca, a.clone(), "right".into(), "A".into(), atx)); 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)); let sb = tokio::spawn(session(cb, b.clone(), "wrong".into(), "B".into(), btx, false));
let _ = tokio::time::timeout(Duration::from_secs(1), async { let _ = tokio::time::timeout(Duration::from_secs(1), async {
let _ = sa.await; let _ = sa.await;
@ -538,8 +563,8 @@ mod tests {
let (sside, cside) = tokio::io::duplex(64 * 1024); let (sside, cside) = tokio::io::duplex(64 * 1024);
let name = ServerName::try_from("echo").unwrap(); let name = ServerName::try_from("echo").unwrap();
let (server, client) = tokio::join!(acceptor.accept(sside), connector.connect(name, cside)); 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 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)); let sb = tokio::spawn(session(client.expect("tls connect"), b.clone(), "s3cret".into(), "B".into(), btx, false));
let mut converged = false; let mut converged = false;
for _ in 0..100 { for _ in 0..100 {

17
src/link.rs
View file

@ -55,20 +55,19 @@ fn redact(line: &str) -> Cow<'_, str> {
let after = p + kw.len(); let after = p + kw.len();
if let Some(c) = line[after..].find(" :") { if let Some(c) = line[after..].find(" :") {
let tstart = after + c + 2; let tstart = after + c + 2;
let mut words = line[tstart..].split(' '); // Tokenise the trailer exactly like the dispatcher (`split_whitespace`),
let first = words.next().unwrap_or(""); // so a TAB or a leading/collapsed space can't sneak a credential past
let mut cmd = first.to_ascii_lowercase(); // redaction while still reaching the command handler.
let mut argstart = tstart + first.len(); let mut words = line[tstart..].split_whitespace();
let mut cmd = words.next().unwrap_or("").to_ascii_lowercase();
// "NS <cmd> ..." / "NICKSERV <cmd> ..." — unwrap one level. // "NS <cmd> ..." / "NICKSERV <cmd> ..." — unwrap one level.
if matches!(cmd.as_str(), "ns" | "nickserv") { if matches!(cmd.as_str(), "ns" | "nickserv") {
if let Some(w2) = words.next() { cmd = words.next().unwrap_or("").to_ascii_lowercase();
cmd = w2.to_ascii_lowercase();
argstart += 1 + w2.len();
}
} }
let is_set_password = cmd == "set" let is_set_password = cmd == "set"
&& words.next().map(|w| w.eq_ignore_ascii_case("password")).unwrap_or(false); && 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(); return format!("{} :[REDACTED]", line[..tstart].trim_end_matches(" :")).into();
} }
} }