Skip to content

Remove filesystem checks #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
12 changes: 0 additions & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
82 changes: 0 additions & 82 deletions src/core/ipc_handler/unix_socket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -32,9 +21,6 @@ pub struct Handler {

impl Connect for Handler {
fn connect(&self) -> Result<Box<dyn ReadWrite>> {
#[cfg(not(feature = "no-fs-permission-check"))]
self.secure_parsec_socket_folder()?;

let stream = UnixStream::connect(self.path.clone()).map_err(ClientErrorKind::Ipc)?;

stream
Expand All @@ -57,74 +43,6 @@ impl Handler {
pub fn new(path: PathBuf, timeout: Option<Duration>) -> 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 {
Expand Down
22 changes: 3 additions & 19 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines -66 to -84
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clippy started complaining about this in the new version

matches!(other, ClientErrorKind::InvalidServiceResponseType)
}
ClientErrorKind::InvalidProvider => matches!(other, ClientErrorKind::InvalidProvider),
ClientErrorKind::NoProvider => matches!(other, ClientErrorKind::NoProvider),
}
}
}
Expand Down