Merge branch 'fix-macos-dir-exploit'

6 files changed, 160 insertions, 73 deletions

diff --git a/mullvad-paths/src/cache.rs b/mullvad-paths/src/cache.rs
index dfd7a46cae..3b31a9460b 100644
--- a/mullvad-paths/src/cache.rs
+++ b/mullvad-paths/src/cache.rs
@@ -4,13 +4,11 @@ use std::{env, path::PathBuf};
 /// Creates and returns the cache directory pointed to by `MULLVAD_CACHE_DIR`, or the default
 /// one if that variable is unset.
 pub fn cache_dir() -> Result<PathBuf> {
-    #[cfg(not(any(target_os = "macos", target_os = "windows")))]
-    let permissions = None;
-    #[cfg(target_os = "macos")]
-    let permissions = Some(std::os::unix::fs::PermissionsExt::from_mode(0o755));
+    #[cfg(unix)]
+    let permissions = crate::unix::Permissions::ReadExecOnly;
     #[cfg(target_os = "windows")]
     let permissions = true;
-    crate::create_and_return(get_cache_dir, permissions)
+    crate::create_dir(get_cache_dir()?, permissions)
 }
 
 pub fn get_cache_dir() -> Result<PathBuf> {
@@ -28,7 +26,7 @@ pub fn get_default_cache_dir() -> Result<PathBuf> {
 
 #[cfg(windows)]
 pub fn get_default_cache_dir() -> Result<PathBuf> {
-    let dir = crate::get_allusersprofile_dir()?
+    let dir = crate::windows::get_allusersprofile_dir()?
         .join(crate::PRODUCT_NAME)
         .join("cache");
     Ok(dir)
diff --git a/mullvad-paths/src/lib.rs b/mullvad-paths/src/lib.rs
index 05641360c0..c5eb5b9a52 100644
--- a/mullvad-paths/src/lib.rs
+++ b/mullvad-paths/src/lib.rs
@@ -1,11 +1,18 @@
 #![cfg(not(target_os = "android"))]
 
-#[cfg(any(target_os = "linux", target_os = "macos"))]
-use std::fs;
-use std::{io, path::PathBuf};
+use std::io;
 
 #[cfg(windows)]
-use crate::windows::create_dir_recursive;
+pub mod windows;
+
+#[cfg(windows)]
+pub use windows::PRODUCT_NAME;
+
+#[cfg(unix)]
+mod unix;
+
+#[cfg(unix)]
+pub use unix::PRODUCT_NAME;
 
 pub type Result<T> = std::result::Result<T, Error>;
 
@@ -14,6 +21,12 @@ pub enum Error {
     #[error("Failed to create directory {0}")]
     CreateDirFailed(String, #[source] io::Error),
 
+    #[error("Failed to remove directory {0}")]
+    RemoveDir(String, #[source] io::Error),
+
+    #[error("Failed to get directory permissions on {0}")]
+    GetDirPermissionFailed(String, #[source] io::Error),
+
     #[error("Failed to set directory permissions on {0}")]
     SetDirPermissionFailed(String, #[source] io::Error),
 
@@ -33,43 +46,11 @@ pub enum Error {
     NoDataDir,
 }
 
-#[cfg(any(target_os = "linux", target_os = "macos"))]
-const PRODUCT_NAME: &str = "mullvad-vpn";
-
-#[cfg(windows)]
-pub const PRODUCT_NAME: &str = "Mullvad VPN";
-
-#[cfg(windows)]
-fn get_allusersprofile_dir() -> Result<PathBuf> {
-    match std::env::var_os("ALLUSERSPROFILE") {
-        Some(dir) => Ok(PathBuf::from(&dir)),
-        None => Err(Error::NoProgramDataDir),
-    }
-}
-
-#[cfg(any(target_os = "linux", target_os = "macos"))]
-fn create_and_return(
-    dir_fn: fn() -> Result<PathBuf>,
-    permissions: Option<fs::Permissions>,
-) -> Result<PathBuf> {
-    let dir = dir_fn()?;
-    fs::create_dir_all(&dir).map_err(|e| Error::CreateDirFailed(dir.display().to_string(), e))?;
-    if let Some(permissions) = permissions {
-        fs::set_permissions(&dir, permissions)
-            .map_err(|e| Error::SetDirPermissionFailed(dir.display().to_string(), e))?;
-    }
-    Ok(dir)
-}
+#[cfg(unix)]
+use unix::create_dir;
 
 #[cfg(windows)]
-fn create_and_return(
-    dir_fn: fn() -> Result<PathBuf>,
-    set_security_permissions: bool,
-) -> Result<PathBuf> {
-    let dir = dir_fn()?;
-    create_dir_recursive(&dir, set_security_permissions)?;
-    Ok(dir)
-}
+use windows::create_dir;
 
 mod cache;
 pub use crate::cache::{cache_dir, get_cache_dir, get_default_cache_dir};
@@ -85,6 +66,3 @@ pub use crate::rpc_socket::{get_default_rpc_socket_path, get_rpc_socket_path};
 
 mod settings;
 pub use crate::settings::{get_default_settings_dir, settings_dir};
-
-#[cfg(windows)]
-pub mod windows;
diff --git a/mullvad-paths/src/logs.rs b/mullvad-paths/src/logs.rs
index 53f7763589..03ab990632 100644
--- a/mullvad-paths/src/logs.rs
+++ b/mullvad-paths/src/logs.rs
@@ -6,13 +6,11 @@ use std::{env, path::PathBuf};
 pub fn log_dir() -> Result<PathBuf> {
     #[cfg(unix)]
     {
-        use std::os::unix::fs::PermissionsExt;
-        let permissions = Some(PermissionsExt::from_mode(0o755));
-        crate::create_and_return(get_log_dir, permissions)
+        crate::create_dir(get_log_dir()?, crate::unix::Permissions::ReadExecOnly)
     }
     #[cfg(target_os = "windows")]
     {
-        crate::create_and_return(get_log_dir, true)
+        crate::create_dir(get_log_dir()?, true)
     }
 }
 
@@ -32,6 +30,6 @@ pub fn get_default_log_dir() -> Result<PathBuf> {
 
 #[cfg(windows)]
 pub fn get_default_log_dir() -> Result<PathBuf> {
-    let dir = crate::get_allusersprofile_dir()?.join(crate::PRODUCT_NAME);
+    let dir = crate::windows::get_allusersprofile_dir()?.join(crate::PRODUCT_NAME);
     Ok(dir)
 }
diff --git a/mullvad-paths/src/settings.rs b/mullvad-paths/src/settings.rs
index d6ebabe6e7..016e158bee 100644
--- a/mullvad-paths/src/settings.rs
+++ b/mullvad-paths/src/settings.rs
@@ -4,14 +4,14 @@ use std::{env, path::PathBuf};
 /// Creates and returns the settings directory pointed to by `MULLVAD_SETTINGS_DIR`, or the default
 /// one if that variable is unset.
 pub fn settings_dir() -> Result<PathBuf> {
-    #[cfg(not(target_os = "windows"))]
+    #[cfg(unix)]
     {
-        crate::create_and_return(get_settings_dir, None)
+        crate::create_dir(get_settings_dir()?, crate::unix::Permissions::Any)
     }
 
     #[cfg(target_os = "windows")]
     {
-        crate::create_and_return(get_settings_dir, false)
+        crate::create_dir(get_settings_dir()?, false)
     }
 }
 
diff --git a/mullvad-paths/src/unix.rs b/mullvad-paths/src/unix.rs
new file mode 100644
index 0000000000..90a5d2fc5c
--- /dev/null
+++ b/mullvad-paths/src/unix.rs
@@ -0,0 +1,103 @@
+use std::{
+    fs, io,
+    os::unix::fs::{DirBuilderExt, MetadataExt, PermissionsExt},
+    path::{Path, PathBuf},
+};
+
+use crate::{Error, Result};
+
+pub const PRODUCT_NAME: &str = "mullvad-vpn";
+
+#[derive(Clone, Copy, PartialEq)]
+pub enum Permissions {
+    /// Do not set any particular permissions. They will be inherited instead.
+    Any,
+    /// Only root should have write access. Other users will have
+    /// read and execute permissions (0o755).
+    ReadExecOnly,
+}
+
+impl Permissions {
+    fn fs_permissions(self) -> Option<fs::Permissions> {
+        match self {
+            Permissions::Any => None,
+            Permissions::ReadExecOnly => Some(std::os::unix::fs::PermissionsExt::from_mode(0o755)),
+        }
+    }
+}
+
+/// Create a directory at `dir`, setting the permissions given by `permissions`, unless it exists.
+/// If the directory already exists, but the permissions are not at least as strict as expected,
+/// then it will be deleted and recreated.
+pub fn create_dir(dir: PathBuf, permissions: Permissions) -> Result<PathBuf> {
+    let mut dir_builder = fs::DirBuilder::new();
+    let fs_perms = permissions.fs_permissions();
+    if let Some(fs_perms) = fs_perms.as_ref() {
+        dir_builder.mode(fs_perms.mode());
+    }
+    match dir_builder.create(&dir) {
+        Ok(()) => Ok(dir),
+        // The directory already exists
+        Err(error) if error.kind() == io::ErrorKind::AlreadyExists => {
+            // Recreate the directory if the ownership and permissions are unexpected
+            if !dir_is_root_owned(&dir, fs_perms.as_ref())? {
+                log::debug!(
+                    "Removing old directory due to unexpected permissions: {}",
+                    dir.display()
+                );
+
+                fs::remove_dir_all(&dir)
+                    .or_else(|err| {
+                        // If the path is not a directory, try to remove the file
+                        if err.kind() == io::ErrorKind::NotADirectory {
+                            fs::remove_file(&dir)
+                        } else {
+                            Err(err)
+                        }
+                    })
+                    .map_err(|e| Error::RemoveDir(dir.display().to_string(), e))?;
+
+                // Try to create it again
+                return create_dir(dir, permissions);
+            }
+            // Correct permissions, so we're done
+            Ok(dir)
+        }
+        // Fail on any other error
+        Err(error) => Err(Error::CreateDirFailed(dir.display().to_string(), error)),
+    }
+}
+
+/// Return whether the directory is owned by root and, optionally, is no less strict
+/// than the desired permissions
+fn dir_is_root_owned(dir: &Path, perms: Option<&fs::Permissions>) -> Result<bool> {
+    let meta = fs::symlink_metadata(dir)
+        .map_err(|e| Error::GetDirPermissionFailed(dir.display().to_string(), e))?;
+    let matching_perms = perms
+        .map(|perms| has_at_most_mask(meta.permissions().mode(), perms.mode()))
+        .unwrap_or(true);
+    Ok(matching_perms && meta.uid() == 0)
+}
+
+/// Return whether `mask` is *at least* as strict as `at_most`
+/// This only considers the read, write, and exec bits.
+fn has_at_most_mask(mask: u32, at_most: u32) -> bool {
+    // Ignore "D" bit, setuid bit, etc.
+    const RELEVANT_BITS: u32 = 0o777;
+    ((mask & RELEVANT_BITS) & !(at_most & RELEVANT_BITS)) == 0
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn test_has_at_most_mask() {
+        assert!(!has_at_most_mask(0o777, 0o577));
+        assert!(!has_at_most_mask(0o777, 0o707));
+        assert!(!has_at_most_mask(0o777, 0o770));
+
+        assert!(has_at_most_mask(0o777, 0o777));
+        assert!(has_at_most_mask(0o000, 0o777));
+    }
+}
diff --git a/mullvad-paths/src/windows.rs b/mullvad-paths/src/windows.rs
index fa652f31f7..102fa53f5f 100644
--- a/mullvad-paths/src/windows.rs
+++ b/mullvad-paths/src/windows.rs
@@ -45,6 +45,33 @@ use windows_sys::{
     },
 };
 
+pub const PRODUCT_NAME: &str = "Mullvad VPN";
+
+pub fn get_allusersprofile_dir() -> Result<PathBuf> {
+    match std::env::var_os("ALLUSERSPROFILE") {
+        Some(dir) => Ok(PathBuf::from(&dir)),
+        None => Err(Error::NoProgramDataDir),
+    }
+}
+
+/// This recursively creates directories, if set_security_permissions is true it will set
+/// file permissions corresponding to Authenticated Users - Read Only and Administrators - Full
+/// Access. Only directories that do not already exist and the leaf directory will have their
+/// permissions set.
+pub fn create_dir(path: PathBuf, set_security_permissions: bool) -> Result<PathBuf> {
+    if set_security_permissions {
+        create_dir_with_permissions_recursive(&path)?;
+    } else {
+        std::fs::create_dir_all(&path).map_err(|e| {
+            Error::CreateDirFailed(
+                format!("Could not create directory at {}", path.display()),
+                e,
+            )
+        })?;
+    }
+    Ok(path)
+}
+
 struct Handle(HANDLE);
 
 impl Drop for Handle {
@@ -66,23 +93,6 @@ fn get_wide_str<S: AsRef<OsStr>>(string: S) -> Vec<u16> {
     wide_string
 }
 
-/// Recursively creates directories, if set_security_permissions is true it will set
-/// file permissions corresponding to Authenticated Users - Read Only and Administrators - Full
-/// Access. Only directories that do not already exist and the leaf directory will have their
-/// permissions set.
-pub fn create_dir_recursive(path: &Path, set_security_permissions: bool) -> Result<()> {
-    if set_security_permissions {
-        create_dir_with_permissions_recursive(path)
-    } else {
-        std::fs::create_dir_all(path).map_err(|e| {
-            Error::CreateDirFailed(
-                format!("Could not create directory at {}", path.display()),
-                e,
-            )
-        })
-    }
-}
-
 /// If directory at path already exists, set permissions for it.
 /// If directory at path don't exist but parent does, create directory and set permissions.
 /// If parent directory at path does not exist then recurse and create parent directory and set