Use user-accessible random temp dir on macOS

8 files changed, 101 insertions, 69 deletions

diff --git a/Cargo.lock b/Cargo.lock
index af13f48aa6..73019fabba 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2041,6 +2041,7 @@ dependencies = [
  "env_logger 0.10.2",
  "insta",
  "log",
+ "mullvad-paths",
  "mullvad-update",
  "native-windows-gui",
  "objc_id",
@@ -2890,7 +2891,6 @@ dependencies = [
  "insta",
  "json-canon",
  "mockito",
- "mullvad-paths",
  "mullvad-version",
  "rand 0.8.5",
  "reqwest",
diff --git a/installer-downloader/Cargo.toml b/installer-downloader/Cargo.toml
index e84e2091be..a8d54d2a14 100644
--- a/installer-downloader/Cargo.toml
+++ b/installer-downloader/Cargo.toml
@@ -31,6 +31,7 @@ mullvad-update = { path = "../mullvad-update", features = ["client", "native-tls
 [target.'cfg(target_os = "windows")'.dependencies]
 windows-sys = { workspace = true, features = ["Win32_UI", "Win32_UI_WindowsAndMessaging", "Win32_Graphics", "Win32_Graphics_Gdi"] }
 native-windows-gui = { version = "1.0.12", features = ["frame", "image-decoder", "progress-bar"], default-features = false }
+mullvad-paths = { path = "../mullvad-paths" }
 
 [target.'cfg(target_os = "macos")'.dependencies]
 cacao = "0.3.2"
diff --git a/installer-downloader/src/controller.rs b/installer-downloader/src/controller.rs
index 1485f7d2b5..bf492c0900 100644
--- a/installer-downloader/src/controller.rs
+++ b/installer-downloader/src/controller.rs
@@ -2,6 +2,7 @@
 
 use crate::delegate::{AppDelegate, AppDelegateQueue};
 use crate::resource;
+use crate::temp::DirectoryProvider;
 use crate::ui_downloader::{UiAppDownloader, UiAppDownloaderParameters, UiProgressUpdater};
 
 use mullvad_update::{
@@ -11,8 +12,6 @@ use mullvad_update::{
 };
 use rand::seq::SliceRandom;
 
-use std::future::Future;
-use std::path::PathBuf;
 use tokio::sync::{mpsc, oneshot};
 
 /// Actions handled by an async worker task in [handle_action_messages].
@@ -24,21 +23,6 @@ enum TaskMessage {
     TryStable,
 }
 
-/// Provide a directory to use for [AppDownloader]
-pub trait DirectoryProvider: 'static {
-    /// Provide a directory to use for [AppDownloader]
-    fn create_download_dir() -> impl Future<Output = anyhow::Result<PathBuf>> + Send;
-}
-
-struct TempDirProvider;
-
-impl DirectoryProvider for TempDirProvider {
-    /// Create a locked-down directory to store downloads in
-    fn create_download_dir() -> impl Future<Output = anyhow::Result<PathBuf>> + Send {
-        mullvad_update::dir::admin_temp_dir()
-    }
-}
-
 /// See the [module-level docs](self).
 pub struct AppController {}
 
@@ -49,7 +33,7 @@ pub fn initialize_controller<T: AppDelegate + 'static>(delegate: &mut T) {
     // App downloader to use
     type Downloader<T> = HttpAppDownloader<UiProgressUpdater<T>>;
     // Directory provider to use
-    type DirProvider = TempDirProvider;
+    type DirProvider = crate::temp::TempDirProvider;
 
     // Version info provider to use
     const TEST_PUBKEY: &str = include_str!("../../mullvad-update/test-pubkey");
@@ -218,6 +202,8 @@ async fn handle_action_messages<D, A, DirProvider>(
 
     let mut target_version = TargetVersion::Stable;
 
+    let temp_dir = DirProvider::create_download_dir().await;
+
     while let Some(msg) = rx.recv().await {
         match msg {
             TaskMessage::SetVersionInfo(new_version_info) => {
@@ -288,9 +274,11 @@ async fn handle_action_messages<D, A, DirProvider>(
                 });
 
                 // Create temporary dir
-                let download_dir = match DirProvider::create_download_dir().await {
-                    Ok(dir) => dir,
-                    Err(_err) => {
+                let download_dir = match &temp_dir {
+                    Ok(dir) => dir.clone(),
+                    Err(error) => {
+                        log::error!("Failed to create temporary directory: {error:?}");
+
                         queue.queue_main(move |self_| {
                             self_.set_status_text("");
                             self_.hide_download_button();
@@ -307,6 +295,8 @@ async fn handle_action_messages<D, A, DirProvider>(
                     }
                 };
 
+                log::debug!("Download directory: {}", download_dir.display());
+
                 // Begin download
                 let (tx, rx) = oneshot::channel();
                 queue.queue_main(move |self_| {
diff --git a/installer-downloader/src/lib.rs b/installer-downloader/src/lib.rs
index 2a9896ab4c..13e4759086 100644
--- a/installer-downloader/src/lib.rs
+++ b/installer-downloader/src/lib.rs
@@ -5,4 +5,6 @@ pub mod delegate;
 #[cfg(any(target_os = "windows", target_os = "macos"))]
 pub mod resource;
 #[cfg(any(target_os = "windows", target_os = "macos"))]
+pub mod temp;
+#[cfg(any(target_os = "windows", target_os = "macos"))]
 pub mod ui_downloader;
diff --git a/installer-downloader/src/temp.rs b/installer-downloader/src/temp.rs
new file mode 100644
index 0000000000..95487e10d1
--- /dev/null
+++ b/installer-downloader/src/temp.rs
@@ -0,0 +1,85 @@
+//! Creates a temporary directory for the installer.
+//!
+//! # Windows
+//!
+//! Since the Windows downloader runs as admin, we can use a persistent directory and prevent
+//! non-admins from accessing it.
+//!
+//! # macOS
+//!
+//! The downloader does not run as a privileged user, so we store downloads in a temporary
+//! directory.
+//!
+//! This is vulnerable to TOCTOU, ie replacing the file after its hash has been verified, but only
+//! by the current user. Using a random directory name mitigates this issue.
+
+use anyhow::Context;
+use std::{future::Future, path::PathBuf};
+
+/// Provide a directory to use for [AppDownloader]
+pub trait DirectoryProvider: 'static {
+    /// Provide a directory to use for [AppDownloader]
+    fn create_download_dir() -> impl Future<Output = anyhow::Result<PathBuf>> + Send;
+}
+
+/// See [module-level](self) docs.
+pub struct TempDirProvider;
+
+impl DirectoryProvider for TempDirProvider {
+    /// Create a locked-down directory to store downloads in
+    fn create_download_dir() -> impl Future<Output = anyhow::Result<PathBuf>> + Send {
+        #[cfg(windows)]
+        {
+            admin_temp_dir()
+        }
+
+        #[cfg(target_os = "macos")]
+        {
+            temp_dir()
+        }
+    }
+}
+
+/// This returns a directory where only admins have write access.
+///
+/// This function is a bit racey, as the directory is created before being restricted.
+/// This is acceptable as long as the checksum of each file is verified before being used.
+#[cfg(windows)]
+async fn admin_temp_dir() -> anyhow::Result<PathBuf> {
+    /// Name of subdirectory in the temp directory
+    const CACHE_DIRNAME: &str = "mullvad-updates";
+
+    let temp_dir = std::env::temp_dir().join(CACHE_DIRNAME);
+
+    let dir_clone = temp_dir.clone();
+    tokio::task::spawn_blocking(move || {
+        mullvad_paths::windows::create_privileged_directory(&dir_clone)
+    })
+    .await
+    .unwrap()
+    .context("Failed to create cache directory")?;
+
+    Ok(temp_dir)
+}
+
+#[cfg(target_os = "macos")]
+async fn temp_dir() -> anyhow::Result<PathBuf> {
+    use rand::{distributions::Alphanumeric, Rng};
+    use std::{fs::Permissions, os::unix::fs::PermissionsExt};
+    use tokio::fs;
+
+    // Randomly generate a directory name
+    let dir_name: String = (0..10)
+        .map(|_| rand::thread_rng().sample(Alphanumeric) as char)
+        .collect();
+    let temp_dir = std::env::temp_dir().join(dir_name);
+
+    fs::create_dir_all(&temp_dir)
+        .await
+        .context("Failed to create cache directory")?;
+    fs::set_permissions(&temp_dir, Permissions::from_mode(0o700))
+        .await
+        .context("Failed to set cache directory permissions")?;
+
+    Ok(temp_dir)
+}
diff --git a/mullvad-update/Cargo.toml b/mullvad-update/Cargo.toml
index f601f26ad3..1a21d7aa12 100644
--- a/mullvad-update/Cargo.toml
+++ b/mullvad-update/Cargo.toml
@@ -13,7 +13,7 @@ workspace = true
 [features]
 default = []
 sign = ["rand", "clap"]
-client = ["async-trait", "mullvad-paths", "reqwest", "sha2", "tokio", "thiserror"]
+client = ["async-trait", "reqwest", "sha2", "tokio", "thiserror"]
 native-tls = ["reqwest/native-tls"]
 
 [dependencies]
@@ -37,9 +37,6 @@ mullvad-version = { path = "../mullvad-version", features = ["serde"] }
 clap = { workspace = true, optional = true }
 rand = { version = "0.8.5", optional = true }
 
-[target.'cfg(target_os = "windows")'.dependencies]
-mullvad-paths = { path = "../mullvad-paths", optional = true }
-
 [dev-dependencies]
 async-tempfile = "0.6"
 insta = { workspace = true }
diff --git a/mullvad-update/src/client/dir.rs b/mullvad-update/src/client/dir.rs
deleted file mode 100644
index cf3a85eb24..0000000000
--- a/mullvad-update/src/client/dir.rs
+++ /dev/null
@@ -1,42 +0,0 @@
-//! This provides a secure temp directory suitable for storing updates, with admin-only write access.
-
-use std::path::PathBuf;
-
-use anyhow::Context;
-
-/// Name of subdirectory in the temp directory
-const CACHE_DIRNAME: &str = "mullvad-updates";
-
-/// This returns a directory suitable for storing updates, where only admins have write access.
-///
-/// This function is a bit racey, as the directory is created before being restricted.
-/// This is acceptable as long as the checksum of each file is verified before being used.
-pub async fn admin_temp_dir() -> anyhow::Result<PathBuf> {
-    let temp_dir = std::env::temp_dir().join(CACHE_DIRNAME);
-
-    #[cfg(windows)]
-    {
-        let dir_clone = temp_dir.clone();
-        tokio::task::spawn_blocking(move || {
-            mullvad_paths::windows::create_privileged_directory(&dir_clone)
-        })
-        .await
-        .unwrap()
-        .context("Failed to create cache directory")?;
-    }
-
-    #[cfg(unix)]
-    {
-        use std::{fs::Permissions, os::unix::fs::PermissionsExt};
-        use tokio::fs;
-
-        fs::create_dir_all(&temp_dir)
-            .await
-            .context("Failed to create cache directory")?;
-        fs::set_permissions(&temp_dir, Permissions::from_mode(0o700))
-            .await
-            .context("Failed to set cache directory permissions")?;
-    }
-
-    Ok(temp_dir)
-}
diff --git a/mullvad-update/src/client/mod.rs b/mullvad-update/src/client/mod.rs
index ac9ca6641c..4d8a4cc67a 100644
--- a/mullvad-update/src/client/mod.rs
+++ b/mullvad-update/src/client/mod.rs
@@ -1,5 +1,4 @@
 pub mod api;
 pub mod app;
-pub mod dir;
 pub mod fetch;
 pub mod verify;