Merge branch 'simplify-pipe-permissions'

8 files changed, 10 insertions, 182 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 244fafbe95..4772e6844b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -51,6 +51,10 @@ Line wrap the file at 100 chars.                                              Th
   OpenVPN by correctly applying the fix for
   [CVE-2019-14899](https://seclists.org/oss-sec/2019/q4/122).
 
+#### Windows
+- Deny network access to the OpenVPN plugin pipe, which allowed for anonymous write access when
+  passwordless file sharing was enabled.
+
 ### Changed
 - Allow the API to be accessed while in a blocking state.
 - Prefer the last used API endpoint when the service starts back up, as well as in other tools such
diff --git a/Cargo.lock b/Cargo.lock
index 66de6efc0b..a0cf1de768 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1310,7 +1310,6 @@ dependencies = [
  "tonic-build",
  "tower",
  "triggered",
- "winapi 0.3.9",
 ]
 
 [[package]]
@@ -1661,9 +1660,9 @@ dependencies = [
 
 [[package]]
 name = "parity-tokio-ipc"
-version = "0.7.0"
+version = "0.8.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "c1d417ba1ab454723ff2271bf999fd700027dc48759a13d43e488cc8ca38b87f"
+checksum = "fd7f6c69d7687501b2205fe51ade1d7b8797bb3aa141fe5bf13dd78c0483bc89"
 dependencies = [
  "futures",
  "libc",
diff --git a/mullvad-management-interface/Cargo.toml b/mullvad-management-interface/Cargo.toml
index bbd5917f4e..88bbe06ab7 100644
--- a/mullvad-management-interface/Cargo.toml
+++ b/mullvad-management-interface/Cargo.toml
@@ -14,11 +14,10 @@ tonic = "0.3.1"
 tower = "0.3"
 prost = "0.6"
 prost-types = "0.6"
-parity-tokio-ipc = "0.7"
+parity-tokio-ipc = "0.8"
 futures = "0.3"
 tokio = { version = "0.2", features =  [ "rt-util" ] }
 triggered = "0.1.1"
-winapi = { version = "0.3", features = ["errhandlingapi", "handleapi", "libloaderapi", "ntlsa", "synchapi", "tlhelp32", "winbase", "winerror", "winuser"] }
 
 [build-dependencies]
 tonic-build = { version = "0.3", default-features = false, features = ["transport", "prost"] }
diff --git a/mullvad-management-interface/src/lib.rs b/mullvad-management-interface/src/lib.rs
index 419e2d48b1..c527764f19 100644
--- a/mullvad-management-interface/src/lib.rs
+++ b/mullvad-management-interface/src/lib.rs
@@ -38,9 +38,6 @@ pub enum Error {
     PermissionsError(#[error(source)] io::Error),
 }
 
-#[cfg(target_os = "windows")]
-mod windows_permissions;
-
 pub async fn new_rpc_client() -> Result<ManagementServiceClient, Error> {
     let ipc_path = mullvad_paths::get_rpc_socket_path();
 
@@ -79,9 +76,6 @@ pub async fn spawn_rpc_server<T: ManagementService>(
     #[cfg(unix)]
     fs::set_permissions(&socket_path, PermissionsExt::from_mode(0o766))
         .map_err(Error::PermissionsError)?;
-    #[cfg(windows)]
-    crate::windows_permissions::deny_network_access(&socket_path)
-        .map_err(Error::PermissionsError)?;
 
     let _ = server_start_tx.send(());
 
diff --git a/mullvad-management-interface/src/windows_permissions.rs b/mullvad-management-interface/src/windows_permissions.rs
deleted file mode 100644
index c538312e03..0000000000
--- a/mullvad-management-interface/src/windows_permissions.rs
+++ /dev/null
@@ -1,168 +0,0 @@
-#![cfg(windows)]
-use std::{ffi::OsStr, io, os::windows::ffi::OsStrExt, ptr};
-use winapi::{
-    shared::{minwindef::DWORD, winerror::ERROR_SUCCESS},
-    um::{
-        accctrl::*,
-        aclapi::{SetEntriesInAclW, SetSecurityInfo},
-        fileapi::{CreateFileW, OPEN_EXISTING},
-        handleapi::{CloseHandle, INVALID_HANDLE_VALUE},
-        securitybaseapi::{AllocateAndInitializeSid, FreeSid},
-        winbase::LocalFree,
-        winnt::*,
-    },
-};
-
-struct Sid {
-    sid_ptr: PSID,
-}
-
-impl Sid {
-    pub fn new(authority: PSID_IDENTIFIER_AUTHORITY, relative_id: DWORD) -> Result<Sid, io::Error> {
-        let mut sid = Sid {
-            sid_ptr: ptr::null_mut(),
-        };
-
-        let result = unsafe {
-            AllocateAndInitializeSid(
-                authority,
-                1,
-                relative_id,
-                0,
-                0,
-                0,
-                0,
-                0,
-                0,
-                0,
-                &mut sid.sid_ptr as *mut _,
-            )
-        };
-        if result != 0 {
-            Ok(sid)
-        } else {
-            Err(io::Error::last_os_error())
-        }
-    }
-
-    pub fn as_ptr(&self) -> PSID {
-        self.sid_ptr
-    }
-}
-
-impl Drop for Sid {
-    fn drop(&mut self) {
-        unsafe { FreeSid(self.sid_ptr) };
-    }
-}
-
-struct WinHandle(HANDLE);
-
-impl WinHandle {
-    pub fn get_raw(&self) -> HANDLE {
-        self.0
-    }
-}
-
-impl Drop for WinHandle {
-    fn drop(&mut self) {
-        unsafe { CloseHandle(self.0) };
-    }
-}
-
-pub fn deny_network_access<T: AsRef<OsStr>>(ipc_path: T) -> Result<(), io::Error> {
-    let mut ipc_w: Vec<_> = ipc_path.as_ref().encode_wide().collect();
-    ipc_w.push(0u16);
-
-    let pipe_handle = unsafe {
-        CreateFileW(
-            ipc_w.as_ptr(),
-            GENERIC_WRITE | WRITE_DAC,
-            0,
-            ptr::null_mut(),
-            OPEN_EXISTING,
-            0,
-            ptr::null_mut(),
-        )
-    };
-
-    if pipe_handle == INVALID_HANDLE_VALUE {
-        return Err(io::Error::last_os_error());
-    }
-
-    let pipe_handle = WinHandle(pipe_handle);
-
-    let network_sid = Sid::new(
-        SECURITY_NT_AUTHORITY.as_ptr() as *const _ as *mut _,
-        SECURITY_NETWORK_RID,
-    )?;
-
-    let mut network_access: EXPLICIT_ACCESS_W = unsafe { std::mem::zeroed() };
-    network_access.grfAccessPermissions = GENERIC_READ | GENERIC_WRITE;
-    network_access.grfAccessMode = DENY_ACCESS;
-    network_access.grfInheritance = NO_INHERITANCE;
-    network_access.Trustee.TrusteeForm = TRUSTEE_IS_SID;
-    network_access.Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;
-    network_access.Trustee.ptstrName = network_sid.as_ptr() as *mut _;
-
-    let network_svc_sid = Sid::new(
-        SECURITY_NT_AUTHORITY.as_ptr() as *const _ as *mut _,
-        SECURITY_NETWORK_SERVICE_RID,
-    )?;
-
-    let mut network_svc_access: EXPLICIT_ACCESS_W = unsafe { std::mem::zeroed() };
-    network_svc_access.grfAccessPermissions = GENERIC_READ | GENERIC_WRITE;
-    network_svc_access.grfAccessMode = DENY_ACCESS;
-    network_svc_access.grfInheritance = NO_INHERITANCE;
-    network_svc_access.Trustee.TrusteeForm = TRUSTEE_IS_SID;
-    network_svc_access.Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;
-    network_svc_access.Trustee.ptstrName = network_svc_sid.as_ptr() as *mut _;
-
-    let everyone_sid = Sid::new(
-        SECURITY_WORLD_SID_AUTHORITY.as_ptr() as *const _ as *mut _,
-        SECURITY_WORLD_RID,
-    )?;
-
-    let mut world_access: EXPLICIT_ACCESS_W = unsafe { std::mem::zeroed() };
-    world_access.grfAccessPermissions = GENERIC_READ | GENERIC_WRITE;
-    world_access.grfAccessMode = SET_ACCESS;
-    world_access.grfInheritance = NO_INHERITANCE;
-    world_access.Trustee.TrusteeForm = TRUSTEE_IS_SID;
-    world_access.Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;
-    world_access.Trustee.ptstrName = everyone_sid.as_ptr() as *mut _;
-
-    let mut ace_entries = vec![network_access, network_svc_access, world_access];
-
-    let mut new_dacl: PACL = unsafe { std::mem::zeroed() };
-    let result = unsafe {
-        SetEntriesInAclW(
-            ace_entries.len() as u32,
-            ace_entries.as_mut_ptr(),
-            ptr::null_mut(),
-            &mut new_dacl as *mut PACL,
-        )
-    };
-    if result != ERROR_SUCCESS {
-        return Err(io::Error::from_raw_os_error(result as i32));
-    }
-
-    let result = unsafe {
-        SetSecurityInfo(
-            pipe_handle.get_raw(),
-            SE_KERNEL_OBJECT,
-            DACL_SECURITY_INFORMATION,
-            ptr::null_mut(),
-            ptr::null_mut(),
-            new_dacl as *mut ACL,
-            ptr::null_mut(),
-        )
-    };
-
-    unsafe { LocalFree(new_dacl as *mut _) };
-
-    if result != ERROR_SUCCESS {
-        return Err(io::Error::from_raw_os_error(result as i32));
-    }
-
-    Ok(())
-}
diff --git a/mullvad-tests/Cargo.toml b/mullvad-tests/Cargo.toml
index d2fca84af4..148f5283aa 100644
--- a/mullvad-tests/Cargo.toml
+++ b/mullvad-tests/Cargo.toml
@@ -29,7 +29,7 @@ tokio = { version = "0.2", features =  [ "io-util", "process", "rt-core", "rt-th
 tonic = "0.3.1"
 tower = "0.3"
 prost = "0.6"
-parity-tokio-ipc = "0.7"
+parity-tokio-ipc = "0.8"
 
 [build-dependencies]
 tonic-build = { version = "0.3", default-features = false, features = ["transport", "prost"] }
diff --git a/talpid-core/Cargo.toml b/talpid-core/Cargo.toml
index 6c03f1af90..1a698402f2 100644
--- a/talpid-core/Cargo.toml
+++ b/talpid-core/Cargo.toml
@@ -32,7 +32,7 @@ rand = "0.7"
 
 [target.'cfg(not(target_os="android"))'.dependencies]
 openvpn-plugin = { version = "0.4", features = ["serde", "auth-failed-event"] }
-parity-tokio-ipc = "0.7"
+parity-tokio-ipc = "0.8"
 triggered = "0.1.1"
 tonic = "0.3.1"
 prost = "0.6"
diff --git a/talpid-openvpn-plugin/Cargo.toml b/talpid-openvpn-plugin/Cargo.toml
index b005ed60ec..91a68b37aa 100644
--- a/talpid-openvpn-plugin/Cargo.toml
+++ b/talpid-openvpn-plugin/Cargo.toml
@@ -14,7 +14,7 @@ crate-type = ["cdylib"]
 err-derive = "0.2.1"
 log = "0.4"
 env_logger = "0.7"
-parity-tokio-ipc = "0.7"
+parity-tokio-ipc = "0.8"
 tokio = { version = "0.2", features =  [ "rt-core" ] }
 
 openvpn-plugin = { version = "0.4", features = ["serde", "log", "auth-failed-event"] }