From b70f6697f96e6f6c537c666f290fbb94731d964a Mon Sep 17 00:00:00 2001 From: Ionut Mihalcea Date: Wed, 14 Oct 2020 12:45:15 +0100 Subject: [PATCH] Remove filesystem checks This commit removes the filesystem checks for correct permissions on Parsec "assets". The checks can now be removed because the new locations in the filesystem mean that only a privileged user can create the files. Signed-off-by: Ionut Mihalcea --- Cargo.toml | 3 +- README.md | 12 ----- src/core/ipc_handler/unix_socket.rs | 82 ----------------------------- src/error.rs | 22 ++------ 4 files changed, 4 insertions(+), 115 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a898e6d..4c5de50 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,5 +24,4 @@ users = "0.10.0" mockstream = "0.0.3" [features] -testing = ["parsec-interface/testing", "no-fs-permission-check"] -no-fs-permission-check = [] +testing = ["parsec-interface/testing"] diff --git a/README.md b/README.md index efabd06..bd802e7 100644 --- a/README.md +++ b/README.md @@ -7,18 +7,6 @@ This repository contains a Rust client for consuming the API provided by the [Parsec service](https://github.com/parallaxsecond/parsec). The low-level functionality that this library uses for IPC is implemented in the [interface crate](https://github.com/parallaxsecond/parsec-interface-rs). -## Filesystem permission check - -To make sure that the client is communicating with a trusted Parsec service, some permission checks -are done on the socket location. Please see the -[Recommendations for Secure Deployment](https://parallaxsecond.github.io/parsec-book/threat_model/secure_deployment.html) -for more information. -This feature is activated by default but, knowing the risks, you can remove it with: -``` -cargo build --features no-fs-permission-check -``` -It is also desactivated for testing. - ## License The software is provided under Apache-2.0. Contributions to this project are accepted under the same license. diff --git a/src/core/ipc_handler/unix_socket.rs b/src/core/ipc_handler/unix_socket.rs index 1be8b46..b94b759 100644 --- a/src/core/ipc_handler/unix_socket.rs +++ b/src/core/ipc_handler/unix_socket.rs @@ -7,17 +7,6 @@ use std::os::unix::net::UnixStream; use std::path::PathBuf; use std::time::Duration; -#[cfg(not(feature = "no-fs-permission-check"))] -use log::error; -#[cfg(not(feature = "no-fs-permission-check"))] -use std::ffi::OsStr; -#[cfg(not(feature = "no-fs-permission-check"))] -use std::fs; -#[cfg(not(feature = "no-fs-permission-check"))] -use std::io::{Error, ErrorKind}; -#[cfg(not(feature = "no-fs-permission-check"))] -use std::os::unix::fs::MetadataExt; - const DEFAULT_SOCKET_PATH: &str = "/run/parsec/parsec.sock"; const DEFAULT_TIMEOUT: Duration = Duration::from_secs(1); @@ -32,9 +21,6 @@ pub struct Handler { impl Connect for Handler { fn connect(&self) -> Result> { - #[cfg(not(feature = "no-fs-permission-check"))] - self.secure_parsec_socket_folder()?; - let stream = UnixStream::connect(self.path.clone()).map_err(ClientErrorKind::Ipc)?; stream @@ -57,74 +43,6 @@ impl Handler { pub fn new(path: PathBuf, timeout: Option) -> Self { Handler { path, timeout } } - - /// Checks if the socket is inside a folder with correct owners and permissions to make sure it - /// is from the Parsec service. - #[cfg(not(feature = "no-fs-permission-check"))] - fn secure_parsec_socket_folder(&self) -> Result<()> { - let mut socket_dir = self.path.clone(); - if !socket_dir.pop() { - return Err(ClientErrorKind::Ipc(Error::new( - ErrorKind::Other, - "Socket permission checks failed", - )) - .into()); - } - let meta = fs::metadata(socket_dir).map_err(ClientErrorKind::Ipc)?; - - match users::get_user_by_uid(meta.uid()) { - Some(user) => { - if user.name() != OsStr::new("parsec") { - error!("The socket directory must be owned by the parsec user."); - return Err(ClientErrorKind::Ipc(Error::new( - ErrorKind::Other, - "Socket permission checks failed", - )) - .into()); - } - } - None => { - error!("Can not find socket directory user owner."); - return Err(ClientErrorKind::Ipc(Error::new( - ErrorKind::Other, - "Socket permission checks failed", - )) - .into()); - } - } - - match users::get_group_by_gid(meta.gid()) { - Some(group) => { - if group.name() != OsStr::new("parsec-clients") { - error!("The socket directory must be owned by the parsec-clients group."); - return Err(ClientErrorKind::Ipc(Error::new( - ErrorKind::Other, - "Socket permission checks failed", - )) - .into()); - } - } - None => { - error!("Can not find socket directory group owner."); - return Err(ClientErrorKind::Ipc(Error::new( - ErrorKind::Other, - "Socket permission checks failed", - )) - .into()); - } - } - - if (meta.mode() & 0o777) != 0o750 { - error!("The permission bits of the folder containing the Parsec socket must be 750."); - return Err(ClientErrorKind::Ipc(Error::new( - ErrorKind::Other, - "Socket permission checks failed", - )) - .into()); - } - - Ok(()) - } } impl Default for Handler { diff --git a/src/error.rs b/src/error.rs index 81be41b..e96d190 100644 --- a/src/error.rs +++ b/src/error.rs @@ -63,26 +63,10 @@ impl PartialEq for ClientErrorKind { } } ClientErrorKind::InvalidServiceResponseType => { - if let ClientErrorKind::InvalidServiceResponseType = other { - true - } else { - false - } - } - ClientErrorKind::InvalidProvider => { - if let ClientErrorKind::InvalidProvider = other { - true - } else { - false - } - } - ClientErrorKind::NoProvider => { - if let ClientErrorKind::NoProvider = other { - true - } else { - false - } + matches!(other, ClientErrorKind::InvalidServiceResponseType) } + ClientErrorKind::InvalidProvider => matches!(other, ClientErrorKind::InvalidProvider), + ClientErrorKind::NoProvider => matches!(other, ClientErrorKind::NoProvider), } } }