diff --git a/daemon/src/collab_handler/mod.rs b/daemon/src/collab_handler/mod.rs index e259f758..1903600d 100644 --- a/daemon/src/collab_handler/mod.rs +++ b/daemon/src/collab_handler/mod.rs @@ -1822,7 +1822,9 @@ async fn handle_doc_request_inner( .await } - "sync/diff" => sync_methods::handle_sync_diff(doc_store, id, ¶ms).await, + "sync/diff" => { + sync_methods::handle_sync_diff(auth_principal, transport, doc_store, id, ¶ms).await + } "docs/list" => docs_methods::handle_docs_list(doc_store, id).await, @@ -1830,6 +1832,8 @@ async fn handle_doc_request_inner( "sync/resync" => { sync_methods::handle_sync_resync( + auth_principal, + transport, doc_store, broadcaster, session_id, diff --git a/daemon/src/collab_handler/sync_methods.rs b/daemon/src/collab_handler/sync_methods.rs index 8b1cb186..20ec82a7 100644 --- a/daemon/src/collab_handler/sync_methods.rs +++ b/daemon/src/collab_handler/sync_methods.rs @@ -305,11 +305,23 @@ pub(super) async fn handle_sync_full_state( } pub(super) async fn handle_sync_diff( + auth_principal: Option<&str>, + transport: Transport, doc_store: &DocStore, id: serde_json::Value, params: &serde_json::Value, ) -> JsonRpcResponse { let doc_name = params["doc"].as_str().unwrap_or("default").to_string(); + // @ai-caution: [security] `sync/diff` returns the same KB CRDT bytes + // `sync/state_vector` and `sync/full_state` are gated for. It was ungated, + // so a peer could read any other peer's KB node state by asking for a diff + // instead of a read — while `kb/node_fetch` on the same node correctly + // refused. `deny_kb_doc_read` only denies `kb:`/`kbc:` docs, so plain file + // collaboration is unaffected. + if let Err(msg) = deny_kb_doc_read(doc_store, &doc_name, auth_principal, transport).await { + warn!(doc = %doc_name, reason = %msg, "sync/diff denied"); + return JsonRpcResponse::error(id, McpError::internal_error(msg)); + } let sv_b64 = match params["sv"].as_str() { Some(s) => s, None => { @@ -346,7 +358,14 @@ pub(super) async fn handle_sync_diff( } } +// `deny_kb_doc_read` needs the principal and the transport, which pushes this +// past the 7-argument lint. Splitting the signature into a params struct here +// would obscure the call site for one lint, and the alternative — leaving the +// gate off — is what this function is being fixed for. +#[allow(clippy::too_many_arguments)] pub(super) async fn handle_sync_resync( + auth_principal: Option<&str>, + transport: Transport, doc_store: &DocStore, broadcaster: &SharedBroadcaster, session_id: u64, @@ -358,15 +377,34 @@ pub(super) async fn handle_sync_resync( // BUG C fix: atomic state + sv under single lock (INV-2). let raw_name = params["doc"].as_str().unwrap_or("default").to_string(); info!(session = session_id, doc = %raw_name, "sync/resync: processing"); + // @ai-caution: [security] `sync/resync` returns FULL document state and + // additionally SUBSCRIBES the session to that doc's live broadcast. It was + // ungated, which made it both a read of any peer's KB node and a way to + // self-subscribe to every subsequent update to it. Gate before either. + // Checked on the raw name as well as the resolved one so suffix resolution + // cannot be used to reach a `kb:` doc by another spelling. + if let Err(msg) = deny_kb_doc_read(doc_store, &raw_name, auth_principal, transport).await { + warn!(session = session_id, doc = %raw_name, reason = %msg, "sync/resync denied"); + return JsonRpcResponse::error(id, McpError::internal_error(msg)); + } // Resolve bare filenames via suffix matching (e.g. "test.txt" finds "file:no-project/test.txt"). let doc_name = if doc_store.has_doc(&raw_name).await { - raw_name + raw_name.clone() } else if let Some(found) = doc_store.find_doc_by_suffix(&raw_name).await { info!(requested = %raw_name, resolved = %found, "resolved doc by suffix match"); found } else { - raw_name // fall through — will create new empty doc + raw_name.clone() // fall through — will create new empty doc }; + // Suffix resolution can map a bare name onto a `kb:`/`kbc:` doc, so the + // RESOLVED name has to be checked too — otherwise the gate above is + // bypassable by asking for "notes.org" instead of "kb:notes.org". + if doc_name != raw_name { + if let Err(msg) = deny_kb_doc_read(doc_store, &doc_name, auth_principal, transport).await { + warn!(session = session_id, doc = %doc_name, reason = %msg, "sync/resync denied (resolved)"); + return JsonRpcResponse::error(id, McpError::internal_error(msg)); + } + } // Track this doc for disconnect cleanup and doc-scoped broadcast filtering. if session_docs.insert(doc_name.clone()) { let _ = doc_store.track_client_connect(&doc_name).await; diff --git a/daemon/src/collab_handler/tests/collab_handler_cross_kb_node_isolation_tests.rs b/daemon/src/collab_handler/tests/collab_handler_cross_kb_node_isolation_tests.rs index 695dca50..7cb3eca5 100644 --- a/daemon/src/collab_handler/tests/collab_handler_cross_kb_node_isolation_tests.rs +++ b/daemon/src/collab_handler/tests/collab_handler_cross_kb_node_isolation_tests.rs @@ -148,3 +148,85 @@ async fn kb_node_fetch_cannot_read_a_node_belonging_to_another_kb() { ); assert!(!victim_docs.contains("kb:concept:a-own")); } + +/// The same attack via the raw sync surface, which was ungated entirely. +/// +/// `deny_kb_doc_read` existed and was correct — it was called from exactly TWO +/// of the paths that needed it (`sync/state_vector`, `sync/full_state`). +/// `sync/resync` returns the same bytes under a different method name, and +/// `sync/diff` returns them as a delta. Its own doc comment says KB content +/// "must be fetched via the access-gated `kb/node_fetch`"; these two were what +/// made that false. +/// +/// `sync/resync` is the worse of the pair, for the reason this file's header +/// gives about `kb/node_fetch`: it also `subscribe_doc`s the session, so an +/// ungated call grants a STANDING feed of every future edit, not just one read. +#[tokio::test] +async fn raw_sync_methods_cannot_read_a_node_belonging_to_another_kb() { + let store = test_doc_store(); + let bc = test_broadcaster(); + + let mut mallory_docs = HashSet::new(); + kb_share_as( + &store, + &bc, + Some("mallory"), + Some(&fp("mallory")), + "kb-a", + "mallory", + &mut mallory_docs, + ) + .await; + + let mut victim_docs = HashSet::new(); + kb_share_as( + &store, + &bc, + Some("victim"), + Some(&fp("victim")), + "kb-b", + "victim", + &mut victim_docs, + ) + .await; + let added = dispatch_as( + &store, + &bc, + Some("victim"), + Some(&fp("victim")), + serde_json::json!({ + "jsonrpc":"2.0","id":1,"method":"kb/collection_node_add", + "params":{"kb_id":"kb-b","node_id":"concept:b-secret","title":"B"}}), + &mut victim_docs, + ) + .await; + assert!(added.error.is_none(), "seed failed: {:?}", added.error); + + for method in ["sync/resync", "sync/diff"] { + let before = mallory_docs.len(); + let attack = dispatch_as( + &store, + &bc, + Some("mallory"), + Some(&fp("mallory")), + serde_json::json!({ + "jsonrpc":"2.0","id":9,"method": method, + "params":{"doc":"kb:concept:b-secret","sv":""}}), + &mut mallory_docs, + ) + .await; + + assert!( + attack.error.is_some(), + "{method} returned kb-b's node to an outsider: {:?}", + attack.result + ); + // The load-bearing half: a refusal that still subscribed would pass the + // assertion above and keep leaking every subsequent edit. + assert_eq!( + mallory_docs.len(), + before, + "{method} subscribed the session to another KB's node despite refusing the read" + ); + } +} diff --git a/daemon/src/config.rs b/daemon/src/config.rs index 626e4f2b..9f6d6cc7 100644 --- a/daemon/src/config.rs +++ b/daemon/src/config.rs @@ -815,6 +815,8 @@ impl DaemonConfig { let mut issues = Vec::new(); let c = &self.collab; + issues.extend(crate::config_guards::unauthenticated_bind_issues(c)); + if c.storage.compact_threshold == 0 { issues.push("collab.storage.compact_threshold must be > 0".to_string()); } diff --git a/daemon/src/config_bind_tests.rs b/daemon/src/config_bind_tests.rs new file mode 100644 index 00000000..e51610f6 --- /dev/null +++ b/daemon/src/config_bind_tests.rs @@ -0,0 +1,72 @@ +//! ADVERSARIAL: an unauthenticated collab port that is reachable off-host. +//! +//! Split out of `config.rs` rather than blessing its growth — the structural +//! ratchet is doing its job, and a security test module is exactly the kind of +//! thing that should not push a config module past its ceiling. + +use crate::config::DaemonConfig; + +fn cfg(bind: &str, mode: &str) -> DaemonConfig { + let mut c = DaemonConfig::default(); + c.collab.enabled = true; + c.collab.bind = bind.parse().unwrap(); + c.collab.auth.mode = mode.to_string(); + c +} + +/// An unauthenticated session reaches `kb_access_with_coll` with +/// `principal == None`, which returns `Allow`. So a non-loopback bind under +/// `mode = "none"` grants Manage on every KB to every host that can reach +/// the port — and `mode` DEFAULTS to "none". +/// +/// The oracle is that the config is rejected, and that the message names +/// both halves: an operator who sees only "bad config" will re-read the +/// wrong line. +#[test] +fn an_unauthenticated_off_host_bind_is_refused() { + for (bind, mode) in [ + ("0.0.0.0:9473", "none"), + ("0.0.0.0:9473", "psk"), + ("[::]:9473", "none"), + ("10.0.0.5:9473", "none"), + ("10.0.0.5:9473", "psk"), + ] { + let issues = cfg(bind, mode).check_collab(); + assert!( + issues.iter().any(|i| i.contains("reachable off-host")), + "bind={bind} mode={mode} must be refused, got: {issues:?}" + ); + } +} + +/// The three configurations that must NOT be refused. Without these the +/// check above would be satisfied by a function that rejects everything, +/// and loopback development would be broken. +#[test] +fn loopback_and_key_mode_are_accepted() { + for (bind, mode) in [ + ("127.0.0.1:9473", "none"), + ("127.0.0.1:9473", "psk"), + ("[::1]:9473", "none"), + ("0.0.0.0:9473", "key"), + ("10.0.0.5:9473", "key"), + ] { + let issues = cfg(bind, mode).check_collab(); + assert!( + !issues.iter().any(|i| i.contains("reachable off-host")), + "bind={bind} mode={mode} must be accepted, got: {issues:?}" + ); + } +} + +/// Disabling collab must not produce a bind complaint about a listener that +/// never starts. +#[test] +fn a_disabled_collab_listener_is_not_flagged() { + let mut c = cfg("0.0.0.0:9473", "none"); + c.collab.enabled = false; + assert!(!c + .check_collab() + .iter() + .any(|i| i.contains("reachable off-host"))); +} diff --git a/daemon/src/config_guards.rs b/daemon/src/config_guards.rs new file mode 100644 index 00000000..0a6c3e7c --- /dev/null +++ b/daemon/src/config_guards.rs @@ -0,0 +1,41 @@ +//! Configuration guards that refuse a dangerous deployment shape. +//! +//! Separate from `config.rs` because that file is already a tracked +//! ceiling exception, and because these are *security* refusals rather than +//! field-validity checks — they answer "is this safe to expose", not "is this +//! well-formed". + +use crate::config::CollabConfig; +use std::net::SocketAddr; + +/// Whether a bind address is loopback-only, i.e. unreachable from another host. +/// `0.0.0.0`/`::` are explicitly NOT loopback — they are the wildcard binds, +/// which is exactly the case this exists to catch. +fn bind_is_loopback(addr: &SocketAddr) -> bool { + addr.ip().is_loopback() +} + +/// Refuse an unauthenticated collab port that is reachable off-host. +/// +/// @ai-caution: [security] `AuthConfig::default().mode` is `"none"`, and an +/// unauthenticated session reaches `kb_access_with_coll` with +/// `principal == None`, which returns `AccessDecision::Allow`. So +/// `--bind 0.0.0.0` on a stock config granted Manage on every KB to every host +/// that could reach the port, while `doctor` printed "collab config: OK". +/// `psk` is plaintext on the wire and no better off-host. +/// +/// This is an ERROR, not a warning: a warning gets read past, and the shipped +/// `assets/daemon-config.toml` has no `[collab.auth]` block at all while +/// DAEMON_ADMIN tells operators to start from it. +pub fn unauthenticated_bind_issues(c: &CollabConfig) -> Vec { + if c.enabled && !bind_is_loopback(&c.bind) && c.auth.mode != "key" { + return vec![format!( + "collab.bind is {} (reachable off-host) but collab.auth.mode is \ + '{}' — that accepts any client that can reach the port. Set \ + [collab.auth] mode = \"key\" (Ed25519 mTLS), or bind to loopback \ + and put a reverse proxy or VPN in front.", + c.bind, c.auth.mode + )]; + } + Vec::new() +} diff --git a/daemon/src/main.rs b/daemon/src/main.rs index 78da9942..f7b34f18 100644 --- a/daemon/src/main.rs +++ b/daemon/src/main.rs @@ -15,6 +15,9 @@ mod cli; mod config; +#[cfg(test)] +mod config_bind_tests; +mod config_guards; mod conn_limit; mod dialer; pub mod enrichment;