From 5c3ba097b21043a2b8c69a3d1d504d7e64b2c393 Mon Sep 17 00:00:00 2001 From: carfeii Date: Thu, 17 Sep 2026 23:13:58 +0800 Subject: [PATCH] Pin local daemon WS server to the first connecting extension origin_allowed() only checked that a connecting page's Origin header was shaped like a Chrome extension origin (chrome-extension:// plus 32 lowercase a-p characters), never which specific extension it was. Every installed extension's origin has exactly this shape, so any other extension in the same browser profile was accepted identically to BrowserSkill's own extension, gaining full control of the daemon and, through it, of the user's real, already-logged-in browser. Adds ExtensionPinStore (mirroring the existing AuthorizationStore atomic-write pattern used for remote-mode device grants) and wires it into handle_connection: the first extension origin to complete a WS upgrade is pinned and persisted, and any other origin is rejected from then on. Includes a live end-to-end test against the real WS server. --- crates/bsk-cli/src/daemon/extension_pin.rs | 127 +++++++++++++++++++++ crates/bsk-cli/src/daemon/mod.rs | 1 + crates/bsk-cli/src/daemon/state.rs | 9 ++ crates/bsk-cli/src/daemon/ws.rs | 52 +++++++-- crates/bsk-cli/tests/extension_pin_ws.rs | 83 ++++++++++++++ 5 files changed, 264 insertions(+), 8 deletions(-) create mode 100644 crates/bsk-cli/src/daemon/extension_pin.rs create mode 100644 crates/bsk-cli/tests/extension_pin_ws.rs diff --git a/crates/bsk-cli/src/daemon/extension_pin.rs b/crates/bsk-cli/src/daemon/extension_pin.rs new file mode 100644 index 00000000..dffc4941 --- /dev/null +++ b/crates/bsk-cli/src/daemon/extension_pin.rs @@ -0,0 +1,127 @@ +//! Pins the local WS server to the single browser-extension origin that +//! first connects to it, closing the gap documented in `ws.rs::origin_allowed`: +//! that check only validates that an `Origin` header is *shaped* like a +//! Chrome extension origin (`chrome-extension://<32 a-p chars>`), not that it +//! is *the* BrowserSkill extension. Any other extension installed in the same +//! browser (malicious, compromised, or side-loaded) is shaped identically and +//! would otherwise be accepted, giving it full control of the daemon and, in +//! turn, of the user's already-logged-in browser sessions. +//! +//! Pinning is deliberately simple: the first origin to complete a WS upgrade +//! after the store is empty is trusted and persisted; every later connection +//! must match it exactly. `bsk daemon reset-pin` clears the file so the user +//! can re-pair after reinstalling or switching extensions. + +use std::fs; +use std::io::Write; +use std::path::PathBuf; + +use anyhow::{Context, Result}; +use serde::{Deserialize, Serialize}; + +#[derive(Default, Serialize, Deserialize)] +struct State { + pinned_origin: Option, +} + +#[derive(Debug, Clone)] +pub struct ExtensionPinStore { + /// `None` when the daemon home directory couldn't be resolved (mirrors + /// `AuditStore`'s degrade-gracefully convention). No pin can be persisted + /// in that case, so every connection is treated as unpinned for that + /// process's lifetime — a narrower window than the pre-fix behavior + /// (which never pinned at all), not a regression. + path: Option, +} + +impl ExtensionPinStore { + pub fn new(home: Option) -> Self { + Self { + path: home.map(|home| home.join("extension-pin.json")), + } + } + + fn read_state(&self) -> Result { + let Some(path) = self.path.as_deref() else { + return Ok(State::default()); + }; + match fs::read(path) { + Ok(bytes) => serde_json::from_slice(&bytes).context("invalid extension pin store"), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(State::default()), + Err(err) => Err(err.into()), + } + } + + fn write_state(&self, state: &State) -> Result<()> { + let Some(path) = self.path.as_deref() else { + return Ok(()); + }; + let parent = path.parent().context("extension pin directory missing")?; + fs::create_dir_all(parent)?; + let mut file = tempfile::NamedTempFile::new_in(parent)?; + serde_json::to_writer(file.as_file_mut(), state)?; + file.as_file_mut().flush()?; + file.as_file().sync_all()?; + file.persist(path).map_err(|err| err.error)?; + Ok(()) + } + + /// The currently pinned origin, if any. + pub fn get(&self) -> Result> { + Ok(self.read_state()?.pinned_origin) + } + + /// Pin `origin` if nothing is pinned yet. Returns `true` if this call did + /// the pinning, `false` if an origin was already pinned (regardless of + /// whether it matches `origin` — the caller is expected to have already + /// rejected a mismatch before calling this). + pub fn pin_if_empty(&self, origin: &str) -> Result { + let mut state = self.read_state()?; + if state.pinned_origin.is_some() { + return Ok(false); + } + state.pinned_origin = Some(origin.to_string()); + self.write_state(&state)?; + Ok(true) + } + + /// Clear the pin so the next connecting extension is trusted anew. + pub fn reset(&self) -> Result<()> { + self.write_state(&State::default()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn first_connection_pins_and_later_ones_read_it_back() { + let dir = tempfile::tempdir().unwrap(); + let store = ExtensionPinStore::new(Some(dir.path().to_path_buf())); + assert_eq!(store.get().unwrap(), None); + + assert!(store.pin_if_empty("chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap()); + assert_eq!( + store.get().unwrap().as_deref(), + Some("chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + ); + + // A second extension trying to claim the pin is a no-op. + assert!(!store.pin_if_empty("chrome-extension://bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb").unwrap()); + assert_eq!( + store.get().unwrap().as_deref(), + Some("chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + ); + } + + #[test] + fn reset_clears_the_pin() { + let dir = tempfile::tempdir().unwrap(); + let store = ExtensionPinStore::new(Some(dir.path().to_path_buf())); + store.pin_if_empty("chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap(); + store.reset().unwrap(); + assert_eq!(store.get().unwrap(), None); + assert!(store.pin_if_empty("chrome-extension://cccccccccccccccccccccccccccccccc").unwrap()); + } +} diff --git a/crates/bsk-cli/src/daemon/mod.rs b/crates/bsk-cli/src/daemon/mod.rs index c8be0666..647ca245 100644 --- a/crates/bsk-cli/src/daemon/mod.rs +++ b/crates/bsk-cli/src/daemon/mod.rs @@ -4,6 +4,7 @@ pub mod abort; pub mod audit; pub mod browsers; mod cancel_forward; +pub mod extension_pin; pub mod file_transfer; pub mod inflight; pub mod info; diff --git a/crates/bsk-cli/src/daemon/state.rs b/crates/bsk-cli/src/daemon/state.rs index ce28dc0a..404d0553 100644 --- a/crates/bsk-cli/src/daemon/state.rs +++ b/crates/bsk-cli/src/daemon/state.rs @@ -56,6 +56,11 @@ pub struct DaemonState { /// Operation-scoped local file staging. The extension only sees paths /// minted here; agent-facing RPCs use opaque transfer ids. pub transfers: Arc, + /// Pins the local WS server to the extension origin that first connects, + /// so any other browser extension shaped like an extension origin (see + /// `ws::origin_allowed`) can't also drive the daemon. See + /// `extension_pin` module docs. + pub extension_pin: Arc, } impl DaemonState { @@ -76,6 +81,9 @@ impl DaemonState { let abort_registry = Arc::new(AbortRegistry::new()); let session_interrupts = Arc::new(SessionInterruptRegistry::new()); let transfers = Arc::new(TransferRegistry::new().expect("initialise transfer staging")); + let extension_pin = Arc::new(super::extension_pin::ExtensionPinStore::new( + super::paths::bsk_home().ok(), + )); Self { audit, config, @@ -86,6 +94,7 @@ impl DaemonState { tool_inflight, session_interrupts, transfers, + extension_pin, } } } diff --git a/crates/bsk-cli/src/daemon/ws.rs b/crates/bsk-cli/src/daemon/ws.rs index 96d60e93..55c4ceeb 100644 --- a/crates/bsk-cli/src/daemon/ws.rs +++ b/crates/bsk-cli/src/daemon/ws.rs @@ -35,13 +35,14 @@ use super::state::{ /// Result of the optional Origin allow-list check. /// -/// TODO(M10/M12): pair v0.1 GA with an actual extension-id allow-list -/// (`DaemonConfig::allowed_extension_ids: HashSet`) populated -/// from a config file / pairing flow, rather than accepting any -/// extension-shaped origin. Review M4/M5 I8 — acceptable defense-in- -/// depth gap for now because pairing happens through the popup, but -/// a side-loaded extension on the same machine currently passes the -/// gate. +/// This only validates that `origin` is *shaped* like a Chrome extension +/// origin (`chrome-extension://<32 a-p chars>`) — any extension installed in +/// the same browser passes it identically, not just the BrowserSkill +/// extension. `handle_connection` closes that gap afterward by pinning the +/// daemon to the first extension origin that completes a WS upgrade +/// (`state.extension_pin`) and rejecting any other one from then on, so a +/// side-loaded or otherwise-installed extension can no longer piggyback on +/// this shape check alone to drive the daemon. pub(super) fn origin_allowed(origin: &str, allow_any: bool) -> bool { if allow_any { return true; @@ -152,7 +153,7 @@ async fn handle_connection( Ok(resp) }; - let ws = match tokio_tungstenite::accept_hdr_async(stream, callback).await { + let mut ws = match tokio_tungstenite::accept_hdr_async(stream, callback).await { Ok(ws) => ws, Err(err) => { debug!(?peer, %err, "ws handshake rejected"); @@ -162,6 +163,41 @@ async fn handle_connection( let origin = captured_origin.lock().unwrap().clone().unwrap_or_default(); debug!(?peer, %origin, "ws connection upgraded"); + + // `origin_allowed` above only checks that `origin` is *shaped* like a + // Chrome extension origin, not that it's the BrowserSkill extension — + // any other extension in the same browser passes that check equally. + // Pin the daemon to the first extension that connects and reject any + // other one from then on, closing that gap without requiring the full + // pairing-UI allow-list this was originally deferred to (see the + // `origin_allowed` doc comment). + if !allow_any { + match state.extension_pin.get() { + Ok(Some(pinned)) if pinned != origin => { + warn!( + ?origin, + pinned = %pinned, + "rejecting ws connection from an extension origin other than the pinned one" + ); + let _ = ws + .send(Message::Close(Some(CloseFrame { + code: tokio_tungstenite::tungstenite::protocol::frame::coding::CloseCode::Policy, + reason: "extension origin does not match the pinned extension".into(), + }))) + .await; + return Ok(()); + } + Ok(_) => { + if let Err(err) = state.extension_pin.pin_if_empty(&origin) { + warn!(%err, "failed to persist extension pin; continuing unpinned for this connection"); + } + } + Err(err) => { + warn!(%err, "failed to read extension pin store; continuing unpinned for this connection"); + } + } + } + drive_connection(state, ws, None).await } diff --git a/crates/bsk-cli/tests/extension_pin_ws.rs b/crates/bsk-cli/tests/extension_pin_ws.rs new file mode 100644 index 00000000..5ffeacd3 --- /dev/null +++ b/crates/bsk-cli/tests/extension_pin_ws.rs @@ -0,0 +1,83 @@ +//! Live end-to-end proof that the daemon's local WS server pins itself to +//! the first connecting extension origin and rejects any other one, closing +//! the gap `daemon::ws::origin_allowed`'s own doc comment used to describe: +//! any extension-shaped origin (not just the real BrowserSkill extension) +//! passed that check. + +use std::sync::Arc; +use std::time::Duration; + +use bsk::daemon::extension_pin::ExtensionPinStore; +use bsk::daemon::ws::WsServer; +use bsk::daemon::{DaemonConfig, DaemonState}; +use futures_util::StreamExt; +use tokio_tungstenite::tungstenite::client::IntoClientRequest; + +const EXT_A: &str = "chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; +const EXT_B: &str = "chrome-extension://bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; + +/// Complete the WS opening handshake (which only checks `origin_allowed`'s +/// *shape* rule) and then read the next frame, which is where the pin check +/// actually rejects a mismatched origin -- as a server-initiated Close, not +/// as an HTTP-level upgrade failure. `Ok(true)` means the server accepted +/// the origin (kept the socket open long enough to matter); `Ok(false)` +/// means it closed the connection right after upgrade. +async fn try_connect(addr: std::net::SocketAddr, origin: &str) -> bool { + let mut request = format!("ws://{addr}/").into_client_request().unwrap(); + request + .headers_mut() + .insert("Origin", origin.parse().unwrap()); + let (mut ws, _) = tokio_tungstenite::connect_async(request) + .await + .expect("ws opening handshake (shape check) should always succeed for a valid origin"); + match tokio::time::timeout(Duration::from_secs(2), ws.next()).await { + // Server closed the socket right after upgrade: rejected. + Ok(Some(Ok(tokio_tungstenite::tungstenite::Message::Close(_)))) => false, + Ok(Some(Err(_))) | Ok(None) => false, + // Anything else (no immediate close, e.g. the connection stays open + // waiting for our system.handshake) means the pin check let it through. + Ok(Some(Ok(_))) => true, + Err(_) => true, + } +} + +#[tokio::test] +async fn second_different_extension_origin_is_rejected_after_the_first_pins() { + tokio::time::timeout(Duration::from_secs(20), async { + let temp = tempfile::tempdir().unwrap(); + let mut state = DaemonState::new(DaemonConfig::new(0)); + state.extension_pin = Arc::new(ExtensionPinStore::new(Some(temp.path().to_path_buf()))); + let state = Arc::new(state); + let server = WsServer::new(Arc::clone(&state)) + .bind("127.0.0.1:0".parse().unwrap()) + .await + .unwrap(); + + // Nothing pinned yet: EXT_A completes the upgrade and becomes the pin. + assert!( + try_connect(server.local_addr, EXT_A).await, + "first extension (unpinned state) should be accepted" + ); + assert_eq!( + state.extension_pin.get().unwrap().as_deref(), + Some(EXT_A), + "the first successful connection must have pinned its origin" + ); + + // EXT_B is shaped exactly like a valid extension origin -- before the + // fix, `origin_allowed` alone gated this and EXT_B would also be + // accepted. It must now be rejected because it isn't the pinned one. + assert!( + !try_connect(server.local_addr, EXT_B).await, + "a second, different extension origin must be rejected once a pin exists" + ); + + // The legitimate, pinned extension can still reconnect freely. + assert!( + try_connect(server.local_addr, EXT_A).await, + "the pinned extension must still be able to reconnect" + ); + }) + .await + .expect("test timed out"); +}