windows: Encrypt SSH passwords stored in memory (#38427)

Release Notes:

- N/A

---------

Co-authored-by: Julia <julia@zed.dev>

Cargo.lock

@@ -813,11 +813,13 @@ dependencies = [
  "futures 0.3.31",
  "gpui",
  "net",
- "parking_lot",
+ "proto",
  "smol",
  "tempfile",
  "util",
+ "windows 0.61.1",
  "workspace-hack",
+ "zeroize",
 ]
 
 [[package]]
@@ -6946,6 +6948,7 @@ dependencies = [
  "workspace",
  "workspace-hack",
  "zed_actions",
+ "zeroize",
  "zlog",
 ]
 
@@ -13141,6 +13144,7 @@ dependencies = [
  "which 6.0.3",
  "workspace-hack",
  "worktree",
+ "zeroize",
  "zlog",
 ]
 
@@ -13839,6 +13843,7 @@ name = "recent_projects"
 version = "0.1.0"
 dependencies = [
  "anyhow",
+ "askpass",
  "auto_update",
  "dap",
  "editor",

Cargo.toml

@@ -717,6 +717,7 @@ windows-core = "0.61"
 wit-component = "0.221"
 workspace-hack = "0.1.0"
 yawc = "0.2.5"
+zeroize = "1.8"
 zstd = "0.11"
 
 [workspace.dependencies.windows]
@@ -743,6 +744,7 @@ features = [
     "Win32_Networking_WinSock",
     "Win32_Security",
     "Win32_Security_Credentials",
+    "Win32_Security_Cryptography",
     "Win32_Storage_FileSystem",
     "Win32_System_Com",
     "Win32_System_Com_StructuredStorage",

crates/askpass/Cargo.toml

@@ -16,8 +16,12 @@ anyhow.workspace = true
 futures.workspace = true
 gpui.workspace = true
 net.workspace = true
-parking_lot.workspace = true
+proto.workspace = true
 smol.workspace = true
 tempfile.workspace = true
 util.workspace = true
 workspace-hack.workspace = true
+zeroize.workspace = true
+
+[target.'cfg(target_os = "windows")'.dependencies]
+windows.workspace = true

crates/askpass/src/askpass.rs

@@ -1,3 +1,9 @@
+mod encrypted_password;
+
+pub use encrypted_password::{EncryptedPassword, ProcessExt};
+
+#[cfg(target_os = "windows")]
+use std::sync::OnceLock;
 use std::{ffi::OsStr, time::Duration};
 
 use anyhow::{Context as _, Result};
@@ -10,6 +16,8 @@ use gpui::{AsyncApp, BackgroundExecutor, Task};
 use smol::fs;
 use util::ResultExt as _;
 
+use crate::encrypted_password::decrypt;
+
 #[derive(PartialEq, Eq)]
 pub enum AskPassResult {
     CancelledByUser,
@@ -17,16 +25,19 @@ pub enum AskPassResult {
 }
 
 pub struct AskPassDelegate {
-    tx: mpsc::UnboundedSender<(String, oneshot::Sender<String>)>,
+    tx: mpsc::UnboundedSender<(String, oneshot::Sender<EncryptedPassword>)>,
     _task: Task<()>,
 }
 
 impl AskPassDelegate {
     pub fn new(
         cx: &mut AsyncApp,
-        password_prompt: impl Fn(String, oneshot::Sender<String>, &mut AsyncApp) + Send + Sync + 'static,
+        password_prompt: impl Fn(String, oneshot::Sender<EncryptedPassword>, &mut AsyncApp)
+        + Send
+        + Sync
+        + 'static,
     ) -> Self {
-        let (tx, mut rx) = mpsc::unbounded::<(String, oneshot::Sender<String>)>();
+        let (tx, mut rx) = mpsc::unbounded::<(String, oneshot::Sender<_>)>();
         let task = cx.spawn(async move |cx: &mut AsyncApp| {
             while let Some((prompt, channel)) = rx.next().await {
                 password_prompt(prompt, channel, cx);
@@ -35,7 +46,7 @@ impl AskPassDelegate {
         Self { tx, _task: task }
     }
 
-    pub async fn ask_password(&mut self, prompt: String) -> Result<String> {
+    pub async fn ask_password(&mut self, prompt: String) -> Result<EncryptedPassword> {
         let (tx, rx) = oneshot::channel();
         self.tx.send((prompt, tx)).await?;
         Ok(rx.await?)
@@ -48,7 +59,7 @@ pub struct AskPassSession {
     #[cfg(target_os = "windows")]
     askpass_helper: String,
     #[cfg(target_os = "windows")]
-    secret: std::sync::Arc<parking_lot::Mutex<String>>,
+    secret: std::sync::Arc<OnceLock<EncryptedPassword>>,
     _askpass_task: Task<()>,
     askpass_opened_rx: Option<oneshot::Receiver<()>>,
     askpass_kill_master_rx: Option<oneshot::Receiver<()>>,
@@ -68,7 +79,7 @@ impl AskPassSession {
         use util::fs::make_file_executable;
 
         #[cfg(target_os = "windows")]
-        let secret = std::sync::Arc::new(parking_lot::Mutex::new(String::new()));
+        let secret = std::sync::Arc::new(OnceLock::new());
         let temp_dir = tempfile::Builder::new().prefix("zed-askpass").tempdir()?;
         let askpass_socket = temp_dir.path().join("askpass.sock");
         let askpass_script_path = temp_dir.path().join(ASKPASS_SCRIPT_NAME);
@@ -104,10 +115,12 @@ impl AskPassSession {
                     .context("getting askpass password")
                     .log_err()
                 {
-                    stream.write_all(password.as_bytes()).await.log_err();
                     #[cfg(target_os = "windows")]
                     {
-                        *askpass_secret.lock() = password;
+                        askpass_secret.get_or_init(|| password.clone());
+                    }
+                    if let Ok(decrypted) = decrypt(password) {
+                        stream.write_all(decrypted.as_bytes()).await.log_err();
                     }
                 } else {
                     if let Some(kill_tx) = kill_tx.take() {
@@ -188,8 +201,8 @@ impl AskPassSession {
 
     /// This will return the password that was last set by the askpass script.
     #[cfg(target_os = "windows")]
-    pub fn get_password(&self) -> String {
-        self.secret.lock().clone()
+    pub fn get_password(&self) -> Option<EncryptedPassword> {
+        self.secret.get().cloned()
     }
 }
 

crates/askpass/src/encrypted_password.rs

@@ -0,0 +1,111 @@
+//! This module provides [EncryptedPassword] for storage of passwords in memory.
+//! On Windows that's implemented with CryptProtectMemory/CryptUnprotectMemory; on other platforms it just falls through
+//! to string for now.
+//!
+//! The "safety" of this module lies in exploiting visibility rules of Rust:
+//! 1. No outside module has access to the internal representation of [EncryptedPassword].
+//! 2. [EncryptedPassword] cannot be converted into a [String] or any other plaintext representation.
+//! All use cases that do need such functionality (of which we have two right now) are implemented within this module.
+//!
+//! Note that this is not bulletproof.
+//! 1. [ProcessExt] is implemented for [smol::process::Command], which is a builder for smol processes.
+//! Before the process itself is spawned the contents of [EncryptedPassword] are unencrypted in env var storage of said builder.
+//! 2. We're also sending plaintext passwords over RPC with [proto::AskPassResponse]. Go figure how great that is.
+//!
+//! Still, the goal of this module is to not have passwords laying around nilly-willy in memory.
+//! We do not claim that it is fool-proof.
+use anyhow::Result;
+use zeroize::Zeroize;
+
+type LengthWithoutPadding = u32;
+#[derive(Clone)]
+pub struct EncryptedPassword(Vec<u8>, LengthWithoutPadding);
+
+pub trait ProcessExt {
+    fn encrypted_env(&mut self, name: &str, value: EncryptedPassword) -> &mut Self;
+}
+
+impl ProcessExt for smol::process::Command {
+    fn encrypted_env(&mut self, name: &str, value: EncryptedPassword) -> &mut Self {
+        if let Ok(password) = decrypt(value) {
+            self.env(name, password);
+        }
+        self
+    }
+}
+
+impl TryFrom<EncryptedPassword> for proto::AskPassResponse {
+    type Error = anyhow::Error;
+    fn try_from(pw: EncryptedPassword) -> Result<Self, Self::Error> {
+        let pw = decrypt(pw)?;
+        Ok(Self { response: pw })
+    }
+}
+
+impl Drop for EncryptedPassword {
+    fn drop(&mut self) {
+        self.0.zeroize();
+        self.1.zeroize();
+    }
+}
+
+impl TryFrom<&str> for EncryptedPassword {
+    type Error = anyhow::Error;
+    fn try_from(password: &str) -> Result<EncryptedPassword> {
+        let len: u32 = password.len().try_into()?;
+        #[cfg(windows)]
+        {
+            use windows::Win32::Security::Cryptography::{
+                CRYPTPROTECTMEMORY_BLOCK_SIZE, CRYPTPROTECTMEMORY_SAME_PROCESS, CryptProtectMemory,
+            };
+            let mut value = password.bytes().collect::<Vec<_>>();
+            let padded_length = len.next_multiple_of(CRYPTPROTECTMEMORY_BLOCK_SIZE);
+            if padded_length != len {
+                value.resize(padded_length as usize, 0);
+            }
+            unsafe {
+                CryptProtectMemory(
+                    value.as_mut_ptr() as _,
+                    len,
+                    CRYPTPROTECTMEMORY_SAME_PROCESS,
+                )?;
+            }
+            Ok(Self(value, len))
+        }
+        #[cfg(not(windows))]
+        Ok(Self(String::from(password).into(), len))
+    }
+}
+
+pub(crate) fn decrypt(mut password: EncryptedPassword) -> Result<String> {
+    #[cfg(windows)]
+    {
+        use anyhow::Context;
+        use windows::Win32::Security::Cryptography::{
+            CRYPTPROTECTMEMORY_BLOCK_SIZE, CRYPTPROTECTMEMORY_SAME_PROCESS, CryptUnprotectMemory,
+        };
+        assert_eq!(
+            password.0.len() % CRYPTPROTECTMEMORY_BLOCK_SIZE as usize,
+            0,
+            "Violated pre-condition (buffer size <{}> must be a multiple of CRYPTPROTECTMEMORY_BLOCK_SIZE <{}>) for CryptUnprotectMemory.",
+            password.0.len(),
+            CRYPTPROTECTMEMORY_BLOCK_SIZE
+        );
+        unsafe {
+            CryptUnprotectMemory(
+                password.0.as_mut_ptr() as _,
+                password.1,
+                CRYPTPROTECTMEMORY_SAME_PROCESS,
+            )
+            .context("while decrypting a SSH password")?
+        };
+
+        {
+            // Remove padding
+            _ = password.0.drain(password.1 as usize..);
+        }
+        Ok(String::from_utf8(std::mem::take(&mut password.0))?)
+    }
+    #[cfg(not(windows))]
+    Ok(String::from_utf8(std::mem::take(&mut password.0))?)
+}

crates/git_ui/Cargo.toml

@@ -61,6 +61,7 @@ watch.workspace = true
 workspace-hack.workspace = true
 workspace.workspace = true
 zed_actions.workspace = true
+zeroize.workspace = true
 
 [target.'cfg(windows)'.dependencies]
 windows.workspace = true

crates/git_ui/src/askpass_modal.rs

@@ -1,3 +1,4 @@
+use askpass::EncryptedPassword;
 use editor::Editor;
 use futures::channel::oneshot;
 use gpui::{AppContext, DismissEvent, Entity, EventEmitter, Focusable, Styled};
@@ -7,13 +8,15 @@ use ui::{
     LabelSize, ParentElement, Render, SharedString, StyledExt, StyledTypography, Window, div,
     h_flex, v_flex,
 };
+use util::maybe;
 use workspace::ModalView;
+use zeroize::Zeroize;
 
 pub(crate) struct AskPassModal {
     operation: SharedString,
     prompt: SharedString,
     editor: Entity<Editor>,
-    tx: Option<oneshot::Sender<String>>,
+    tx: Option<oneshot::Sender<EncryptedPassword>>,
 }
 
 impl EventEmitter<DismissEvent> for AskPassModal {}
@@ -28,7 +31,7 @@ impl AskPassModal {
     pub fn new(
         operation: SharedString,
         prompt: SharedString,
-        tx: oneshot::Sender<String>,
+        tx: oneshot::Sender<EncryptedPassword>,
         window: &mut Window,
         cx: &mut Context<Self>,
     ) -> Self {
@@ -53,10 +56,20 @@ impl AskPassModal {
         cx.emit(DismissEvent);
     }
 
-    fn confirm(&mut self, _: &menu::Confirm, _window: &mut Window, cx: &mut Context<Self>) {
-        if let Some(tx) = self.tx.take() {
-            tx.send(self.editor.read(cx).text(cx)).ok();
-        }
+    fn confirm(&mut self, _: &menu::Confirm, window: &mut Window, cx: &mut Context<Self>) {
+        maybe!({
+            let tx = self.tx.take()?;
+            let mut text = self.editor.update(cx, |this, cx| {
+                let text = this.text(cx);
+                this.clear(window, cx);
+                text
+            });
+            let pw = askpass::EncryptedPassword::try_from(text.as_ref()).ok()?;
+            text.zeroize();
+            tx.send(pw).ok();
+            Some(())
+        });
+
         cx.emit(DismissEvent);
     }
 

crates/project/Cargo.toml

@@ -89,6 +89,7 @@ util.workspace = true
 watch.workspace = true
 which.workspace = true
 worktree.workspace = true
+zeroize.workspace = true
 zlog.workspace = true
 workspace-hack.workspace = true
 

crates/project/src/git_store.rs

@@ -7,7 +7,7 @@ use crate::{
     worktree_store::{WorktreeStore, WorktreeStoreEvent},
 };
 use anyhow::{Context as _, Result, anyhow, bail};
-use askpass::AskPassDelegate;
+use askpass::{AskPassDelegate, EncryptedPassword};
 use buffer_diff::{BufferDiff, BufferDiffEvent};
 use client::ProjectId;
 use collections::HashMap;
@@ -68,6 +68,7 @@ use worktree::{
     File, PathChange, PathKey, PathProgress, PathSummary, PathTarget, ProjectEntryId,
     UpdatedGitRepositoriesSet, UpdatedGitRepository, Worktree,
 };
+use zeroize::Zeroize;
 
 pub struct GitStore {
     state: GitStoreState,
@@ -2106,7 +2107,7 @@ impl GitStore {
             .lock()
             .insert(envelope.payload.askpass_id, askpass);
 
-        Ok(proto::AskPassResponse { response })
+        response.try_into()
     }
 
     async fn handle_check_for_pushed_commits(
@@ -2740,7 +2741,10 @@ fn make_remote_delegate(
                 prompt,
             });
             cx.spawn(async move |_, _| {
-                tx.send(response.await?.response).ok();
+                let mut response = response.await?.response;
+                tx.send(EncryptedPassword::try_from(response.as_ref())?)
+                    .ok();
+                response.zeroize();
                 anyhow::Ok(())
             })
             .detach_and_log_err(cx);

crates/recent_projects/Cargo.toml

@@ -14,6 +14,7 @@ doctest = false
 
 [dependencies]
 anyhow.workspace = true
+askpass.workspace = true
 auto_update.workspace = true
 editor.workspace = true
 extension_host.workspace = true

crates/recent_projects/src/remote_connections.rs

@@ -1,6 +1,7 @@
 use std::{path::PathBuf, sync::Arc};
 
 use anyhow::{Context as _, Result};
+use askpass::EncryptedPassword;
 use auto_update::AutoUpdater;
 use editor::Editor;
 use extension_host::ExtensionStore;
@@ -118,7 +119,7 @@ pub struct RemoteConnectionPrompt {
     nickname: Option<SharedString>,
     is_wsl: bool,
     status_message: Option<SharedString>,
-    prompt: Option<(Entity<Markdown>, oneshot::Sender<String>)>,
+    prompt: Option<(Entity<Markdown>, oneshot::Sender<EncryptedPassword>)>,
     cancellation: Option<oneshot::Sender<()>>,
     editor: Entity<Editor>,
 }
@@ -160,10 +161,10 @@ impl RemoteConnectionPrompt {
         self.cancellation = Some(tx);
     }
 
-    pub fn set_prompt(
+    fn set_prompt(
         &mut self,
         prompt: String,
-        tx: oneshot::Sender<String>,
+        tx: oneshot::Sender<EncryptedPassword>,
         window: &mut Window,
         cx: &mut Context<Self>,
     ) {
@@ -203,8 +204,12 @@ impl RemoteConnectionPrompt {
     pub fn confirm(&mut self, window: &mut Window, cx: &mut Context<Self>) {
         if let Some((_, tx)) = self.prompt.take() {
             self.status_message = Some("Connecting".into());
+
             self.editor.update(cx, |editor, cx| {
-                tx.send(editor.text(cx)).ok();
+                let pw = editor.text(cx);
+                if let Ok(secure) = EncryptedPassword::try_from(pw.as_ref()) {
+                    tx.send(secure).ok();
+                }
                 editor.clear(window, cx);
             });
         }
@@ -438,11 +443,16 @@ impl ModalView for RemoteConnectionModal {
 pub struct RemoteClientDelegate {
     window: AnyWindowHandle,
     ui: WeakEntity<RemoteConnectionPrompt>,
-    known_password: Option<String>,
+    known_password: Option<EncryptedPassword>,
 }
 
 impl remote::RemoteClientDelegate for RemoteClientDelegate {
-    fn ask_password(&self, prompt: String, tx: oneshot::Sender<String>, cx: &mut AsyncApp) {
+    fn ask_password(
+        &self,
+        prompt: String,
+        tx: oneshot::Sender<EncryptedPassword>,
+        cx: &mut AsyncApp,
+    ) {
         let mut known_password = self.known_password.clone();
         if let Some(password) = known_password.take() {
             tx.send(password).ok();
@@ -531,7 +541,10 @@ pub fn connect_over_ssh(
     cx: &mut App,
 ) -> Task<Result<Option<Entity<RemoteClient>>>> {
     let window = window.window_handle();
-    let known_password = connection_options.password.clone();
+    let known_password = connection_options
+        .password
+        .as_deref()
+        .and_then(|pw| EncryptedPassword::try_from(pw).ok());
     let (tx, rx) = oneshot::channel();
     ui.update(cx, |ui, _cx| ui.set_cancellation_tx(tx));
 
@@ -557,9 +570,10 @@ pub fn connect(
 ) -> Task<Result<Option<Entity<RemoteClient>>>> {
     let window = window.window_handle();
     let known_password = match &connection_options {
-        RemoteConnectionOptions::Ssh(ssh_connection_options) => {
-            ssh_connection_options.password.clone()
-        }
+        RemoteConnectionOptions::Ssh(ssh_connection_options) => ssh_connection_options
+            .password
+            .as_deref()
+            .and_then(|pw| pw.try_into().ok()),
         _ => None,
     };
     let (tx, rx) = oneshot::channel();
@@ -644,7 +658,10 @@ pub async fn open_remote_project(
                     known_password: if let RemoteConnectionOptions::Ssh(options) =
                         &connection_options
                     {
-                        options.password.clone()
+                        options
+                            .password
+                            .as_deref()
+                            .and_then(|pw| EncryptedPassword::try_from(pw).ok())
                     } else {
                         None
                     },

crates/remote/Cargo.toml

@@ -44,6 +44,7 @@ util.workspace = true
 which.workspace = true
 workspace-hack.workspace = true
 
+
 [dev-dependencies]
 gpui = { workspace = true, features = ["test-support"] }
 fs = { workspace = true, features = ["test-support"] }

crates/remote/src/remote_client.rs

@@ -8,6 +8,7 @@ use crate::{
     },
 };
 use anyhow::{Context as _, Result, anyhow};
+use askpass::EncryptedPassword;
 use async_trait::async_trait;
 use collections::HashMap;
 use futures::{
@@ -60,7 +61,12 @@ pub struct CommandTemplate {
 }
 
 pub trait RemoteClientDelegate: Send + Sync {
-    fn ask_password(&self, prompt: String, tx: oneshot::Sender<String>, cx: &mut AsyncApp);
+    fn ask_password(
+        &self,
+        prompt: String,
+        tx: oneshot::Sender<EncryptedPassword>,
+        cx: &mut AsyncApp,
+    );
     fn get_download_params(
         &self,
         platform: RemotePlatform,
@@ -1373,6 +1379,7 @@ mod fake {
     use super::{ChannelClient, RemoteClientDelegate, RemoteConnection, RemotePlatform};
     use crate::remote_client::{CommandTemplate, RemoteConnectionOptions};
     use anyhow::Result;
+    use askpass::EncryptedPassword;
     use async_trait::async_trait;
     use collections::HashMap;
     use futures::{
@@ -1517,7 +1524,7 @@ mod fake {
     pub(super) struct Delegate;
 
     impl RemoteClientDelegate for Delegate {
-        fn ask_password(&self, _: String, _: oneshot::Sender<String>, _: &mut AsyncApp) {
+        fn ask_password(&self, _: String, _: oneshot::Sender<EncryptedPassword>, _: &mut AsyncApp) {
             unreachable!()
         }
 

crates/remote/src/transport/ssh.rs

@@ -74,6 +74,8 @@ struct SshSocket {
     #[cfg(not(target_os = "windows"))]
     socket_path: PathBuf,
     envs: HashMap<String, String>,
+    #[cfg(target_os = "windows")]
+    password: askpass::EncryptedPassword,
 }
 
 macro_rules! shell_script {
@@ -347,7 +349,13 @@ impl SshRemoteConnection {
         #[cfg(not(target_os = "windows"))]
         let socket = SshSocket::new(connection_options, socket_path)?;
         #[cfg(target_os = "windows")]
-        let socket = SshSocket::new(connection_options, &temp_dir, askpass.get_password())?;
+        let socket = SshSocket::new(
+            connection_options,
+            &temp_dir,
+            askpass
+                .get_password()
+                .context("Failed to fetch askpass password")?,
+        )?;
         drop(askpass);
 
         let ssh_platform = socket.platform().await?;
@@ -674,16 +682,21 @@ impl SshSocket {
     }
 
     #[cfg(target_os = "windows")]
-    fn new(options: SshConnectionOptions, temp_dir: &TempDir, secret: String) -> Result<Self> {
+    fn new(
+        options: SshConnectionOptions,
+        temp_dir: &TempDir,
+        password: askpass::EncryptedPassword,
+    ) -> Result<Self> {
         let askpass_script = temp_dir.path().join("askpass.bat");
         std::fs::write(&askpass_script, "@ECHO OFF\necho %ZED_SSH_ASKPASS%")?;
         let mut envs = HashMap::default();
         envs.insert("SSH_ASKPASS_REQUIRE".into(), "force".into());
         envs.insert("SSH_ASKPASS".into(), askpass_script.display().to_string());
-        envs.insert("ZED_SSH_ASKPASS".into(), secret);
+
         Ok(Self {
             connection_options: options,
             envs,
+            password,
         })
     }
 
@@ -737,12 +750,14 @@ impl SshSocket {
 
     #[cfg(target_os = "windows")]
     fn ssh_options<'a>(&self, command: &'a mut process::Command) -> &'a mut process::Command {
+        use askpass::ProcessExt;
         command
             .stdin(Stdio::piped())
             .stdout(Stdio::piped())
             .stderr(Stdio::piped())
             .args(self.connection_options.additional_args())
             .envs(self.envs.clone())
+            .encrypted_env("ZED_SSH_ASKPASS", self.password.clone())
     }
 
     // On Windows, we need to use `SSH_ASKPASS` to provide the password to ssh.