diff --git a/docs/berdctl-architecture.md b/docs/berdctl-architecture.md index 8e7480172..08473fc93 100644 --- a/docs/berdctl-architecture.md +++ b/docs/berdctl-architecture.md @@ -10,16 +10,18 @@ berdctl project create --name demo The implementation has three layers: 1. CLI: `src-tauri/crates/berdctl/` - Parses flags with clap, prints help, reads the app discovery file, and sends - JSON calls. CLI validation is convenience only. + Parses flags with clap, prints help, reads the private app discovery file, + authenticates each loopback request with its per-broker capability, and + sends JSON calls. CLI validation is convenience only. 2. Broker: `src-tauri/plugins/berdctl/` - Runs a localhost server inside the app, rejects browser-origin requests, - enforces in-flight and timeout limits, and forwards calls to the renderer - without command-specific logic. + Runs a localhost server inside the app, requires the current discovery-file + capability, rejects browser-origin requests, enforces in-flight and timeout + limits, and forwards calls to the renderer without command-specific logic. 3. Renderer registry: `src/features/berdctl/commands/` Strict-parses args with zod, runs guards, executes through app state, and - returns JSON results. This is the trust boundary because any same-user - process can bypass the CLI and POST to the broker directly. + returns JSON results. This remains the command-policy trust boundary; the + broker capability limits access to processes that can read the owning + user's private discovery file. ## Layer rules @@ -87,9 +89,13 @@ belongs in error messages, not generic help text. ## Safety model -v1 has no auth tokens and no confirmation dialogs. That remains acceptable only -while mutations are visible in the UI and either reversible or direct -user-requested product actions, such as creating a session or sending a prompt. +v1 requires a fresh 256-bit bearer capability for every broker start. The +plugin writes it beside the port and generation in the discovery file, with +owner-only directory/file permissions on Unix, and the CLI presents it on +both `/v1/ping` and `/v1/call`. Missing, malformed, wrong, stale, symlinked, +or non-private capability records fail closed. This authenticates possession +of the app-issued session endpoint; it does not replace renderer command +policy or add interactive confirmation dialogs. Required command properties: @@ -108,8 +114,10 @@ piecemeal auth in a command PR. ## Versioning -The broker writes a discovery file with `protocolVersion`, generation, and port. -The CLI verifies it via `/v1/ping` before calls. +The broker writes a private discovery file with `protocolVersion`, generation, +port, and a per-start capability. The CLI authenticates and verifies it via +`/v1/ping` before calls. Requiring that capability is a breaking wire reshape, +so the authenticated surface starts at protocol version 5. Breaking wire reshapes must bump all three constants: @@ -138,4 +146,4 @@ bump. Adding a command or optional field is not a wire reshape. | safety metadata complete | berdctl command tests | Review-only rules: single renderer dispatch point, detecting breaking wire -reshapes, and product judgment for no-auth command eligibility. +reshapes, and product judgment for command eligibility under capability authentication. diff --git a/scripts/windows/CI-Windows.ps1 b/scripts/windows/CI-Windows.ps1 index e83627ebd..037c14ffd 100644 --- a/scripts/windows/CI-Windows.ps1 +++ b/scripts/windows/CI-Windows.ps1 @@ -3,9 +3,9 @@ # Runs the Rust checks that only a real Windows host can exercise: the # `managed_node` / `managed_acp_tools` module tests (including the native gate # that downloads and executes the real pinned Node ZIP), security-sensitive -# Windows process-launch tests, plus Windows clippy in the default and -# app-feature configurations. Invoked through `just ci-windows` for local and -# release validation. +# Windows process-launch and berdctl discovery tests, plus Windows clippy in the +# default and app-feature configurations. Invoked through `just ci-windows` for +# local and release validation. $ErrorActionPreference = "Stop" trap { Write-Host $_.Exception.Message -ForegroundColor Red @@ -58,6 +58,11 @@ Invoke-CargoCheck -ArgumentList @( "test", "--lib", "commands::system::tests::windows_chrome_launch_" ) -Label "cargo test Windows Chrome launch" +# Exercise the DACL and atomic-publication paths on a native Windows filesystem. +Invoke-CargoCheck -ArgumentList @( + "test", "-p", "tauri-plugin-berdctl", "--features", "server", "discovery::tests::" +) -Label "cargo test berdctl discovery" + # Clippy compiles both configurations, so separate `cargo check` calls only # repeat the same compile coverage. Invoke-CargoCheck -ArgumentList @( diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index 539f0d573..ce88deb0e 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -617,9 +617,11 @@ version = "0.6.2" dependencies = [ "clap", "indexmap 2.13.1", + "libc", "serde", "serde_json", "ureq 3.4.0", + "windows-sys 0.59.0", ] [[package]] @@ -6643,14 +6645,19 @@ name = "tauri-plugin-berdctl" version = "0.6.2" dependencies = [ "axum", + "getrandom 0.4.3", + "hex", + "libc", "log", "reqwest 0.13.4", "serde", "serde_json", + "subtle", "tauri", "tauri-plugin", "tokio", "uuid", + "windows-sys 0.59.0", ] [[package]] diff --git a/src-tauri/README.md b/src-tauri/README.md index c95a93255..627653cbd 100644 --- a/src-tauri/README.md +++ b/src-tauri/README.md @@ -15,8 +15,9 @@ The Tauri 2 shell: the app crate (`src/`), the berdctl workspace crates The CLI embeds the contract artifacts (`crates/berdctl/api-surface.json` + `cli-surface.json`) and builds its clap tree at startup. It locates the -broker through the `BERDCTL_LOCK` discovery file, verifies -`protocolVersion`/generation via `GET /v1/ping`, and sends +broker through the `BERDCTL_LOCK` discovery file, reads its per-start +capability, verifies `protocolVersion`/generation through an authenticated +`GET /v1/ping`, and sends authenticated `POST /v1/call {"command", "args"}`. The broker forwards to the renderer over Tauri IPC (`berdctl:request` event out, `submit_result` back). Command dispatch, zod validation, guards, and execution live in the @@ -41,7 +42,8 @@ capability grants a permission allowing that command. window. This ACL gates webview → Rust IPC only; the localhost HTTP side is governed -separately (discovery file, header rejection, global caps). +separately by the owner-private discovery capability, browser/DNS-rebinding +header rejection, and global caps. Stock Tauri 2 plugin layout. Docs: [Plugin Development](https://v2.tauri.app/develop/plugins/), diff --git a/src-tauri/crates/berdctl/Cargo.toml b/src-tauri/crates/berdctl/Cargo.toml index 5fa6346ad..ca1da813e 100644 --- a/src-tauri/crates/berdctl/Cargo.toml +++ b/src-tauri/crates/berdctl/Cargo.toml @@ -18,6 +18,18 @@ serde = { version = "1", features = ["derive"] } serde_json = "1" ureq = { version = "3", features = ["json"] } +[target.'cfg(unix)'.dependencies] +libc = "0.2" + +[target.'cfg(windows)'.dependencies] +windows-sys = { version = "0.59", features = [ + "Win32_Foundation", + "Win32_Security", + "Win32_Security_Authorization", + "Win32_Storage_FileSystem", + "Win32_System_Threading", +] } + [features] default = [] block-feedback = [] diff --git a/src-tauri/crates/berdctl/api-surface-feedback.json b/src-tauri/crates/berdctl/api-surface-feedback.json index 7ed5474dd..4f6956ed1 100644 --- a/src-tauri/crates/berdctl/api-surface-feedback.json +++ b/src-tauri/crates/berdctl/api-surface-feedback.json @@ -1,6 +1,6 @@ { "$comment": "GENERATED FILE — do not hand-edit; run `pnpm generate:berdctl-contract`. Client-neutral wire surface of the Berd desktop app's control API: POST /v1/call {\"command\": \"\", \"args\": {\"action\": \"\", ...fields}} against the loopback endpoint in the berdctl discovery file. protocolVersion mirrors PROTOCOL_VERSION in both discovery.rs copies (berdctl and plugin crate tests pin them equal). Per action: description, fields (flat wire model: name, required, kind, values, description, bounds), and schema (JSON Schema 2020-12 of the args object, minus the action discriminator). Derived from the authoritative zod schemas in the colocated command modules (src/features/berdctl/commands/impl/*.ts); the renderer registry's strict zod parse — not this file — is the trust boundary. vitest asserts freshness (src/features/berdctl/__tests__/apiSurface.test.ts).", - "protocolVersion": 4, + "protocolVersion": 5, "groups": { "sessions": { "description": "Manage the user's chat sessions: create (fire-and-forget, on any installed agent harness), send, open, list, get, rename, move, move to group, clear project, fork, archive.", diff --git a/src-tauri/crates/berdctl/api-surface.json b/src-tauri/crates/berdctl/api-surface.json index b7daa844d..3d1cb4118 100644 --- a/src-tauri/crates/berdctl/api-surface.json +++ b/src-tauri/crates/berdctl/api-surface.json @@ -1,6 +1,6 @@ { "$comment": "GENERATED FILE — do not hand-edit; run `pnpm generate:berdctl-contract`. Client-neutral wire surface of the Berd desktop app's control API: POST /v1/call {\"command\": \"\", \"args\": {\"action\": \"\", ...fields}} against the loopback endpoint in the berdctl discovery file. protocolVersion mirrors PROTOCOL_VERSION in both discovery.rs copies (berdctl and plugin crate tests pin them equal). Per action: description, fields (flat wire model: name, required, kind, values, description, bounds), and schema (JSON Schema 2020-12 of the args object, minus the action discriminator). Derived from the authoritative zod schemas in the colocated command modules (src/features/berdctl/commands/impl/*.ts); the renderer registry's strict zod parse — not this file — is the trust boundary. vitest asserts freshness (src/features/berdctl/__tests__/apiSurface.test.ts).", - "protocolVersion": 4, + "protocolVersion": 5, "groups": { "sessions": { "description": "Manage the user's chat sessions: create (fire-and-forget, on any installed agent harness), send, open, list, get, rename, move, move to group, clear project, fork, archive.", diff --git a/src-tauri/crates/berdctl/src/client.rs b/src-tauri/crates/berdctl/src/client.rs index 6383d2986..8adfa5758 100644 --- a/src-tauri/crates/berdctl/src/client.rs +++ b/src-tauri/crates/berdctl/src/client.rs @@ -62,11 +62,26 @@ pub struct PingResponse { pub struct Endpoint { pub port: u16, + capability: String, +} + +impl std::fmt::Debug for Endpoint { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("Endpoint") + .field("port", &self.port) + .field("capability", &"[redacted]") + .finish() + } } fn agent(timeout: Duration) -> ureq::Agent { ureq::Agent::config_builder() .timeout_global(Some(timeout)) + // The broker is a literal loopback service. Never hand its bearer + // capability to a user-configured proxy or redirect target. + .proxy(None) + .max_redirects(0) // Non-2xx responses carry the broker's structured error body; read it // instead of treating the status as a transport error. .http_status_as_error(false) @@ -74,53 +89,112 @@ fn agent(timeout: Duration) -> ureq::Agent { .new_agent() } +#[derive(Debug)] +struct PingFailure { + detail: String, + status: Option, +} + +impl PingFailure { + fn transport(detail: String) -> Self { + Self { + detail, + status: None, + } + } + + fn status(detail: String, status: u16) -> Self { + Self { + detail, + status: Some(status), + } + } +} + /// Probe the listener before sending any payload (command args can contain /// prompt text, which must not be sprayed at an unknown local service). /// Returns the failure detail only; callers decide the exit class. -pub fn ping(port: u16) -> Result { +fn ping(port: u16, capability: &str) -> Result { let url = format!("http://127.0.0.1:{port}/v1/ping"); let mut response = agent(PING_TIMEOUT) .get(&url) + .header("Authorization", format!("Bearer {capability}")) .call() - .map_err(|err| format!("nothing answered on 127.0.0.1:{port} ({err})"))?; + .map_err(|err| { + PingFailure::transport(format!("nothing answered on 127.0.0.1:{port} ({err})")) + })?; let status = response.status().as_u16(); if status != 200 { - return Err(format!( - "the listener on 127.0.0.1:{port} does not look like the Berd app \ - control endpoint (ping returned status {status})" + return Err(PingFailure::status( + format!( + "the listener on 127.0.0.1:{port} does not look like the Berd app \ + control endpoint (ping returned status {status})" + ), + status, )); } response .body_mut() .read_json::() .map_err(|err| { - format!( - "the listener on 127.0.0.1:{port} does not look like the Berd app \ - control endpoint (unrecognized ping response: {err})" + PingFailure::status( + format!( + "the listener on 127.0.0.1:{port} does not look like the Berd app \ + control endpoint (unrecognized ping response: {err})" + ), + status, ) }) } /// Read the discovery file and verify the broker behind it echoes the file's -/// generation and this binary's protocol version. A generation mismatch means -/// the file was read across a broker restart: re-read once and retry once. +/// generation and this binary's protocol version. A generation mismatch or +/// authentication failure can mean the file was read across a broker restart: +/// re-read once and retry once. pub fn handshake(lock_path: &Path) -> Result { let mut file = discovery::load_with_retry(lock_path)?; for attempt in 0..2 { if file.protocol_version != PROTOCOL_VERSION { return Err(Failure::env(APP_UPDATED)); } - let ping = ping(file.port).map_err(|detail| { - Failure::env(format!( - "the Berd desktop app is not reachable: {detail}. The app may have \ - quit; {CONTROL_REMEDIATION}" - )) - })?; + let ping = match ping(file.port, &file.capability) { + Ok(ping) => ping, + Err(failure) if failure.status == Some(403) => { + if attempt == 0 { + // Authentication failure can be the observable edge of a + // broker restart: the process has rotated the capability but + // this command opened the previous discovery inode. Re-read + // once, just as for the existing generation-mismatch path. + file = discovery::load(lock_path).map_err(|err| { + Failure::env(format!( + "the Berd desktop app restarted its control endpoint and the new \ + one could not be read ({err}); {CONTROL_REMEDIATION}" + )) + })?; + continue; + } + return Err(Failure::env(format!( + "the Berd desktop app is not reachable: {}. The app may have \ + quit; {CONTROL_REMEDIATION}", + failure.detail + ))); + } + Err(failure) => { + return Err(Failure::env(format!( + "the Berd desktop app is not reachable: {}. The app may have \ + quit; {CONTROL_REMEDIATION}", + failure.detail + ))); + } + }; if ping.protocol_version != PROTOCOL_VERSION { return Err(Failure::env(APP_UPDATED)); } if ping.generation == file.generation { - return Ok(Endpoint { port: file.port }); + return Ok(Endpoint { + port: file.port, + capability: file.capability, + }); } if attempt == 0 { file = discovery::load(lock_path).map_err(|err| { @@ -140,30 +214,90 @@ pub fn handshake(lock_path: &Path) -> Result { } /// POST one command. `args` already contains the `action` discriminator. +/// +/// A legitimate broker restart can happen after [`handshake`] and before this +/// request. Its old capability is rejected before dispatch, so re-read and +/// re-handshake exactly once before retrying. `Expect: 100-continue` prevents +/// the rejected listener from receiving the command body: the body is sent +/// only after that listener has accepted the request headers. pub fn call( + lock_path: &Path, endpoint: &Endpoint, command: &str, args: Map, timeout_ms: Option, ) -> Result { - let url = format!("http://127.0.0.1:{}/v1/call", endpoint.port); let mut payload = Map::new(); payload.insert("command".into(), Value::String(command.into())); payload.insert("args".into(), Value::Object(args)); if let Some(ms) = timeout_ms { payload.insert("timeout_ms".into(), Value::from(ms)); } + let payload = Value::Object(payload); + + match call_once(endpoint, &payload) { + Ok(result) => Ok(result), + Err(CallFailure::BrokerAuthForbidden) => { + // The broker emits this 403 before reading the body or dispatching + // it. Do not retry any other response: HTTP cannot establish + // whether it may already have been dispatched. + let endpoint = handshake(lock_path)?; + call_once(&endpoint, &payload).map_err(CallFailure::into_failure) + } + Err(failure) => Err(failure.into_failure()), + } +} + +/// A 403 with this exact broker error is known not to have been dispatched: +/// the broker checks its bearer capability before reading `/v1/call`'s body. +enum CallFailure { + BrokerAuthForbidden, + Other(Failure), +} + +impl CallFailure { + fn into_failure(self) -> Failure { + match self { + Self::BrokerAuthForbidden => Failure::transport( + "the app control endpoint rejected a retried command before dispatch", + ), + Self::Other(failure) => failure, + } + } +} + +fn call_once(endpoint: &Endpoint, payload: &Value) -> Result { + let url = format!("http://127.0.0.1:{}/v1/call", endpoint.port); let mut response = agent(CALL_TIMEOUT) .post(&url) - .send_json(Value::Object(payload)) - .map_err(|err| transport_error_failure(endpoint.port, &err))?; + .header("Authorization", format!("Bearer {}", endpoint.capability)) + // A final response to this header prevents ureq from sending the body. + // This limits a port-reuse listener to request metadata; HTTP cannot + // remove the unavoidable TOCTOU after a peer sends 100 Continue. + .header("Expect", "100-continue") + .send_json(payload) + .map_err(|err| CallFailure::Other(transport_error_failure(endpoint.port, &err)))?; let status = response.status().as_u16(); let body = response.body_mut().read_to_string().map_err(|err| { - Failure::transport(format!( + CallFailure::Other(Failure::transport(format!( "the app control endpoint's response could not be read ({err})" - )) + ))) })?; - classify_response(status, &body) + if is_broker_auth_forbidden(status, &body) { + return Err(CallFailure::BrokerAuthForbidden); + } + classify_response(status, &body).map_err(CallFailure::Other) +} + +fn is_broker_auth_forbidden(status: u16, body: &str) -> bool { + status == 403 + && serde_json::from_str::(body) + .ok() + .as_ref() + .and_then(error_parts) + .is_some_and(|(code, message)| { + code == "forbidden" && message == "valid bearer capability required" + }) } /// The app quitting between ping and call surfaces as a refused connection — @@ -247,6 +381,515 @@ fn error_parts(value: &Value) -> Option<(String, String)> { #[cfg(test)] mod tests { use super::*; + #[cfg(unix)] + use std::io::{BufRead, BufReader, Read, Write}; + #[cfg(unix)] + use std::net::{TcpListener, TcpStream}; + #[cfg(unix)] + use std::path::PathBuf; + #[cfg(unix)] + use std::sync::{mpsc, Arc}; + #[cfg(unix)] + use std::thread; + + #[cfg(unix)] + const CURRENT_CAPABILITY: &str = + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + #[cfg(unix)] + const STALE_CAPABILITY: &str = + "abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"; + + // `discovery::load` deliberately verifies Unix ownership and mode bits. + // The production Windows reader instead validates a handle's owner/DACL; + // do not run this POSIX fixture there with an inherited temp-directory ACL. + #[cfg(unix)] + struct TempDiscoveryFile(PathBuf); + + #[cfg(unix)] + impl TempDiscoveryFile { + fn new(label: &str, port: u16, capability: &str) -> Self { + let base = std::env::temp_dir().join(format!( + "berdctl-client-auth-{}-{label}-{port}", + std::process::id() + )); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).expect("create discovery directory"); + let path = base.join("control.json"); + Self::write(&path, port, capability); + Self(path) + } + + fn replace(&self, port: u16, capability: &str) { + Self::write(&self.0, port, capability); + } + + fn write(path: &Path, port: u16, capability: &str) { + use std::os::unix::fs::PermissionsExt; + + std::fs::write( + path, + format!( + r#"{{"port":{port},"pid":4242,"generation":7,"protocolVersion":{PROTOCOL_VERSION},"capability":"{capability}"}}"# + ), + ) + .expect("write discovery file"); + std::fs::set_permissions( + path.parent().expect("test discovery has a parent"), + std::fs::Permissions::from_mode(0o700), + ) + .expect("make discovery directory private"); + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) + .expect("make discovery file private"); + } + } + + #[cfg(unix)] + impl Drop for TempDiscoveryFile { + fn drop(&mut self) { + if let Some(parent) = self.0.parent() { + std::fs::remove_dir_all(parent).ok(); + } + } + } + + #[cfg(unix)] + struct RecordedRequest { + request_line: String, + authorization: Option, + expects_continue: bool, + body: String, + } + + #[cfg(unix)] + fn read_request(stream: &mut TcpStream) -> RecordedRequest { + let mut reader = BufReader::new(stream.try_clone().expect("clone request stream")); + let mut request_line = String::new(); + reader + .read_line(&mut request_line) + .expect("read request line"); + let mut authorization = None; + let mut content_length = 0; + let mut expects_continue = false; + loop { + let mut line = String::new(); + reader.read_line(&mut line).expect("read request header"); + if line == "\r\n" { + break; + } + let Some((name, value)) = line.trim_end().split_once(':') else { + continue; + }; + if name.eq_ignore_ascii_case("authorization") { + authorization = Some(value.trim().to_string()); + } + if name.eq_ignore_ascii_case("content-length") { + content_length = value.trim().parse().expect("valid content length"); + } + if name.eq_ignore_ascii_case("expect") + && value.trim().eq_ignore_ascii_case("100-continue") + { + expects_continue = true; + } + } + if expects_continue { + stream + .write_all(b"HTTP/1.1 100 Continue\r\n\r\n") + .expect("accept request body"); + } + let mut body = vec![0; content_length]; + reader.read_exact(&mut body).expect("read request body"); + RecordedRequest { + request_line: request_line.trim_end().to_string(), + authorization, + expects_continue, + body: String::from_utf8(body).expect("request body is UTF-8"), + } + } + + #[cfg(unix)] + fn write_response(stream: &mut TcpStream, status: &str, body: &str) { + write!( + stream, + "HTTP/1.1 {status}\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ) + .expect("write test response"); + } + + #[cfg(unix)] + fn spawn_broker_sequence( + expected_requests: Vec<(&'static str, BrokerResponse)>, + ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { + spawn_broker_responses(expected_requests) + } + + #[cfg(unix)] + #[derive(Clone, Copy)] + enum BrokerResponse { + Ping { + generation: u64, + }, + Call, + CallError { + status: &'static str, + body: &'static str, + }, + } + + #[cfg(unix)] + fn spawn_broker_responses( + expected_requests: Vec<(&'static str, BrokerResponse)>, + ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { + spawn_broker_responses_with_sync(expected_requests, None) + } + + #[cfg(unix)] + fn spawn_broker_responses_with_sync( + expected_requests: Vec<(&'static str, BrokerResponse)>, + first_response_sync: Option<(Arc, Arc)>, + ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { + let listener = TcpListener::bind(("127.0.0.1", 0)).expect("bind test broker"); + let port = listener.local_addr().expect("test broker address").port(); + let (requests_tx, requests_rx) = mpsc::channel(); + let handle = thread::spawn(move || { + for (request_number, (expected_capability, response)) in + expected_requests.into_iter().enumerate() + { + let (mut stream, _) = listener.accept().expect("accept client request"); + let request = read_request(&mut stream); + let authorized = request.authorization.as_deref() + == Some(&format!("Bearer {expected_capability}")); + requests_tx.send(request).expect("record client request"); + if request_number == 0 { + if let Some((request_seen, response_ready)) = &first_response_sync { + request_seen.wait(); + response_ready.wait(); + } + } + match (authorized, response) { + (true, BrokerResponse::Ping { generation }) => write_response( + &mut stream, + "200 OK", + &format!( + r#"{{"generation":{generation},"protocolVersion":{PROTOCOL_VERSION}}}"# + ), + ), + (true, BrokerResponse::Call) => { + write_response(&mut stream, "200 OK", r#"{"ok":true,"result":"ok"}"#) + } + (true, BrokerResponse::CallError { status, body }) => { + write_response(&mut stream, status, body) + } + (false, _) => write_response( + &mut stream, + "403 Forbidden", + r#"{"ok":false,"error":{"code":"forbidden","message":"valid bearer capability required"}}"#, + ), + } + } + }); + (port, requests_rx, handle) + } + + #[cfg(unix)] + fn spawn_broker( + expected_capability: &'static str, + request_count: usize, + ) -> (u16, mpsc::Receiver, thread::JoinHandle<()>) { + spawn_broker_responses( + (0..request_count) + .map(|request_number| { + let response = if request_number == 0 { + BrokerResponse::Ping { generation: 7 } + } else { + BrokerResponse::Call + }; + (expected_capability, response) + }) + .collect(), + ) + } + + #[cfg(unix)] + #[test] + fn handshake_and_call_send_current_capability() { + let (port, requests, broker) = spawn_broker(CURRENT_CAPABILITY, 2); + let lock_file = TempDiscoveryFile::new("current", port, CURRENT_CAPABILITY); + + let endpoint = handshake(&lock_file.0).expect("current capability handshakes"); + assert_eq!( + format!("{endpoint:?}"), + format!("Endpoint {{ port: {port}, capability: \"[redacted]\" }}"), + "debug output must not disclose the bearer capability" + ); + let result = call( + &lock_file.0, + &endpoint, + "sessions", + Map::from_iter([("action".to_string(), Value::String("list".to_string()))]), + None, + ) + .expect("current capability calls"); + assert_eq!(result, Value::String("ok".to_string())); + + let ping = requests.recv().expect("record ping"); + assert_eq!(ping.request_line, "GET /v1/ping HTTP/1.1"); + assert_eq!( + ping.authorization.as_deref(), + Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + ); + assert!(ping.body.is_empty()); + assert!(!ping.expects_continue); + + let call = requests.recv().expect("record call"); + assert_eq!(call.request_line, "POST /v1/call HTTP/1.1"); + assert_eq!( + call.authorization.as_deref(), + Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + ); + assert!(call.expects_continue); + let call_body: Value = serde_json::from_str(&call.body).expect("call body is JSON"); + assert_eq!(call_body["command"], "sessions"); + assert_eq!(call_body["args"]["action"], "list"); + + broker.join().expect("test broker exits"); + } + + #[cfg(unix)] + #[test] + fn call_rehandshakes_once_after_broker_auth_403_and_retries_identical_payload() { + let (port, requests, broker) = spawn_broker_sequence(vec![ + (STALE_CAPABILITY, BrokerResponse::Ping { generation: 7 }), + (CURRENT_CAPABILITY, BrokerResponse::Call), + (CURRENT_CAPABILITY, BrokerResponse::Ping { generation: 7 }), + (CURRENT_CAPABILITY, BrokerResponse::Call), + ]); + let lock_file = TempDiscoveryFile::new("call-capability-rotation", port, STALE_CAPABILITY); + let endpoint = handshake(&lock_file.0).expect("stale endpoint handshakes before rotation"); + lock_file.replace(port, CURRENT_CAPABILITY); + + let args = Map::from_iter([ + ("action".to_string(), Value::String("create".to_string())), + ( + "name".to_string(), + Value::String("payload must survive retry".to_string()), + ), + ]); + let result = call(&lock_file.0, &endpoint, "sessions", args, Some(4_242)) + .expect("broker auth rejection is retried with the rotated capability"); + assert_eq!(result, Value::String("ok".to_string())); + + let initial_ping = requests.recv().expect("record initial ping"); + let rejected_call = requests.recv().expect("record rejected call"); + let retry_ping = requests.recv().expect("record retry handshake ping"); + let retried_call = requests.recv().expect("record retried call"); + broker + .join() + .expect("test broker exits after exactly four requests"); + + assert_eq!( + initial_ping.authorization.as_deref(), + Some(format!("Bearer {STALE_CAPABILITY}").as_str()) + ); + assert_eq!( + rejected_call.authorization.as_deref(), + Some(format!("Bearer {STALE_CAPABILITY}").as_str()) + ); + assert_eq!( + retry_ping.authorization.as_deref(), + Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + ); + assert_eq!( + retried_call.authorization.as_deref(), + Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + ); + assert!(rejected_call.expects_continue); + assert!(retried_call.expects_continue); + assert_eq!( + rejected_call.body, retried_call.body, + "retry must preserve the complete payload" + ); + assert_eq!( + serde_json::from_str::(&retried_call.body).expect("retried payload is JSON"), + serde_json::json!({ + "command": "sessions", + "args": {"action": "create", "name": "payload must survive retry"}, + "timeout_ms": 4242, + }) + ); + } + + #[cfg(unix)] + #[test] + fn call_does_not_retry_other_403_responses() { + let (port, requests, broker) = spawn_broker_sequence(vec![ + (CURRENT_CAPABILITY, BrokerResponse::Ping { generation: 7 }), + ( + CURRENT_CAPABILITY, + BrokerResponse::CallError { + status: "403 Forbidden", + body: r#"{"ok":false,"error":{"code":"forbidden","message":"origin rejected"}}"#, + }, + ), + ]); + let lock_file = TempDiscoveryFile::new("call-other-403", port, CURRENT_CAPABILITY); + let endpoint = handshake(&lock_file.0).expect("current endpoint handshakes"); + + let failure = call( + &lock_file.0, + &endpoint, + "sessions", + Map::from_iter([("action".to_string(), Value::String("list".to_string()))]), + None, + ) + .expect_err("non-broker 403 must not be retried"); + assert_eq!(failure.exit, EXIT_TRANSPORT); + assert!(failure.message.starts_with("forbidden: origin rejected")); + + let ping = requests.recv().expect("record initial ping"); + let call = requests.recv().expect("record call"); + broker + .join() + .expect("test broker exits after exactly two requests"); + assert_eq!(ping.request_line, "GET /v1/ping HTTP/1.1"); + assert_eq!(call.request_line, "POST /v1/call HTTP/1.1"); + } + + #[cfg(unix)] + #[test] + fn handshake_recovers_when_capability_rotates_after_discovery_read() { + let first_request = Arc::new(std::sync::Barrier::new(2)); + let response_ready = Arc::new(std::sync::Barrier::new(2)); + let (port, requests, broker) = spawn_broker_responses_with_sync( + vec![ + (CURRENT_CAPABILITY, BrokerResponse::Ping { generation: 7 }), + (CURRENT_CAPABILITY, BrokerResponse::Ping { generation: 7 }), + ], + Some((first_request.clone(), response_ready.clone())), + ); + let lock_file = TempDiscoveryFile::new("rotating", port, STALE_CAPABILITY); + let path = lock_file.0.clone(); + let rewrite_first_request = first_request.clone(); + let rewrite_response_ready = response_ready.clone(); + let rewrite = thread::spawn(move || { + rewrite_first_request.wait(); + std::fs::write( + &path, + format!( + r#"{{"port":{port},"pid":4242,"generation":7,"protocolVersion":{PROTOCOL_VERSION},"capability":"{CURRENT_CAPABILITY}"}}"# + ), + ) + .expect("publish rotated discovery capability"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("keep rewritten discovery private"); + } + rewrite_response_ready.wait(); + let stale_ping = requests.recv().expect("record stale ping"); + assert_eq!( + stale_ping.authorization.as_deref(), + Some(format!("Bearer {STALE_CAPABILITY}").as_str()) + ); + requests.recv().expect("record retried current ping") + }); + + let endpoint = handshake(&lock_file.0).expect("rotated capability retries successfully"); + assert_eq!(endpoint.port, port); + let current_ping = rewrite.join().expect("discovery rewrite exits"); + assert_eq!( + current_ping.authorization.as_deref(), + Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + ); + broker.join().expect("test broker exits"); + } + + #[cfg(unix)] + #[test] + fn handshake_retries_generation_mismatch_with_rotated_capability() { + let first_request = Arc::new(std::sync::Barrier::new(2)); + let response_ready = Arc::new(std::sync::Barrier::new(2)); + let (port, requests, broker) = spawn_broker_responses_with_sync( + vec![ + (STALE_CAPABILITY, BrokerResponse::Ping { generation: 6 }), + (CURRENT_CAPABILITY, BrokerResponse::Ping { generation: 7 }), + ], + Some((first_request.clone(), response_ready.clone())), + ); + let lock_file = TempDiscoveryFile::new("generation-race", port, STALE_CAPABILITY); + let path = lock_file.0.clone(); + let rewrite = thread::spawn(move || { + first_request.wait(); + let stale_ping = requests.recv().expect("record old-generation ping"); + assert_eq!( + stale_ping.authorization.as_deref(), + Some(format!("Bearer {STALE_CAPABILITY}").as_str()) + ); + std::fs::write( + &path, + format!( + r#"{{"port":{port},"pid":4242,"generation":7,"protocolVersion":{PROTOCOL_VERSION},"capability":"{CURRENT_CAPABILITY}"}}"# + ), + ) + .expect("publish new generation and capability"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("keep rewritten discovery private"); + } + response_ready.wait(); + requests.recv().expect("record new-generation ping") + }); + + let endpoint = handshake(&lock_file.0).expect("generation mismatch retries successfully"); + assert_eq!(endpoint.port, port); + let current_ping = rewrite.join().expect("discovery rewrite exits"); + assert_eq!( + current_ping.authorization.as_deref(), + Some(format!("Bearer {CURRENT_CAPABILITY}").as_str()) + ); + broker.join().expect("test broker exits"); + } + + #[cfg(unix)] + #[test] + fn handshake_rejects_stale_capability() { + // The first 403 triggers the one permitted discovery re-read; an + // unchanged stale record must still fail closed on the second probe. + let (port, requests, broker) = spawn_broker(CURRENT_CAPABILITY, 2); + let lock_file = TempDiscoveryFile::new("stale", port, STALE_CAPABILITY); + + let failure = handshake(&lock_file.0).expect_err("stale capability fails closed"); + assert_eq!(failure.exit, EXIT_ENV); + assert!(failure.message.contains("ping returned status 403")); + for _ in 0..2 { + let ping = requests.recv().expect("record stale ping"); + assert_eq!( + ping.authorization.as_deref(), + Some(format!("Bearer {STALE_CAPABILITY}").as_str()) + ); + } + + broker.join().expect("test broker exits"); + } + + #[test] + fn only_the_broker_auth_403_is_safe_to_retry() { + assert!(is_broker_auth_forbidden( + 403, + r#"{"ok":false,"error":{"code":"forbidden","message":"valid bearer capability required"}}"#, + )); + assert!(!is_broker_auth_forbidden( + 403, + r#"{"ok":false,"error":{"code":"forbidden","message":"origin rejected"}}"#, + )); + assert!(!is_broker_auth_forbidden( + 500, + r#"{"ok":false,"error":{"code":"forbidden","message":"valid bearer capability required"}}"#, + )); + } #[test] fn ok_true_yields_the_result_verbatim() { diff --git a/src-tauri/crates/berdctl/src/discovery.rs b/src-tauri/crates/berdctl/src/discovery.rs index 078d63e1f..a11679a82 100644 --- a/src-tauri/crates/berdctl/src/discovery.rs +++ b/src-tauri/crates/berdctl/src/discovery.rs @@ -1,4 +1,5 @@ -//! Discovery-file resolution: how berdctl finds the app's control endpoint. +//! Discovery-file resolution: how berdctl finds and authenticates to the +//! app's control endpoint. use std::path::{Path, PathBuf}; use std::time::Duration; @@ -11,7 +12,7 @@ use crate::client::Failure; /// `PROTOCOL_VERSION` in the `tauri-plugin-berdctl` crate /// (src-tauri/plugins/berdctl) — the CLI does not depend on the plugin /// crate; bump both copies together. -pub const PROTOCOL_VERSION: u32 = 4; +pub const PROTOCOL_VERSION: u32 = 5; /// Exact wording pinned by the implementation spec: the missing env var is the /// provenance signal that we are not running under the app. @@ -19,17 +20,52 @@ pub const NOT_UNDER_APP: &str = "berdctl must run inside a Berd desktop app session (the app sets this up automatically)"; const REREAD_DELAY: Duration = Duration::from_millis(200); +const CAPABILITY_HEX_LEN: usize = 64; +const MAX_DISCOVERY_BYTES: u64 = 4096; /// Shape of the discovery file the berdctl broker writes on start /// (`/berdctl/control-.json`). Duplicated by hand from /// the writer's struct in `tauri-plugin-berdctl`; keep in sync. #[derive(Debug, Deserialize, PartialEq, Eq)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", try_from = "RawDiscoveryFile")] pub struct DiscoveryFile { pub port: u16, pub pid: u32, pub generation: u64, pub protocol_version: u32, + pub capability: String, +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct RawDiscoveryFile { + port: u16, + pid: u32, + generation: u64, + protocol_version: u32, + capability: String, +} + +impl TryFrom for DiscoveryFile { + type Error = String; + + fn try_from(raw: RawDiscoveryFile) -> Result { + if raw.capability.len() != CAPABILITY_HEX_LEN + || !raw + .capability + .bytes() + .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte)) + { + return Err("capability must be a 256-bit hexadecimal value".to_string()); + } + Ok(Self { + port: raw.port, + pid: raw.pid, + generation: raw.generation, + protocol_version: raw.protocol_version, + capability: raw.capability, + }) + } } /// The lock path comes from `--lock-path` or `BERDCTL_LOCK` (clap merges @@ -46,11 +82,304 @@ pub fn parse(contents: &str) -> Result { } pub fn load(path: &Path) -> Result { - let contents = std::fs::read_to_string(path) - .map_err(|err| format!("cannot read {}: {err}", path.display()))?; + let contents = read_private_discovery_file(path)?; parse(&contents) } +#[cfg(unix)] +fn read_private_discovery_file(path: &Path) -> Result { + use std::io::Read; + use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; + + // Check the containing directory first. Once it is owner-private, another + // user cannot replace the final path while it is opened below. + let parent = path + .parent() + .ok_or_else(|| format!("{} has no parent directory", path.display()))?; + let parent_metadata = std::fs::symlink_metadata(parent) + .map_err(|err| format!("cannot inspect {}: {err}", parent.display()))?; + // SAFETY: `geteuid` takes no arguments and has no preconditions. + let current_uid = unsafe { libc::geteuid() }; + if !parent_metadata.file_type().is_dir() + || parent_metadata.uid() != current_uid + || parent_metadata.mode() & 0o077 != 0 + { + return Err(format!( + "{} is not an owner-private directory (expected mode 0700)", + parent.display() + )); + } + + // O_NOFOLLOW makes the final symlink check atomic with opening the file. + // O_NONBLOCK keeps a malicious FIFO from blocking before metadata reveals + // that it is not a regular file. + let file = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) + .open(path) + .map_err(|err| format!("cannot open {}: {err}", path.display()))?; + let metadata = file + .metadata() + .map_err(|err| format!("cannot inspect {}: {err}", path.display()))?; + if !metadata.file_type().is_file() { + return Err(format!("{} is not a regular file", path.display())); + } + if metadata.uid() != current_uid { + return Err(format!( + "{} is not owned by the current user", + path.display() + )); + } + if metadata.mode() & 0o077 != 0 { + return Err(format!( + "{} is accessible by other users (expected mode 0600)", + path.display() + )); + } + if metadata.len() > MAX_DISCOVERY_BYTES { + return Err(format!("{} is unexpectedly large", path.display())); + } + + // Limit the read too: the handle may grow after the metadata check, but it + // must never make berdctl allocate an unbounded discovery record. + let mut contents = String::new(); + file.take(MAX_DISCOVERY_BYTES + 1) + .read_to_string(&mut contents) + .map_err(|err| format!("cannot read {}: {err}", path.display()))?; + if contents.len() as u64 > MAX_DISCOVERY_BYTES { + return Err(format!("{} is unexpectedly large", path.display())); + } + Ok(contents) +} + +#[cfg(windows)] +fn read_private_discovery_file(path: &Path) -> Result { + use std::fs::File; + use std::io::Read; + use std::mem::zeroed; + use std::os::windows::ffi::OsStrExt; + use std::os::windows::io::{AsRawHandle, FromRawHandle, RawHandle}; + use windows_sys::Win32::Foundation::{GENERIC_READ, INVALID_HANDLE_VALUE}; + use windows_sys::Win32::Security::Authorization::{GetSecurityInfo, SE_FILE_OBJECT}; + use windows_sys::Win32::Security::{DACL_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION}; + use windows_sys::Win32::Storage::FileSystem::{ + CreateFileW, GetFileInformationByHandle, GetFileType, BY_HANDLE_FILE_INFORMATION, + FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_REPARSE_POINT, FILE_SHARE_DELETE, FILE_SHARE_READ, + FILE_SHARE_WRITE, FILE_TYPE_DISK, OPEN_EXISTING, + }; + + // FILE_FLAG_OPEN_REPARSE_POINT makes the handle refer to the reparse point + // itself rather than its target. This makes the reparse-point check below + // apply to the object we opened, not to a potentially attacker-selected target. + const FILE_FLAG_OPEN_REPARSE_POINT: u32 = 0x0020_0000; + const READ_CONTROL: u32 = 0x0002_0000; + + let mut wide: Vec = path.as_os_str().encode_wide().collect(); + if wide.contains(&0) { + return Err(format!("cannot open {}: path contains NUL", path.display())); + } + wide.push(0); + let handle = unsafe { + CreateFileW( + wide.as_ptr(), + GENERIC_READ | READ_CONTROL, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + std::ptr::null(), + OPEN_EXISTING, + FILE_FLAG_OPEN_REPARSE_POINT, + std::ptr::null_mut(), + ) + }; + if handle == INVALID_HANDLE_VALUE { + return Err(format!( + "cannot open {}: {}", + path.display(), + std::io::Error::last_os_error() + )); + } + // SAFETY: CreateFileW above returned an owned, valid handle. + let file = unsafe { File::from_raw_handle(handle as RawHandle) }; + + let mut info: BY_HANDLE_FILE_INFORMATION = unsafe { zeroed() }; + if unsafe { GetFileInformationByHandle(file.as_raw_handle() as _, &mut info) } == 0 { + return Err(format!( + "cannot inspect {}: {}", + path.display(), + std::io::Error::last_os_error() + )); + } + if info.dwFileAttributes & (FILE_ATTRIBUTE_REPARSE_POINT | FILE_ATTRIBUTE_DIRECTORY) != 0 + || unsafe { GetFileType(file.as_raw_handle() as _) } != FILE_TYPE_DISK + { + return Err(format!( + "{} is not a regular, non-reparse file", + path.display() + )); + } + let size = (u64::from(info.nFileSizeHigh) << 32) | u64::from(info.nFileSizeLow); + if size > MAX_DISCOVERY_BYTES { + return Err(format!("{} is unexpectedly large", path.display())); + } + + // GetSecurityInfo returns a self-contained descriptor for this handle. The + // ownership and DACL checks are therefore bound to the object read below, + // rather than a path that could be replaced between checks. + let mut owner = std::ptr::null_mut(); + let mut dacl = std::ptr::null_mut(); + let mut descriptor = std::ptr::null_mut(); + let security_error = unsafe { + GetSecurityInfo( + file.as_raw_handle() as _, + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, + &mut owner, + std::ptr::null_mut(), + &mut dacl, + std::ptr::null_mut(), + &mut descriptor, + ) + }; + if security_error != 0 { + return Err(format!( + "cannot inspect permissions for {}: Windows error {security_error}", + path.display() + )); + } + let permissions = unsafe { windows_discovery_permissions_are_private(owner, dacl) }; + // SAFETY: GetSecurityInfo allocated this descriptor with LocalAlloc. + unsafe { windows_sys::Win32::Foundation::LocalFree(descriptor as _) }; + permissions.map_err(|reason| format!("{} {reason}", path.display()))?; + + let mut contents = String::new(); + file.take(MAX_DISCOVERY_BYTES + 1) + .read_to_string(&mut contents) + .map_err(|err| format!("cannot read {}: {err}", path.display()))?; + if contents.len() as u64 > MAX_DISCOVERY_BYTES { + return Err(format!("{} is unexpectedly large", path.display())); + } + Ok(contents) +} + +/// Verifies the handle's owner and DACL. An absent DACL grants everyone full +/// access, and an allow ACE for an identity other than the owner, SYSTEM, or +/// Administrators can expose or replace the capability, so both fail closed. +#[cfg(windows)] +unsafe fn windows_discovery_permissions_are_private( + owner: windows_sys::Win32::Security::PSID, + dacl: *mut windows_sys::Win32::Security::ACL, +) -> Result<(), &'static str> { + use std::mem::size_of; + use windows_sys::Win32::Foundation::{CloseHandle, HANDLE}; + use windows_sys::Win32::Security::{ + EqualSid, GetTokenInformation, TokenUser, TOKEN_QUERY, TOKEN_USER, + }; + use windows_sys::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; + + if owner.is_null() || dacl.is_null() { + return Err("does not have an owner-private DACL"); + } + let mut token: HANDLE = std::ptr::null_mut(); + if unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token) } == 0 { + return Err("cannot determine the current user"); + } + let mut size = 0; + unsafe { GetTokenInformation(token, TokenUser, std::ptr::null_mut(), 0, &mut size) }; + if (size as usize) < size_of::() { + unsafe { CloseHandle(token) }; + return Err("cannot determine the current user"); + } + let mut bytes = vec![0_usize; (size as usize).div_ceil(size_of::())]; + let ok = unsafe { + GetTokenInformation(token, TokenUser, bytes.as_mut_ptr().cast(), size, &mut size) + } != 0; + unsafe { CloseHandle(token) }; + if !ok || bytes.len() * size_of::() < size_of::() { + return Err("cannot determine the current user"); + } + let current_user = unsafe { (*(bytes.as_ptr().cast::())).User.Sid }; + if current_user.is_null() || unsafe { EqualSid(owner, current_user) } == 0 { + return Err("is not owned by the current user"); + } + + // ACL and ACE layouts start with these fixed Windows ABI fields. Parsing + // only ordinary/callback allow ACEs lets us fail closed on every less + // familiar ACE type rather than accidentally treating it as private. + let acl = unsafe { &*dacl }; + let mut offset = size_of::(); + let acl_size = usize::from(acl.AclSize); + if acl_size < offset { + return Err("has a malformed DACL"); + } + for _ in 0..acl.AceCount { + if offset.checked_add(8).is_none_or(|end| end > acl_size) { + return Err("has a malformed DACL"); + } + let ace = unsafe { (dacl as *const u8).add(offset) }; + let ace_type = unsafe { *ace }; + let ace_size = usize::from(unsafe { *(ace.add(2).cast::()) }); + if ace_size < 8 + || ace_size % 4 != 0 + || offset + .checked_add(ace_size) + .is_none_or(|end| end > acl_size) + { + return Err("has a malformed DACL"); + } + // ACCESS_ALLOWED_ACE_TYPE and ACCESS_ALLOWED_CALLBACK_ACE_TYPE. + if matches!(ace_type, 0 | 9) { + let sid = unsafe { ace.add(8).cast_mut().cast() }; + if !unsafe { windows_ace_sid_is_trusted(sid, ace_size - 8, current_user) } { + return Err("is accessible by other users"); + } + } else if !matches!(ace_type, 1 | 6 | 10 | 12) { + // Deny ACEs only further restrict access. Every other ACE type is + // rejected, including object/callback allow ACEs whose SID has a + // variable layout, so an unfamiliar granting ACE cannot slip by. + return Err("has an unsupported DACL entry"); + } + offset += ace_size; + } + Ok(()) +} + +#[cfg(windows)] +unsafe fn windows_ace_sid_is_trusted( + sid: windows_sys::Win32::Security::PSID, + available_bytes: usize, + current_user: windows_sys::Win32::Security::PSID, +) -> bool { + use windows_sys::Win32::Security::EqualSid; + + if sid.is_null() || available_bytes < 8 { + return false; + } + // The SID is embedded in the ACE, so validate its variable-length layout + // against that ACE before passing it to Win32 or inspecting its fields. + let sid = sid as *const u8; + let revision = unsafe { *sid }; + let count = unsafe { *sid.add(1) }; + let Some(sid_len) = 8usize.checked_add(usize::from(count).saturating_mul(4)) else { + return false; + }; + if revision != 1 || sid_len > available_bytes { + return false; + } + if unsafe { EqualSid(sid.cast_mut().cast(), current_user) } != 0 { + return true; + } + // S-1-5-18 (LOCAL SYSTEM) and S-1-5-32-544 (BUILTIN\\Administrators) + // are privileged principals, not other unprivileged users. + let authority = unsafe { std::slice::from_raw_parts(sid.add(2), 6) }; + let sub_authorities = + unsafe { std::slice::from_raw_parts(sid.add(8).cast::(), usize::from(count)) }; + authority == [0, 0, 0, 0, 0, 5] && matches!(sub_authorities, [18] | [32, 544]) +} + +#[cfg(all(not(unix), not(windows)))] +fn read_private_discovery_file(path: &Path) -> Result { + std::fs::read_to_string(path).map_err(|err| format!("cannot read {}: {err}", path.display())) +} + /// The broker writes the file atomically, so a read/parse failure is either /// transient (broker restarting) or means the app is gone; one short retry /// distinguishes the two. @@ -74,7 +403,8 @@ pub fn load_with_retry(path: &Path) -> Result { mod tests { use super::*; - const VALID: &str = r#"{"port":52341,"pid":4242,"generation":3,"protocolVersion":1}"#; + const CAPABILITY: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + const VALID: &str = r#"{"port":52341,"pid":4242,"generation":3,"protocolVersion":1,"capability":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"}"#; #[test] fn parses_a_valid_discovery_file() { @@ -86,14 +416,17 @@ mod tests { pid: 4242, generation: 3, protocol_version: 1, + capability: CAPABILITY.to_string(), } ); } #[test] fn tolerates_unknown_fields_for_forward_compat() { - let file = parse(r#"{"port":1,"pid":2,"generation":3,"protocolVersion":1,"token":"x"}"#) - .expect("unknown fields are ignored"); + let file = parse( + r#"{"port":1,"pid":2,"generation":3,"protocolVersion":1,"capability":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef","future":"x"}"#, + ) + .expect("unknown fields are ignored"); assert_eq!(file.port, 1); } @@ -106,14 +439,37 @@ mod tests { #[test] fn rejects_missing_fields() { assert!(parse(r#"{"port":52341,"pid":4242}"#).is_err()); + assert!( + parse(r#"{"port":52341,"pid":4242,"generation":3,"protocolVersion":1}"#).is_err(), + "legacy discovery without a capability must fail closed" + ); assert!(parse(r#"{}"#).is_err()); } #[test] - fn rejects_wrongly_typed_fields() { + fn rejects_wrongly_typed_or_malformed_fields() { + assert!(parse( + r#"{"port":"not-a-port","pid":1,"generation":1,"protocolVersion":1,"capability":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"}"# + ) + .is_err()); assert!( - parse(r#"{"port":"not-a-port","pid":1,"generation":1,"protocolVersion":1}"#).is_err() + parse(r#"{"port":1,"pid":1,"generation":1,"protocolVersion":1,"capability":123}"#) + .is_err() ); + for capability in [ + "", + "short", + "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", + "0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF", + ] { + let contents = format!( + r#"{{"port":1,"pid":1,"generation":1,"protocolVersion":1,"capability":"{capability}"}}"# + ); + assert!( + parse(&contents).is_err(), + "malformed capability {capability:?} must fail closed" + ); + } } #[test] @@ -135,4 +491,106 @@ mod tests { .expect("present path resolves"); assert_eq!(path, PathBuf::from("/tmp/control-1.json")); } + + #[cfg(unix)] + #[test] + fn load_accepts_private_discovery_file_from_shared_working_directory() { + use std::os::unix::fs::PermissionsExt; + + let base = + std::env::temp_dir().join(format!("berdctl-discovery-private-{}", std::process::id())); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + let path = base.join("control.json"); + std::fs::write(&path, VALID).unwrap(); + std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + + assert_eq!(load(&path).expect("private discovery loads").port, 52341); + std::fs::remove_dir_all(base).ok(); + } + + #[cfg(unix)] + #[test] + fn load_rejects_permissive_discovery_file() { + use std::os::unix::fs::PermissionsExt; + + let base = std::env::temp_dir().join(format!( + "berdctl-discovery-permissions-{}", + std::process::id() + )); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + let path = base.join("control.json"); + std::fs::write(&path, VALID).unwrap(); + std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + let error = load(&path).expect_err("world-readable capability must fail closed"); + assert!(error.contains("accessible by other users")); + std::fs::remove_dir_all(base).ok(); + } + + #[cfg(unix)] + #[test] + fn load_rejects_permissive_discovery_directory() { + use std::os::unix::fs::PermissionsExt; + + let base = std::env::temp_dir().join(format!( + "berdctl-discovery-directory-permissions-{}", + std::process::id() + )); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + let path = base.join("control.json"); + std::fs::write(&path, VALID).unwrap(); + std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o755)).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + + let error = load(&path).expect_err("shared discovery directory must fail closed"); + assert!(error.contains("not an owner-private directory")); + std::fs::remove_dir_all(base).ok(); + } + + #[cfg(unix)] + #[test] + fn load_rejects_symlinked_discovery_file() { + use std::os::unix::fs::{symlink, PermissionsExt}; + + let base = + std::env::temp_dir().join(format!("berdctl-discovery-symlink-{}", std::process::id())); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)).unwrap(); + let target = base.join("target.json"); + let link = base.join("control.json"); + std::fs::write(&target, VALID).unwrap(); + std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o600)).unwrap(); + symlink(&target, &link).unwrap(); + assert!(load(&link).is_err(), "symlink must fail closed"); + std::fs::remove_dir_all(base).ok(); + } + + #[cfg(unix)] + #[test] + fn load_rejects_non_regular_and_oversized_discovery_files() { + use std::os::unix::fs::PermissionsExt; + + let base = + std::env::temp_dir().join(format!("berdctl-discovery-shape-{}", std::process::id())); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)).unwrap(); + + let directory_path = base.join("control-dir"); + std::fs::create_dir(&directory_path).unwrap(); + assert!(load(&directory_path).is_err(), "directory must fail closed"); + + let oversized_path = base.join("control-large.json"); + std::fs::write(&oversized_path, vec![b'x'; 4097]).unwrap(); + std::fs::set_permissions(&oversized_path, std::fs::Permissions::from_mode(0o600)).unwrap(); + let error = load(&oversized_path).expect_err("oversized discovery must fail closed"); + assert!(error.contains("unexpectedly large")); + + std::fs::remove_dir_all(base).ok(); + } } diff --git a/src-tauri/crates/berdctl/src/main.rs b/src-tauri/crates/berdctl/src/main.rs index e762ffeec..bd9d7f859 100644 --- a/src-tauri/crates/berdctl/src/main.rs +++ b/src-tauri/crates/berdctl/src/main.rs @@ -56,7 +56,7 @@ fn main() -> ExitCode { fn run(command: &str, args: Map, globals: &wire::Globals) -> Result<(), Failure> { let lock_path = discovery::resolve_lock_path(globals.lock_path.clone())?; let endpoint = client::handshake(&lock_path)?; - let result = client::call(&endpoint, command, args, globals.timeout_ms)?; + let result = client::call(&lock_path, &endpoint, command, args, globals.timeout_ms)?; let rendered = if globals.json { serde_json::to_string(&result) } else { diff --git a/src-tauri/crates/berdctl/src/validate.rs b/src-tauri/crates/berdctl/src/validate.rs index 9b4ae3cde..80bf464d0 100644 --- a/src-tauri/crates/berdctl/src/validate.rs +++ b/src-tauri/crates/berdctl/src/validate.rs @@ -192,7 +192,7 @@ mod tests { use crate::contract::Contract; const MINIMAL_API: &str = r#"{ - "protocolVersion": 4, + "protocolVersion": 5, "groups": { "sessions": { "description": "Manage the user's chat sessions.", @@ -374,7 +374,7 @@ mod tests { #[test] fn mismatched_protocol_version_is_reported() { - let api = MINIMAL_API.replace("\"protocolVersion\": 4", "\"protocolVersion\": 999"); + let api = MINIMAL_API.replace("\"protocolVersion\": 5", "\"protocolVersion\": 999"); let errors = errors_for(&api, MINIMAL_SURFACE); assert_one_error_containing(&errors, "protocolVersion 999 does not match"); } diff --git a/src-tauri/plugins/berdctl/Cargo.toml b/src-tauri/plugins/berdctl/Cargo.toml index 09a2a11eb..2f7567563 100644 --- a/src-tauri/plugins/berdctl/Cargo.toml +++ b/src-tauri/plugins/berdctl/Cargo.toml @@ -10,18 +10,33 @@ name = "tauri_plugin_berdctl" path = "src/lib.rs" [dependencies] +getrandom = { version = "0.4", optional = true } +hex = { version = "0.4", optional = true } log = "0.4" serde = { version = "1", features = ["derive"] } serde_json = "1" +subtle = { version = "2", optional = true } tauri = { version = "2", default-features = false } tokio = { version = "1", features = ["sync", "time", "rt", "net"] } uuid = { version = "1", features = ["v4"] } axum = { version = "0.8", default-features = false, features = ["http1", "json", "tokio"], optional = true } +[target.'cfg(unix)'.dependencies] +libc = { version = "0.2", optional = true } + +[target.'cfg(windows)'.dependencies] +windows-sys = { version = "0.59", optional = true, features = [ + "Win32_Foundation", + "Win32_Security", + "Win32_Security_Authorization", + "Win32_Storage_FileSystem", + "Win32_System_Threading", +] } + [features] # Without `server` the crate compiles to an inert stub: permissions are still # generated by build.rs, but no runtime code (including `init`) exists. -server = ["dep:axum"] +server = ["dep:axum", "dep:getrandom", "dep:hex", "dep:libc", "dep:subtle", "dep:windows-sys"] [dev-dependencies] reqwest = { version = "0.13", default-features = false, features = ["json"] } diff --git a/src-tauri/plugins/berdctl/src/discovery.rs b/src-tauri/plugins/berdctl/src/discovery.rs index e1ea06495..2f52e33e2 100644 --- a/src-tauri/plugins/berdctl/src/discovery.rs +++ b/src-tauri/plugins/berdctl/src/discovery.rs @@ -1,5 +1,6 @@ -//! Per-instance discovery ("lock") file the berdctl CLI reads to find the -//! running broker: `{port, pid, generation, protocolVersion}`. +//! Per-instance discovery ("lock") file the berdctl CLI reads to find and +//! authenticate to the running broker: `{port, pid, generation, +//! protocolVersion, capability}`. //! //! The path formula and protocol version are exported unconditionally (not //! behind the `server` feature) so the app crate can compute the path for the @@ -12,17 +13,21 @@ use std::path::{Path, PathBuf}; /// (src-tauri/crates/berdctl); the CLI does not depend on this crate — /// bump both together. #[cfg_attr(not(feature = "server"), allow(dead_code))] -pub const PROTOCOL_VERSION: u32 = 4; +pub const PROTOCOL_VERSION: u32 = 5; /// Directory under the app data dir holding the per-instance discovery files. pub const DISCOVERY_DIR_NAME: &str = "berdctl"; const DISCOVERY_FILE_PREFIX: &str = "control-"; const DISCOVERY_FILE_SUFFIX: &str = ".json"; -/// A crash between the temp-file write and the atomic rename below leaves -/// `control-.json.tmp` behind; the app crate's stale-file sweep owns -/// those orphans too. -const DISCOVERY_TEMP_SUFFIX: &str = ".json.tmp"; +/// A crash between a temp-file write and its atomic rename can leave either +/// the legacy fixed-name `control-.json.tmp` orphan or the current +/// `control-.json..tmp` orphan. The app crate's stale-file sweep +/// owns both forms. +const LEGACY_DISCOVERY_TEMP_SUFFIX: &str = ".json.tmp"; +const DISCOVERY_TEMP_MARKER: &str = ".json."; +const DISCOVERY_TEMP_SUFFIX: &str = ".tmp"; +const DISCOVERY_TEMP_NONCE_HEX_LEN: usize = 32; /// `/berdctl/control-.json`. Per-instance (pid /// suffix): dev worktrees share a bundle identifier, so a well-known filename @@ -33,24 +38,446 @@ pub fn discovery_file_path(app_data_dir: &Path, pid: u32) -> PathBuf { )) } -/// Owning app pid encoded in a discovery file name: `control-.json` or -/// its orphaned temp form `control-.json.tmp`. `None` for anything else. +/// Owning app pid encoded in a discovery file name. Recognized forms are the +/// final `control-.json`, legacy `control-.json.tmp`, and current +/// `control-.json.<32 lowercase hex chars>.tmp` orphan names. `None` for +/// anything else, so the stale-file sweep cannot delete unrelated files. pub fn owner_pid_from_discovery_file_name(name: &str) -> Option { let stem = name.strip_prefix(DISCOVERY_FILE_PREFIX)?; - stem.strip_suffix(DISCOVERY_TEMP_SUFFIX) - .or_else(|| stem.strip_suffix(DISCOVERY_FILE_SUFFIX))? - .parse() - .ok() + let pid = if let Some(pid) = stem.strip_suffix(DISCOVERY_FILE_SUFFIX) { + pid + } else if let Some(pid) = stem.strip_suffix(LEGACY_DISCOVERY_TEMP_SUFFIX) { + pid + } else { + let (pid, nonce_with_suffix) = stem.split_once(DISCOVERY_TEMP_MARKER)?; + let nonce = nonce_with_suffix.strip_suffix(DISCOVERY_TEMP_SUFFIX)?; + if nonce.len() != DISCOVERY_TEMP_NONCE_HEX_LEN + || !nonce + .bytes() + .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte)) + { + return None; + } + pid + }; + pid.parse().ok() +} + +#[cfg(all(feature = "server", windows))] +mod windows_discovery_security { + use std::{ffi::c_void, mem::size_of, os::windows::ffi::OsStrExt, path::Path}; + + use windows_sys::Win32::{ + Foundation::{ + CloseHandle, GetLastError, ERROR_INSUFFICIENT_BUFFER, HANDLE, INVALID_HANDLE_VALUE, + }, + Security::Authorization::{SetSecurityInfo, SE_FILE_OBJECT}, + Security::{ + AddAccessAllowedAceEx, GetLengthSid, GetTokenInformation, InitializeAcl, TokenUser, + ACL, ACL_REVISION, DACL_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION, + PROTECTED_DACL_SECURITY_INFORMATION, TOKEN_QUERY, TOKEN_USER, + }, + Storage::FileSystem::{ + CreateFileW, FileAttributeTagInfo, GetFileInformationByHandleEx, MoveFileExW, + CREATE_NEW, FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_NORMAL, + FILE_ATTRIBUTE_REPARSE_POINT, FILE_ATTRIBUTE_TAG_INFO, FILE_FLAG_BACKUP_SEMANTICS, + FILE_FLAG_OPEN_REPARSE_POINT, FILE_SHARE_READ, FILE_SHARE_WRITE, + MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, OPEN_EXISTING, + }, + System::Threading::{GetCurrentProcess, OpenProcessToken}, + }; + + // These access and inheritance values are stable Win32 ABI constants. They + // intentionally avoid relying on an inherited ACL from an app-data parent. + const READ_CONTROL: u32 = 0x0002_0000; + const WRITE_DAC: u32 = 0x0004_0000; + const WRITE_OWNER: u32 = 0x0008_0000; + const GENERIC_WRITE: u32 = 0x4000_0000; + const GENERIC_ALL: u32 = 0x1000_0000; + const OBJECT_INHERIT_ACE: u32 = 0x01; + const CONTAINER_INHERIT_ACE: u32 = 0x02; + const MAX_TOKEN_USER_INFO_SIZE: u32 = 64 * 1024; + const MAX_SID_LENGTH: usize = 68; + + pub(super) struct PrivateDirectory(HANDLE); + + impl Drop for PrivateDirectory { + fn drop(&mut self) { + // SAFETY: this type owns a successful CreateFileW handle. + unsafe { CloseHandle(self.0) }; + } + } + + fn win_error(context: &str, code: u32) -> std::io::Error { + std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + format!("{context}: Windows error {code}"), + ) + } + + fn path_as_wide_null(path: &Path) -> Vec { + path.as_os_str().encode_wide().chain(Some(0)).collect() + } + + /// Kept separate from the syscall wrapper so the security decision is + /// directly unit-testable without creating Windows filesystem objects. + fn is_plain_directory(attributes: u32) -> bool { + attributes & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT) + == FILE_ATTRIBUTE_DIRECTORY + } + + fn acl_size_for_sid(sid_len: usize) -> usize { + size_of::() + 8 + sid_len + } + + fn is_bounded_token_user_info_size(size: u32) -> bool { + (size as usize) >= size_of::() && size <= MAX_TOKEN_USER_INFO_SIZE + } + + unsafe fn with_current_user_sid( + f: impl FnOnce(*mut c_void) -> std::io::Result, + ) -> std::io::Result { + let mut token: HANDLE = std::ptr::null_mut(); + // SAFETY: pseudo process handle is valid and `token` is writable. + if unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token) } == 0 { + return Err(win_error("opening current process token", unsafe { + GetLastError() + })); + } + let result = (|| { + let mut size = 0; + // SAFETY: querying the required buffer size permits a null buffer. + let queried = unsafe { + GetTokenInformation(token, TokenUser, std::ptr::null_mut(), 0, &mut size) + }; + if queried != 0 || unsafe { GetLastError() } != ERROR_INSUFFICIENT_BUFFER { + return Err(win_error("querying current user SID", unsafe { + GetLastError() + })); + } + if !is_bounded_token_user_info_size(size) { + return Err(std::io::Error::other( + "current user SID information has an invalid size", + )); + } + // TOKEN_USER contains pointer-sized fields, so use aligned backing + // before interpreting the initialized bytes as TOKEN_USER. + let mut buffer = vec![0_usize; (size as usize).div_ceil(size_of::())]; + // SAFETY: `buffer` is suitably aligned and has at least the size + // requested by the preceding call. + if unsafe { + GetTokenInformation( + token, + TokenUser, + buffer.as_mut_ptr().cast(), + size, + &mut size, + ) + } == 0 + { + return Err(win_error("reading current user SID", unsafe { + GetLastError() + })); + } + let user = buffer.as_ptr().cast::(); + // SAFETY: GetTokenInformation initialized a suitably aligned + // TOKEN_USER in buffer. + f(unsafe { (*user).User.Sid }) + })(); + // SAFETY: OpenProcessToken returned this handle above. + unsafe { CloseHandle(token) }; + result + } + + unsafe fn apply_current_user_dacl(handle: HANDLE, inherit: u32) -> std::io::Result<()> { + unsafe { + with_current_user_sid(|sid| { + let sid_len = GetLengthSid(sid) as usize; + if sid_len == 0 { + return Err(win_error("measuring current user SID", GetLastError())); + } + if sid_len > MAX_SID_LENGTH { + return Err(std::io::Error::other("current user SID is too large")); + } + // u32 backing gives the ACL its required alignment. + let mut acl_storage = + vec![0_u32; acl_size_for_sid(sid_len).div_ceil(size_of::())]; + let acl = acl_storage.as_mut_ptr().cast::(); + if InitializeAcl( + acl, + (acl_storage.len() * size_of::()) as u32, + ACL_REVISION, + ) == 0 + { + return Err(win_error("initializing discovery ACL", GetLastError())); + } + if AddAccessAllowedAceEx(acl, ACL_REVISION, inherit, GENERIC_ALL, sid) == 0 { + return Err(win_error( + "adding current user discovery ACL", + GetLastError(), + )); + } + let status = SetSecurityInfo( + handle, + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION + | DACL_SECURITY_INFORMATION + | PROTECTED_DACL_SECURITY_INFORMATION, + sid, + std::ptr::null_mut(), + acl, + std::ptr::null(), + ); + if status != 0 { + return Err(win_error("setting private discovery ACL", status)); + } + Ok(()) + }) + } + } + + pub(super) fn open_private_directory(path: &Path) -> std::io::Result { + let wide_path = path_as_wide_null(path); + // Do not share delete access: holding this handle prevents a checked + // directory from being renamed/replaced while paths beneath it are used. + // OPEN_REPARSE_POINT lets us inspect (rather than traverse) junctions. + let handle = unsafe { + CreateFileW( + wide_path.as_ptr(), + READ_CONTROL | WRITE_DAC | WRITE_OWNER, + FILE_SHARE_READ | FILE_SHARE_WRITE, + std::ptr::null(), + OPEN_EXISTING, + FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT, + std::ptr::null_mut(), + ) + }; + if handle == INVALID_HANDLE_VALUE { + return Err(win_error("opening discovery directory", unsafe { + GetLastError() + })); + } + let result = (|| unsafe { + let mut info = FILE_ATTRIBUTE_TAG_INFO { + FileAttributes: 0, + ReparseTag: 0, + }; + if GetFileInformationByHandleEx( + handle, + FileAttributeTagInfo, + (&mut info as *mut FILE_ATTRIBUTE_TAG_INFO).cast(), + size_of::() as u32, + ) == 0 + { + return Err(win_error("checking discovery directory", GetLastError())); + } + if !is_plain_directory(info.FileAttributes) { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "discovery directory is not a plain directory (reparse points are forbidden)", + )); + } + apply_current_user_dacl(handle, OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE) + })(); + match result { + Ok(()) => Ok(PrivateDirectory(handle)), + Err(error) => { + unsafe { CloseHandle(handle) }; + Err(error) + } + } + } + + pub(super) fn private_file_handle(handle: HANDLE) -> std::io::Result<()> { + // Temp files get an explicit protected DACL before the capability is + // written; inheritance alone would not repair an existing weak ACL. + unsafe { apply_current_user_dacl(handle, 0) } + } + + pub(super) fn create_private_file(path: &Path) -> std::io::Result { + use std::os::windows::io::{FromRawHandle, RawHandle}; + + let wide_path = path_as_wide_null(path); + // CREATE_NEW preserves the caller's collision retry semantics and + // avoids opening an attacker-provided existing object. Request every + // right SetSecurityInfo needs explicitly: WRITE_DAC replaces the DACL + // and WRITE_OWNER sets the owner. OPEN_REPARSE_POINT prevents a + // reparse point at the fresh candidate path from being traversed. + let handle = unsafe { + CreateFileW( + wide_path.as_ptr(), + GENERIC_WRITE | READ_CONTROL | WRITE_DAC | WRITE_OWNER, + FILE_SHARE_READ | FILE_SHARE_WRITE, + std::ptr::null(), + CREATE_NEW, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OPEN_REPARSE_POINT, + std::ptr::null_mut(), + ) + }; + if handle == INVALID_HANDLE_VALUE { + return Err(std::io::Error::last_os_error()); + } + // SAFETY: CreateFileW returned an owned, valid file handle above. + Ok(unsafe { std::fs::File::from_raw_handle(handle as RawHandle) }) + } + + /// Replaces an old discovery file in one filesystem operation. `rename` + /// cannot replace an existing destination on Windows, which breaks a + /// broker restart when its PID has been reused. The replacement preserves + /// the temp file's already-applied protected DACL rather than inheriting + /// permissions from the old discovery file. + pub(super) fn replace_discovery_file(tmp: &Path, path: &Path) -> std::io::Result<()> { + let wide_tmp = path_as_wide_null(tmp); + let wide_path = path_as_wide_null(path); + // MOVEFILE_WRITE_THROUGH waits for the filesystem to complete the + // replacement. The containing directory remains held privately by the + // caller, preventing it from being swapped during this operation. + if unsafe { + MoveFileExW( + wide_tmp.as_ptr(), + wide_path.as_ptr(), + MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH, + ) + } == 0 + { + return Err(win_error("replacing discovery file", unsafe { + GetLastError() + })); + } + Ok(()) + } + + #[cfg(test)] + pub(super) fn discovery_file_has_private_acl(path: &Path) -> std::io::Result { + use std::os::windows::io::AsRawHandle; + use windows_sys::Win32::{ + Foundation::LocalFree, + Security::Authorization::{GetSecurityInfo, SE_FILE_OBJECT}, + Security::{EqualSid, GetSecurityDescriptorControl, SE_DACL_PROTECTED}, + }; + + let file = std::fs::File::open(path)?; + let mut owner = std::ptr::null_mut(); + let mut dacl = std::ptr::null_mut(); + let mut descriptor = std::ptr::null_mut(); + let status = unsafe { + GetSecurityInfo( + file.as_raw_handle() as _, + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, + &mut owner, + std::ptr::null_mut(), + &mut dacl, + std::ptr::null_mut(), + &mut descriptor, + ) + }; + if status != 0 { + return Err(win_error("reading discovery file ACL", status)); + } + let result = unsafe { + with_current_user_sid(|current_user_sid| { + let mut control = 0; + let mut revision = 0; + if GetSecurityDescriptorControl(descriptor, &mut control, &mut revision) == 0 { + return Err(win_error( + "reading discovery file ACL control", + GetLastError(), + )); + } + Ok(!dacl.is_null() + && EqualSid(owner, current_user_sid) != 0 + && control & SE_DACL_PROTECTED != 0) + }) + }; + // SAFETY: GetSecurityInfo allocated this descriptor with LocalAlloc. + unsafe { LocalFree(descriptor as _) }; + result + } + + #[cfg(test)] + mod tests { + use super::*; + + #[test] + fn plain_directory_rejects_reparse_points_and_files() { + assert!(is_plain_directory(FILE_ATTRIBUTE_DIRECTORY)); + assert!(!is_plain_directory(0)); + assert!(!is_plain_directory( + FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT + )); + } + + #[test] + fn acl_storage_includes_header_ace_and_sid() { + assert_eq!(acl_size_for_sid(12), size_of::() + 8 + 12); + } + + #[test] + fn token_user_buffer_size_is_bounded() { + assert!(!is_bounded_token_user_info_size(0)); + assert!(is_bounded_token_user_info_size( + size_of::() as u32 + )); + assert!(is_bounded_token_user_info_size(MAX_TOKEN_USER_INFO_SIZE)); + assert!(!is_bounded_token_user_info_size( + MAX_TOKEN_USER_INFO_SIZE + 1 + )); + } + } +} + +#[cfg(all(feature = "server", not(windows)))] +fn private_discovery_directory(dir: &Path) -> std::io::Result<()> { + #[cfg(unix)] + { + use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; + + // Refuse to follow a symlink or repair a directory after it has been + // swapped out from under the checked path. + let handle = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW | libc::O_DIRECTORY) + .open(dir)?; + let metadata = handle.metadata()?; + // SAFETY: `geteuid` takes no arguments and has no preconditions. + let current_uid = unsafe { libc::geteuid() }; + if !metadata.file_type().is_dir() || metadata.uid() != current_uid { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + format!( + "discovery directory {} is not owned by the current user", + dir.display() + ), + )); + } + handle.set_permissions(unix_permissions(0o700))?; + Ok(()) + } + #[cfg(not(unix))] + { + let metadata = std::fs::symlink_metadata(dir)?; + if !metadata.file_type().is_dir() { + return Err(std::io::Error::other(format!( + "discovery directory {} is not a directory", + dir.display() + ))); + } + Ok(()) + } } /// Atomically write the discovery file: private dir + temp file + fsync + -/// rename, so a CLI reading mid-write never sees partial JSON. +/// rename, so a CLI reading mid-write never sees partial JSON. The capability +/// is sensitive to other users on the host, so Unix paths are tightened to +/// owner-only access even when they predate this write. #[cfg(feature = "server")] pub(crate) fn write_discovery_file( path: &Path, port: u16, pid: u32, generation: u64, + capability: &str, ) -> std::io::Result<()> { use std::io::Write; @@ -65,33 +492,109 @@ pub(crate) fn write_discovery_file( dir_builder.mode(0o700); } dir_builder.create(dir)?; + #[cfg(windows)] + let _private_dir = windows_discovery_security::open_private_directory(dir)?; + #[cfg(not(windows))] + private_discovery_directory(dir)?; let payload = serde_json::json!({ "port": port, "pid": pid, "generation": generation, "protocolVersion": PROTOCOL_VERSION, + "capability": capability, }); - let mut tmp_name = path - .file_name() - .map(std::ffi::OsStr::to_os_string) - .unwrap_or_default(); - tmp_name.push(".tmp"); - let tmp = path.with_file_name(tmp_name); + // Use a unique adjacent path for each write. A stale fixed-name temp file + // must never block broker startup, and `create_new` prevents following or + // truncating a same-user symlink planted at the candidate path. + let tmp = (0_u8..16) + .find_map(|_| { + let mut suffix = [0_u8; 16]; + if let Err(err) = getrandom::fill(&mut suffix) { + return Some(Err(std::io::Error::other(err))); + } + let mut tmp_name = path + .file_name() + .map(std::ffi::OsStr::to_os_string) + .unwrap_or_default(); + tmp_name.push(format!(".{}.tmp", hex::encode(suffix))); + let candidate = path.with_file_name(tmp_name); + + #[cfg(windows)] + let file_result = windows_discovery_security::create_private_file(&candidate); + #[cfg(not(windows))] + let file_result = { + let mut options = std::fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + options.open(&candidate) + }; + match file_result { + Ok(file) => Some(Ok((candidate, file))), + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => None, + Err(err) => Some(Err(err)), + } + }) + .transpose()? + .ok_or_else(|| std::io::Error::other("could not allocate discovery temp file"))?; + let (tmp, mut file) = tmp; + let mut renamed = false; + let result = (|| { + #[cfg(windows)] + windows_discovery_security::private_file_handle( + std::os::windows::io::AsRawHandle::as_raw_handle(&file) as _, + )?; + #[cfg(unix)] + file.set_permissions(unix_permissions(0o600))?; + file.write_all(payload.to_string().as_bytes())?; + file.sync_all()?; + drop(file); + replace_discovery_file(&tmp, path)?; + renamed = true; + sync_directory(dir) + })(); + if result.is_err() { + let cleanup_path = if renamed { path } else { &tmp }; + let _ = std::fs::remove_file(cleanup_path); + if renamed { + let _ = sync_directory(dir); + } + } + result +} + +#[cfg(all(feature = "server", windows))] +fn replace_discovery_file(tmp: &Path, path: &Path) -> std::io::Result<()> { + windows_discovery_security::replace_discovery_file(tmp, path) +} + +#[cfg(all(feature = "server", not(windows)))] +fn replace_discovery_file(tmp: &Path, path: &Path) -> std::io::Result<()> { + std::fs::rename(tmp, path) +} - let mut options = std::fs::OpenOptions::new(); - options.write(true).create(true).truncate(true); +#[cfg(feature = "server")] +fn sync_directory(dir: &Path) -> std::io::Result<()> { #[cfg(unix)] { - use std::os::unix::fs::OpenOptionsExt; - options.mode(0o600); - } - let mut file = options.open(&tmp)?; - file.write_all(payload.to_string().as_bytes())?; - file.sync_all()?; - drop(file); - std::fs::rename(&tmp, path) + std::fs::File::open(dir)?.sync_all() + } + #[cfg(not(unix))] + { + let _ = dir; + Ok(()) + } +} + +#[cfg(all(feature = "server", unix))] +fn unix_permissions(mode: u32) -> std::fs::Permissions { + use std::os::unix::fs::PermissionsExt; + std::fs::Permissions::from_mode(mode) } /// Best-effort removal (stop / app exit); missing files are expected. @@ -124,16 +627,34 @@ mod tests { #[test] fn parses_owner_pid_from_file_name() { - assert_eq!( - owner_pid_from_discovery_file_name("control-1234.json"), - Some(1234) - ); - assert_eq!( - owner_pid_from_discovery_file_name("control-1234.json.tmp"), - Some(1234) - ); - assert_eq!(owner_pid_from_discovery_file_name("other.json"), None); - assert_eq!(owner_pid_from_discovery_file_name("control-1234.tmp"), None); + const NONCE: &str = "0123456789abcdef0123456789abcdef"; + + for name in [ + "control-1234.json".to_string(), + "control-1234.json.tmp".to_string(), + format!("control-1234.json.{NONCE}.tmp"), + ] { + assert_eq!( + owner_pid_from_discovery_file_name(&name), + Some(1234), + "expected to recognize {name}" + ); + } + + for name in [ + "other.json", + "control-1234.tmp", + "control-1234.json.short.tmp", + "control-1234.json.0123456789abcdef0123456789abcdeg.tmp", + "control-1234.json.0123456789ABCDEF0123456789ABCDEF.tmp", + "control-1234.json.0123456789abcdef0123456789abcdef.tmp.extra", + ] { + assert_eq!( + owner_pid_from_discovery_file_name(name), + None, + "must not recognize unrelated name {name}" + ); + } // The parser round-trips the name `discovery_file_path` writes. let path = discovery_file_path(Path::new("/data"), 4242); @@ -155,23 +676,108 @@ mod tests { ); } + #[cfg(all(feature = "server", unix))] + #[test] + fn write_rejects_symlinked_discovery_directory() { + use std::os::unix::fs::symlink; + + let base = std::env::temp_dir().join(format!( + "berdctl-discovery-dir-symlink-test-{}", + std::process::id() + )); + std::fs::remove_dir_all(&base).ok(); + std::fs::create_dir(&base).unwrap(); + let target = base.join("target"); + let link = base.join("berdctl"); + std::fs::create_dir(&target).unwrap(); + std::fs::set_permissions(&target, unix_permissions(0o700)).unwrap(); + symlink(&target, &link).unwrap(); + let path = link.join("control-4242.json"); + + let error = write_discovery_file( + &path, + 8080, + 4242, + 7, + "1111111111111111111111111111111111111111111111111111111111111111", + ) + .expect_err("symlinked discovery directory must fail closed"); + assert!(!target.join("control-4242.json").exists()); + assert_ne!(error.kind(), std::io::ErrorKind::NotFound); + + std::fs::remove_dir_all(base).ok(); + } + + #[cfg(all(feature = "server", windows))] + #[test] + fn windows_write_and_rewrite_preserve_private_acl() { + const FIRST_CAPABILITY: &str = + "1111111111111111111111111111111111111111111111111111111111111111"; + const ROTATED_CAPABILITY: &str = + "2222222222222222222222222222222222222222222222222222222222222222"; + let base = std::env::temp_dir().join(format!( + "berdctl-discovery-acl-roundtrip-test-{}", + std::process::id() + )); + std::fs::remove_dir_all(&base).ok(); + let path = discovery_file_path(&base, 4242); + + write_discovery_file(&path, 8080, 4242, 7, FIRST_CAPABILITY).unwrap(); + assert!( + windows_discovery_security::discovery_file_has_private_acl(&path).unwrap(), + "initial discovery file must have an owner-private protected DACL" + ); + + write_discovery_file(&path, 9090, 4242, 8, ROTATED_CAPABILITY).unwrap(); + assert!( + windows_discovery_security::discovery_file_has_private_acl(&path).unwrap(), + "rewritten discovery file must retain an owner-private protected DACL" + ); + let parsed: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); + assert_eq!(parsed["generation"], 8); + assert_eq!(parsed["capability"], ROTATED_CAPABILITY); + + std::fs::remove_dir_all(&base).ok(); + } + #[cfg(feature = "server")] #[test] fn write_and_remove_lifecycle() { + const FIRST_CAPABILITY: &str = + "1111111111111111111111111111111111111111111111111111111111111111"; + const ROTATED_CAPABILITY: &str = + "2222222222222222222222222222222222222222222222222222222222222222"; let base = std::env::temp_dir().join(format!("berdctl-discovery-test-{}", std::process::id())); std::fs::remove_dir_all(&base).ok(); let path = discovery_file_path(&base, 4242); - write_discovery_file(&path, 8080, 4242, 7).unwrap(); + write_discovery_file(&path, 8080, 4242, 7, FIRST_CAPABILITY).unwrap(); let parsed: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); assert_eq!(parsed["port"], 8080); assert_eq!(parsed["pid"], 4242); assert_eq!(parsed["generation"], 7); assert_eq!(parsed["protocolVersion"], PROTOCOL_VERSION); + assert_eq!(parsed["capability"], FIRST_CAPABILITY); // The temp file is renamed away, never left behind. - assert!(!path.with_file_name("control-4242.json.tmp").exists()); + let leftovers: Vec<_> = std::fs::read_dir(path.parent().unwrap()) + .unwrap() + .flatten() + .filter(|entry| { + entry + .file_name() + .to_string_lossy() + .starts_with("control-4242.json.") + }) + .collect(); + assert!(leftovers.is_empty(), "leftover temp files: {leftovers:?}"); + + // A crash orphan at the legacy fixed temp name cannot block a future + // broker start or be overwritten with the new capability. + let legacy_tmp = path.with_file_name("control-4242.json.tmp"); + std::fs::write(&legacy_tmp, "stale").unwrap(); #[cfg(unix)] { @@ -183,14 +789,33 @@ mod tests { assert_eq!(dir_mode & 0o777, 0o700); let file_mode = std::fs::metadata(&path).unwrap().permissions().mode(); assert_eq!(file_mode & 0o777, 0o600); + + // Pre-existing permissive paths are tightened too; creation modes + // alone do not repair them. + std::fs::set_permissions(path.parent().unwrap(), unix_permissions(0o755)).unwrap(); + std::fs::set_permissions(&path, unix_permissions(0o644)).unwrap(); } - // Restart case: a rewrite replaces the content atomically. - write_discovery_file(&path, 9090, 4242, 8).unwrap(); + // Restart case: an atomic rewrite rotates both generation and secret. + write_discovery_file(&path, 9090, 4242, 8, ROTATED_CAPABILITY).unwrap(); let parsed: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); assert_eq!(parsed["port"], 9090); assert_eq!(parsed["generation"], 8); + assert_eq!(parsed["capability"], ROTATED_CAPABILITY); + assert_eq!(std::fs::read_to_string(&legacy_tmp).unwrap(), "stale"); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let dir_mode = std::fs::metadata(path.parent().unwrap()) + .unwrap() + .permissions() + .mode(); + assert_eq!(dir_mode & 0o777, 0o700); + let file_mode = std::fs::metadata(&path).unwrap().permissions().mode(); + assert_eq!(file_mode & 0o777, 0o600); + } remove_discovery_file(&path); assert!(!path.exists()); diff --git a/src-tauri/plugins/berdctl/src/lib.rs b/src-tauri/plugins/berdctl/src/lib.rs index 86d5b6f8a..a3a8f8227 100644 --- a/src-tauri/plugins/berdctl/src/lib.rs +++ b/src-tauri/plugins/berdctl/src/lib.rs @@ -2,8 +2,9 @@ //! //! A lazily started, loopback-only HTTP server (`GET /v1/ping`, `POST //! /v1/call`) that forwards commands over a request/response bridge into the -//! main-window renderer. The CLI finds it through a per-instance discovery -//! file written on start and removed on stop/exit. +//! main-window renderer. The CLI finds it through a per-instance, owner-private +//! discovery file written on start and removed on stop/exit, and presents the +//! file's fresh bearer capability on every broker request. //! //! Without the `server` feature this crate is an inert stub: build.rs still //! generates the command permissions (so capability validation passes in @@ -24,7 +25,8 @@ mod plugin { use crate::bridge::{Bridge, BridgeError, BridgeRequest, BridgeResult}; use crate::discovery; use crate::server::{ - self, BridgeDispatcher, ServerContext, ServerHandle, TimeoutStore, IN_FLIGHT_LIMIT, + self, generate_capability, BridgeDispatcher, ServerContext, ServerHandle, TimeoutStore, + IN_FLIGHT_LIMIT, }; use serde::Serialize; use std::collections::HashMap; @@ -150,6 +152,8 @@ mod plugin { return Ok(StartedEndpoint { port: handle.port }); } let generation = state.generation.fetch_add(1, Ordering::Relaxed) + 1; + let capability = generate_capability() + .map_err(|err| format!("failed to generate berdctl capability: {err}"))?; // Each server gets its own semaphore: graceful shutdown lets the // previous server's in-flight handlers outlive `stop`, and their // permits must release slots on that dead instance, not free up (and @@ -162,6 +166,7 @@ mod plugin { state.timeouts.clone(), Arc::new(tokio::sync::Semaphore::new(IN_FLIGHT_LIMIT)), generation, + capability.clone(), )); let handle = server::start_server(ctx) .await @@ -176,7 +181,8 @@ mod plugin { .map_err(|err| format!("failed to resolve app data dir: {err}"))?; let pid = std::process::id(); let path = discovery::discovery_file_path(&app_data_dir, pid); - if let Err(err) = discovery::write_discovery_file(&path, port, pid, generation) { + if let Err(err) = discovery::write_discovery_file(&path, port, pid, generation, &capability) + { handle.shutdown(); return Err(format!( "failed to write berdctl discovery file {}: {err}", diff --git a/src-tauri/plugins/berdctl/src/server.rs b/src-tauri/plugins/berdctl/src/server.rs index 5b62bc395..9e36d91bb 100644 --- a/src-tauri/plugins/berdctl/src/server.rs +++ b/src-tauri/plugins/berdctl/src/server.rs @@ -1,16 +1,16 @@ //! Loopback-only HTTP broker for the berdctl CLI. //! //! Serves `GET /v1/ping` (generation/protocol handshake) and `POST /v1/call` -//! (command dispatch over the renderer bridge). There is no application auth -//! in v1; the header rejection below (any `Origin`, any `Sec-Fetch-*`, `Host` -//! mismatch) is the sole defense against browser-JS-to-localhost and DNS -//! rebinding, so it applies to every route. +//! (command dispatch over the renderer bridge). Every route requires the +//! per-server bearer capability published in the private discovery file. The +//! existing Origin, Sec-Fetch, and literal Host checks remain a separate +//! defense against browser-JS-to-localhost and DNS rebinding. use crate::bridge::{Bridge, BridgeError, BridgeRequest, BridgeResult}; use crate::discovery::PROTOCOL_VERSION; -use axum::body::Bytes; -use axum::extract::State; -use axum::http::header::{HOST, ORIGIN}; +use axum::body::to_bytes; +use axum::extract::{Request, State}; +use axum::http::header::{AUTHORIZATION, HOST, ORIGIN}; use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; use axum::routing::{get, post}; @@ -21,6 +21,7 @@ use std::collections::HashMap; use std::future::Future; use std::sync::{Arc, OnceLock, RwLock}; use std::time::{Duration, Instant}; +use subtle::ConstantTimeEq; use tauri::{AppHandle, Runtime}; use tokio::sync::{oneshot, Semaphore}; @@ -29,6 +30,18 @@ pub const IN_FLIGHT_LIMIT: usize = 4; const DEFAULT_COMMAND_TIMEOUT: Duration = Duration::from_secs(30); const MIN_REQUEST_TIMEOUT: Duration = Duration::from_secs(1); const MAX_COMMAND_TIMEOUT: Duration = Duration::from_secs(900); +const CAPABILITY_BYTES: usize = 32; +/// Maximum accepted serialized `/v1/call` request body. The broker only +/// forwards compact command envelopes, never arbitrary payload streams. +// A 50,000-character prompt can exceed 200 KiB as UTF-8 and grow further +// through JSON escaping. Leave ample envelope headroom while retaining a hard cap. +const MAX_CALL_BODY_BYTES: usize = 512 * 1024; + +pub fn generate_capability() -> std::io::Result { + let mut bytes = [0_u8; CAPABILITY_BYTES]; + getrandom::fill(&mut bytes).map_err(std::io::Error::other)?; + Ok(hex::encode(bytes)) +} /// Resolve the bridge timeout for a call: a request `timeout_ms` wins /// (clamped to [`MIN_REQUEST_TIMEOUT`]..=[`MAX_COMMAND_TIMEOUT`]); otherwise @@ -118,6 +131,7 @@ pub struct ServerContext { // against their own instance, never the next server's. inflight: Arc, generation: u64, + capability: String, // Set by `start_server` once the listener is bound, before any request. port: OnceLock, } @@ -128,12 +142,14 @@ impl ServerContext { timeouts: Arc, inflight: Arc, generation: u64, + capability: String, ) -> Self { Self { dispatcher, timeouts, inflight, generation, + capability, port: OnceLock::new(), } } @@ -183,8 +199,9 @@ pub fn build_router(ctx: Arc>) -> Router } /// Reject requests that look like they came from a browser (any `Origin` or -/// `Sec-Fetch-*` header) or through DNS rebinding (`Host` other than our -/// loopback bind). Applied by every handler before anything else. +/// `Sec-Fetch-*` header), through DNS rebinding (`Host` other than our +/// loopback bind), or without this server instance's bearer capability. +/// Applied by every handler before reading or dispatching a body. fn forbidden_header_response(ctx: &ServerContext, headers: &HeaderMap) -> Option { let violation = if headers.contains_key(ORIGIN) { Some("Origin header not allowed".to_string()) @@ -199,13 +216,30 @@ fn forbidden_header_response(ctx: &ServerContext, headers: &HeaderMap) -> Some(host) if host == expected => None, _ => Some(format!("Host must be {expected}")), } - }; + } + .or_else(|| { + let authorized = headers + .get(AUTHORIZATION) + .and_then(|value| value.to_str().ok()) + .and_then(|value| value.strip_prefix("Bearer ")) + .is_some_and(|provided| capability_matches(&ctx.capability, provided)); + (!authorized).then(|| "valid bearer capability required".to_string()) + }); violation.map(|message| { log::warn!("[berdctl] rejected request: {message}"); error_response(StatusCode::FORBIDDEN, "forbidden", &message) }) } +fn capability_matches(expected: &str, provided: &str) -> bool { + let expected = expected.as_bytes(); + let provided = provided.as_bytes(); + if expected.len() != provided.len() { + return false; + } + bool::from(expected.ct_eq(provided)) +} + async fn handle_ping( State(ctx): State>>, headers: HeaderMap, @@ -238,12 +272,26 @@ fn empty_object() -> Value { async fn handle_call( State(ctx): State>>, headers: HeaderMap, - body: Bytes, + request: Request, ) -> Response { + // `Request` leaves its body untouched until after all header-only + // defenses pass. In particular, an unauthenticated peer cannot make us + // buffer or parse a body before receiving its 403 response. if let Some(rejection) = forbidden_header_response(&ctx, &headers) { return rejection; } + let body = match to_bytes(request.into_body(), MAX_CALL_BODY_BYTES).await { + Ok(body) => body, + Err(_) => { + return error_response( + StatusCode::PAYLOAD_TOO_LARGE, + "payload_too_large", + "request body exceeds the maximum allowed size", + ); + } + }; + let call = match serde_json::from_slice::(&body) { Ok(call) => call, Err(err) => { @@ -356,9 +404,14 @@ fn log_call(command: &str, result_code: &str, started: Instant) { mod tests { use super::*; use crate::bridge::BridgeErrorBody; + use tokio::net::TcpStream; use tokio::sync::{mpsc, Notify}; const TEST_GENERATION: u64 = 3; + const TEST_CAPABILITY: &str = + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + const STALE_CAPABILITY: &str = + "abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"; #[derive(Clone)] enum StubBehavior { @@ -451,6 +504,7 @@ mod tests { timeouts, Arc::new(Semaphore::new(limits.permits)), TEST_GENERATION, + TEST_CAPABILITY.to_string(), )); let handle = start_server(ctx).await.unwrap(); TestServer { @@ -459,13 +513,67 @@ mod tests { } } - async fn post_call(base: &str, body: &Value) -> reqwest::Response { - reqwest::Client::new() + async fn get_ping(base: &str, capability: Option<&str>) -> reqwest::Response { + let request = reqwest::Client::new().get(format!("{base}/v1/ping")); + let request = match capability { + Some(capability) => request.bearer_auth(capability), + None => request, + }; + request.send().await.unwrap() + } + + async fn post_call_with_capability( + base: &str, + body: &Value, + capability: Option<&str>, + ) -> reqwest::Response { + let request = reqwest::Client::new() .post(format!("{base}/v1/call")) - .json(body) - .send() - .await - .unwrap() + .json(body); + let request = match capability { + Some(capability) => request.bearer_auth(capability), + None => request, + }; + request.send().await.unwrap() + } + + async fn post_call(base: &str, body: &Value) -> reqwest::Response { + post_call_with_capability(base, body, Some(TEST_CAPABILITY)).await + } + + async fn write_all(stream: &TcpStream, bytes: &[u8]) { + let mut written = 0; + while written < bytes.len() { + stream.writable().await.unwrap(); + match stream.try_write(&bytes[written..]) { + Ok(count) => written += count, + Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => continue, + Err(err) => panic!("write request: {err}"), + } + } + } + + async fn read_headers(stream: &TcpStream) -> Vec { + tokio::time::timeout(Duration::from_secs(1), async { + let mut response = Vec::new(); + let mut buffer = [0; 1024]; + loop { + stream.readable().await.unwrap(); + match stream.try_read(&mut buffer) { + Ok(0) => return response, + Ok(count) => { + response.extend_from_slice(&buffer[..count]); + if response.windows(4).any(|window| window == b"\r\n\r\n") { + return response; + } + } + Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => continue, + Err(err) => panic!("read response: {err}"), + } + } + }) + .await + .expect("server must reject headers without waiting for the body") } fn call_body(command: &str, args: Value) -> Value { @@ -475,15 +583,106 @@ mod tests { #[tokio::test] async fn ping_echoes_generation_and_protocol_version() { let server = spawn_server(StubBehavior::Echo, Limits::default()).await; - let response = reqwest::get(format!("{}/v1/ping", server.base)) - .await - .unwrap(); + let response = get_ping(&server.base, Some(TEST_CAPABILITY)).await; assert_eq!(response.status(), 200); let body: Value = response.json().await.unwrap(); assert_eq!(body["generation"], TEST_GENERATION); assert_eq!(body["protocolVersion"], PROTOCOL_VERSION); } + #[tokio::test] + async fn missing_wrong_and_stale_capabilities_are_rejected_on_all_routes() { + let server = spawn_server(StubBehavior::Echo, Limits::default()).await; + let body = call_body("sessions", json!({ "action": "list" })); + + for capability in [None, Some("wrong"), Some(STALE_CAPABILITY)] { + let ping = get_ping(&server.base, capability).await; + assert_eq!(ping.status(), 403, "ping capability {capability:?}"); + let ping_body: Value = ping.json().await.unwrap(); + assert_eq!(ping_body["error"]["code"], "forbidden"); + + let call = post_call_with_capability(&server.base, &body, capability).await; + assert_eq!(call.status(), 403, "call capability {capability:?}"); + let call_body: Value = call.json().await.unwrap(); + assert_eq!(call_body["error"]["code"], "forbidden"); + } + + assert_eq!( + get_ping(&server.base, Some(TEST_CAPABILITY)).await.status(), + 200 + ); + assert_eq!( + post_call_with_capability(&server.base, &body, Some(TEST_CAPABILITY)) + .await + .status(), + 200 + ); + } + + #[tokio::test] + async fn unauthorized_call_is_rejected_before_its_body_is_consumed() { + let server = spawn_server(StubBehavior::Echo, Limits::default()).await; + let stream = TcpStream::connect(server.base.strip_prefix("http://").unwrap()) + .await + .unwrap(); + // Declare a body but deliberately do not send it. A response proves + // header authentication ran before any body collection could wait for + // these bytes. + write_all( + &stream, + format!( + "POST /v1/call HTTP/1.1\r\nHost: 127.0.0.1:{}\r\nContent-Length: {}\r\n\r\n", + server.base.rsplit(':').next().unwrap(), + MAX_CALL_BODY_BYTES + 1, + ) + .as_bytes(), + ) + .await; + + let response = String::from_utf8(read_headers(&stream).await).unwrap(); + assert!(response.starts_with("HTTP/1.1 403"), "{response}"); + } + + #[tokio::test] + async fn call_body_over_explicit_limit_is_413() { + let server = spawn_server(StubBehavior::Echo, Limits::default()).await; + let body = format!( + r#"{{"command":"x","args":{{"padding":"{}"}}}}"#, + "x".repeat(MAX_CALL_BODY_BYTES), + ); + let response = reqwest::Client::new() + .post(format!("{}/v1/call", server.base)) + .bearer_auth(TEST_CAPABILITY) + .header("Content-Type", "application/json") + .body(body) + .send() + .await + .unwrap(); + assert_eq!(response.status(), 413); + let body: Value = response.json().await.unwrap(); + assert_eq!(body["ok"], false); + assert_eq!(body["error"]["code"], "payload_too_large"); + } + + #[test] + fn generated_capabilities_are_random_256_bit_hex() { + let first = generate_capability().unwrap(); + let second = generate_capability().unwrap(); + assert_eq!(first.len(), CAPABILITY_BYTES * 2); + assert!(first + .bytes() + .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte))); + assert_ne!(first, second); + } + + #[test] + fn capability_match_checks_content_and_length() { + assert!(capability_matches(TEST_CAPABILITY, TEST_CAPABILITY)); + assert!(!capability_matches(TEST_CAPABILITY, STALE_CAPABILITY)); + assert!(!capability_matches(TEST_CAPABILITY, "short")); + assert!(!capability_matches(TEST_CAPABILITY, &"0".repeat(128))); + } + #[tokio::test] async fn origin_header_is_rejected_on_all_routes() { let server = spawn_server(StubBehavior::Echo, Limits::default()).await; @@ -491,6 +690,7 @@ mod tests { let ping = client .get(format!("{}/v1/ping", server.base)) + .bearer_auth(TEST_CAPABILITY) .header("Origin", "https://evil.example") .send() .await @@ -502,6 +702,7 @@ mod tests { let call = client .post(format!("{}/v1/call", server.base)) + .bearer_auth(TEST_CAPABILITY) .header("Origin", "http://localhost:3000") .json(&call_body("sessions", json!({ "action": "list" }))) .send() @@ -519,6 +720,7 @@ mod tests { for header in ["Sec-Fetch-Site", "Sec-Fetch-Mode", "Sec-Fetch-Dest"] { let response = client .post(format!("{}/v1/call", server.base)) + .bearer_auth(TEST_CAPABILITY) .header(header, "cross-site") .json(&call_body("sessions", json!({ "action": "list" }))) .send() @@ -539,6 +741,7 @@ mod tests { for host in ["evil.example:1234", "localhost:80"] { let response = client .get(format!("{}/v1/ping", server.base)) + .bearer_auth(TEST_CAPABILITY) .header("Host", host) .send() .await @@ -626,6 +829,7 @@ mod tests { // Not JSON at all. let response = client .post(format!("{}/v1/call", server.base)) + .bearer_auth(TEST_CAPABILITY) .header("Content-Type", "application/json") .body("{not json") .send() diff --git a/src-tauri/src/services/berdctl_discovery.rs b/src-tauri/src/services/berdctl_discovery.rs index 1659caf36..28e143aaa 100644 --- a/src-tauri/src/services/berdctl_discovery.rs +++ b/src-tauri/src/services/berdctl_discovery.rs @@ -3,7 +3,8 @@ //! Each app instance's berdctl broker writes a discovery file at //! `/berdctl/control-.json` and deletes it on //! stop/exit. A crashed instance leaves its file behind (possibly as a -//! `control-.json.tmp` orphan from a crash mid-write); this sweep +//! legacy `control-.json.tmp` or current +//! `control-.json..tmp` orphan from a crash mid-write); this sweep //! removes files whose owning app process is no longer alive. The directory //! and filename formats are owned by the plugin's discovery module. Compiled //! unconditionally — stale files must be cleaned even by builds where the @@ -96,6 +97,10 @@ mod tests { let dead = write_discovery_file(app_data_dir.path(), &format!("control-{gone}.json")); let dead_tmp = write_discovery_file(app_data_dir.path(), &format!("control-{gone}.json.tmp")); + let dead_random_tmp = write_discovery_file( + app_data_dir.path(), + &format!("control-{gone}.json.0123456789abcdef0123456789abcdef.tmp"), + ); let own = write_discovery_file( app_data_dir.path(), &format!("control-{}.json", std::process::id()), @@ -108,6 +113,7 @@ mod tests { assert!(!dead.exists()); assert!(!dead_tmp.exists()); + assert!(!dead_random_tmp.exists()); assert!(own.exists()); assert!(live.exists()); assert!(unrelated.exists()); diff --git a/src/features/berdctl/commands/contract.ts b/src/features/berdctl/commands/contract.ts index 92dad983b..85aefc243 100644 --- a/src/features/berdctl/commands/contract.ts +++ b/src/features/berdctl/commands/contract.ts @@ -29,7 +29,7 @@ import type { AppCommand, ToolGroup } from "./types"; * Mirror of `PROTOCOL_VERSION` in both discovery.rs copies (a berdctl * crate test pins the CLI copy, and a plugin crate test pins the broker * copy); bump all copies together. */ -const WIRE_PROTOCOL_VERSION = 4; +const WIRE_PROTOCOL_VERSION = 5; type FieldSpec = { /** snake_case wire field name. */