From 5e6c5c87795a01b22f520dfb0435377fc83885c2 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Wed, 20 Nov 2024 01:38:21 -0800 Subject: [PATCH] [PM-14993] Add ssh-agent error handling and security fixes (#12048) * Add error handling and security fixes * Add is running status, and add more errors on windows --- .../desktop_native/core/src/ssh_agent/mod.rs | 47 +++++++++++++++---- .../ssh_agent/named_pipe_listener_stream.rs | 28 ++++++++--- .../desktop_native/core/src/ssh_agent/unix.rs | 35 ++++++++++++-- .../core/src/ssh_agent/windows.rs | 4 ++ apps/desktop/desktop_native/napi/index.d.ts | 1 + apps/desktop/desktop_native/napi/src/lib.rs | 6 +++ .../platform/main/main-ssh-agent.service.ts | 4 +- 7 files changed, 104 insertions(+), 21 deletions(-) diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs index ad0ac837af..8cbcd97d91 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs @@ -20,18 +20,16 @@ pub struct BitwardenDesktopAgent { show_ui_request_tx: tokio::sync::mpsc::Sender<(u32, String)>, get_ui_response_rx: Arc>>, request_id: Arc>, -} - -impl BitwardenDesktopAgent { - async fn get_request_id(&self) -> u32 { - let mut request_id = self.request_id.lock().await; - *request_id += 1; - *request_id - } + is_running: Arc>, } impl ssh_agent::Agent for BitwardenDesktopAgent { async fn confirm(&self, ssh_key: Key) -> bool { + if !*self.is_running.lock().await { + println!("[BitwardenDesktopAgent] Agent is not running, but tried to call confirm"); + return false; + } + let request_id = self.get_request_id().await; let mut rx_channel = self.get_ui_response_rx.lock().await.resubscribe(); @@ -50,6 +48,12 @@ impl ssh_agent::Agent for BitwardenDesktopAgent { impl BitwardenDesktopAgent { pub fn stop(&self) { + if !*self.is_running.blocking_lock() { + println!("[BitwardenDesktopAgent] Tried to stop agent while it is not running"); + return; + } + + *self.is_running.blocking_lock() = false; self.cancellation_token.cancel(); self.keystore .0 @@ -62,6 +66,12 @@ impl BitwardenDesktopAgent { &mut self, new_keys: Vec<(String, String, String)>, ) -> Result<(), anyhow::Error> { + if !*self.is_running.blocking_lock() { + return Err(anyhow::anyhow!( + "[BitwardenDesktopAgent] Tried to set keys while agent is not running" + )); + } + let keystore = &mut self.keystore; keystore.0.write().expect("RwLock is not poisoned").clear(); @@ -91,6 +101,12 @@ impl BitwardenDesktopAgent { } pub fn lock(&mut self) -> Result<(), anyhow::Error> { + if !*self.is_running.blocking_lock() { + return Err(anyhow::anyhow!( + "[BitwardenDesktopAgent] Tried to lock agent, but it is not running" + )); + } + let keystore = &mut self.keystore; keystore .0 @@ -102,6 +118,21 @@ impl BitwardenDesktopAgent { }); Ok(()) } + + async fn get_request_id(&self) -> u32 { + if !*self.is_running.lock().await { + println!("[BitwardenDesktopAgent] Agent is not running, but tried to get request id"); + return 0; + } + + let mut request_id = self.request_id.lock().await; + *request_id += 1; + *request_id + } + + pub fn is_running(self) -> bool { + return self.is_running.blocking_lock().clone(); + } } fn parse_key_safe(pem: &str) -> Result { diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/named_pipe_listener_stream.rs b/apps/desktop/desktop_native/core/src/ssh_agent/named_pipe_listener_stream.rs index 69399ae753..49c3aa8061 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/named_pipe_listener_stream.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/named_pipe_listener_stream.rs @@ -1,7 +1,5 @@ use std::{ - io, - pin::Pin, - task::{Context, Poll}, + io, pin::Pin, sync::Arc, task::{Context, Poll} }; use futures::Stream; @@ -19,14 +17,23 @@ pub struct NamedPipeServerStream { } impl NamedPipeServerStream { - pub fn new(cancellation_token: CancellationToken) -> Self { + pub fn new(cancellation_token: CancellationToken, is_running: Arc>) -> Self { let (tx, rx) = tokio::sync::mpsc::channel(16); tokio::spawn(async move { println!( "[SSH Agent Native Module] Creating named pipe server on {}", PIPE_NAME ); - let mut listener = ServerOptions::new().create(PIPE_NAME).unwrap(); + let mut listener = match ServerOptions::new().create(PIPE_NAME) { + Ok(pipe) => pipe, + Err(err) => { + println!("[SSH Agent Native Module] Encountered an error creating the first pipe. The system's openssh service must likely be disabled"); + println!("[SSH Agent Natvie Module] error: {}", err); + cancellation_token.cancel(); + *is_running.lock().await = false; + return; + } + }; loop { println!("[SSH Agent Native Module] Waiting for connection"); select! { @@ -37,7 +44,16 @@ impl NamedPipeServerStream { _ = listener.connect() => { println!("[SSH Agent Native Module] Incoming connection"); tx.send(listener).await.unwrap(); - listener = ServerOptions::new().create(PIPE_NAME).unwrap(); + + listener = match ServerOptions::new().create(PIPE_NAME) { + Ok(pipe) => pipe, + Err(err) => { + println!("[SSH Agent Native Module] Encountered an error creating a new pipe {}", err); + cancellation_token.cancel(); + *is_running.lock().await = false; + return; + } + }; } } } diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs b/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs index c1a3950666..27f27998ce 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs @@ -1,5 +1,7 @@ use std::{ collections::HashMap, + fs, + os::unix::fs::PermissionsExt, sync::{Arc, RwLock}, }; @@ -15,14 +17,13 @@ impl BitwardenDesktopAgent { auth_request_tx: tokio::sync::mpsc::Sender<(u32, String)>, auth_response_rx: Arc>>, ) -> Result { - use std::path::PathBuf; - let agent = BitwardenDesktopAgent { keystore: ssh_agent::KeyStore(Arc::new(RwLock::new(HashMap::new()))), cancellation_token: CancellationToken::new(), show_ui_request_tx: auth_request_tx, get_ui_response_rx: auth_response_rx, request_id: Arc::new(tokio::sync::Mutex::new(0)), + is_running: Arc::new(tokio::sync::Mutex::new(false)), }; let cloned_agent_state = agent.clone(); tokio::spawn(async move { @@ -33,7 +34,12 @@ impl BitwardenDesktopAgent { let ssh_agent_directory = match my_home() { Ok(Some(home)) => home, - _ => PathBuf::from("/tmp/"), + _ => { + println!( + "[SSH Agent Native Module] Could not determine home directory" + ); + return; + } }; ssh_agent_directory .join(".bitwarden-ssh-agent.sock") @@ -48,19 +54,38 @@ impl BitwardenDesktopAgent { ssh_path ); let sockname = std::path::Path::new(&ssh_path); - let _ = std::fs::remove_file(sockname); + if let Err(e) = std::fs::remove_file(sockname) { + println!( + "[SSH Agent Native Module] Could not remove existing socket file: {}", + e + ); + return; + } + match UnixListener::bind(sockname) { Ok(listener) => { + // Only the current user should be able to access the socket + if let Err(e) = fs::set_permissions(sockname, fs::Permissions::from_mode(0o600)) + { + println!( + "[SSH Agent Native Module] Could not set socket permissions: {}", + e + ); + return; + } + let wrapper = tokio_stream::wrappers::UnixListenerStream::new(listener); let cloned_keystore = cloned_agent_state.keystore.clone(); let cloned_cancellation_token = cloned_agent_state.cancellation_token.clone(); + *cloned_agent_state.is_running.lock().await = true; let _ = ssh_agent::serve( wrapper, - cloned_agent_state, + cloned_agent_state.clone(), cloned_keystore, cloned_cancellation_token, ) .await; + *cloned_agent_state.is_running.lock().await = false; println!("[SSH Agent Native Module] SSH Agent server exited"); } Err(e) => { diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs b/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs index fd6d9dacb9..9ccae7af67 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs @@ -21,13 +21,16 @@ impl BitwardenDesktopAgent { get_ui_response_rx: auth_response_rx, cancellation_token: CancellationToken::new(), request_id: Arc::new(tokio::sync::Mutex::new(0)), + is_running: Arc::new(tokio::sync::Mutex::new(true)), }; let stream = named_pipe_listener_stream::NamedPipeServerStream::new( agent_state.cancellation_token.clone(), + agent_state.is_running.clone(), ); let cloned_agent_state = agent_state.clone(); tokio::spawn(async move { + *cloned_agent_state.is_running.lock().await = true; let _ = ssh_agent::serve( stream, cloned_agent_state.clone(), @@ -35,6 +38,7 @@ impl BitwardenDesktopAgent { cloned_agent_state.cancellation_token.clone(), ) .await; + *cloned_agent_state.is_running.lock().await = false; }); Ok(agent_state) } diff --git a/apps/desktop/desktop_native/napi/index.d.ts b/apps/desktop/desktop_native/napi/index.d.ts index 08f9f45b1b..62e448d480 100644 --- a/apps/desktop/desktop_native/napi/index.d.ts +++ b/apps/desktop/desktop_native/napi/index.d.ts @@ -71,6 +71,7 @@ export declare namespace sshagent { } export function serve(callback: (err: Error | null, arg: string) => any): Promise export function stop(agentState: SshAgentState): void + export function isRunning(agentState: SshAgentState): boolean export function setKeys(agentState: SshAgentState, newKeys: Array): void export function lock(agentState: SshAgentState): void export function importKey(encodedKey: string, password: string): SshKeyImportResult diff --git a/apps/desktop/desktop_native/napi/src/lib.rs b/apps/desktop/desktop_native/napi/src/lib.rs index 841d2155f1..8e156eb3ef 100644 --- a/apps/desktop/desktop_native/napi/src/lib.rs +++ b/apps/desktop/desktop_native/napi/src/lib.rs @@ -307,6 +307,12 @@ pub mod sshagent { Ok(()) } + #[napi] + pub fn is_running(agent_state: &mut SshAgentState) -> bool { + let bitwarden_agent_state = agent_state.state.clone(); + bitwarden_agent_state.is_running() + } + #[napi] pub fn set_keys( agent_state: &mut SshAgentState, diff --git a/apps/desktop/src/platform/main/main-ssh-agent.service.ts b/apps/desktop/src/platform/main/main-ssh-agent.service.ts index c8227e5b6a..60487aae4d 100644 --- a/apps/desktop/src/platform/main/main-ssh-agent.service.ts +++ b/apps/desktop/src/platform/main/main-ssh-agent.service.ts @@ -79,7 +79,7 @@ export class MainSshAgentService { ipcMain.handle( "sshagent.setkeys", async (event: any, keys: { name: string; privateKey: string; cipherId: string }[]) => { - if (this.agentState != null) { + if (this.agentState != null && (await sshagent.isRunning(this.agentState))) { sshagent.setKeys(this.agentState, keys); } }, @@ -107,7 +107,7 @@ export class MainSshAgentService { ); ipcMain.handle("sshagent.lock", async (event: any) => { - if (this.agentState != null) { + if (this.agentState != null && (await sshagent.isRunning(this.agentState))) { sshagent.lock(this.agentState); } });