[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

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<Mutex<tokio::sync::broadcast::Receiver<(u32, bool)>>>,
     request_id: Arc<Mutex<u32>>,
-}
-
-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<tokio::sync::Mutex<bool>>,
 }
 
 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<ssh_key::private::PrivateKey, anyhow::Error> {

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<tokio::sync::Mutex<bool>>) -> 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;
+                            }
+                        };
                     }
                 }
             }

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<Mutex<tokio::sync::broadcast::Receiver<(u32, bool)>>>,
     ) -> Result<Self, anyhow::Error> {
-        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) => {

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)
     }

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<SshAgentState>
   export function stop(agentState: SshAgentState): void
+  export function isRunning(agentState: SshAgentState): boolean
   export function setKeys(agentState: SshAgentState, newKeys: Array<PrivateKey>): void
   export function lock(agentState: SshAgentState): void
   export function importKey(encodedKey: string, password: string): SshKeyImportResult

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,

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);
       }
     });