diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 04ae30be0d17fe8bd333b715f6b7ff448808e699..e9d49c3163c632243b52e7e8ac49067244914526 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -132,6 +132,9 @@ build-repro:
integration:
stage: test
image: debian:stable-slim
+ variables:
+ # The build environment here runs as root and seems to have umask 000.
+ ARTI_FS_DISABLE_PERMISSION_CHECKS: "true"
script:
- apt update
- apt install -y tor git python3 curl dnsutils
diff --git a/Cargo.lock b/Cargo.lock
index d1e8c4852320b475dfc39de2262658890a43caaf..f42c913068bd4240482582fa15dee728cbc86855 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -81,6 +81,7 @@ dependencies = [
"clap",
"config",
"derive_builder_fork_arti",
+ "fs-mistrust",
"futures",
"libc",
"notify",
@@ -131,6 +132,7 @@ dependencies = [
"derive_more",
"directories",
"educe",
+ "fs-mistrust",
"futures",
"humantime-serde",
"once_cell",
@@ -3470,6 +3472,7 @@ dependencies = [
"educe",
"event-listener",
"float_eq",
+ "fs-mistrust",
"fslock",
"futures",
"futures-await-test",
@@ -3663,6 +3666,7 @@ dependencies = [
name = "tor-persist"
version = "0.3.0"
dependencies = [
+ "fs-mistrust",
"fslock",
"sanitize-filename",
"serde",
diff --git a/crates/arti-client/Cargo.toml b/crates/arti-client/Cargo.toml
index c31e2c1ba3303eef6f3f0535dec13e68dd5560f6..22462ae13b52eec7a01dc3c2644ee58ff07f3dba 100644
--- a/crates/arti-client/Cargo.toml
+++ b/crates/arti-client/Cargo.toml
@@ -31,6 +31,7 @@ error_detail = []
experimental-api = []
[dependencies]
+fs-mistrust = { path = "../fs-mistrust", version = "0.1.0" }
safelog = { path = "../safelog", version = "0.1.0" }
tor-basic-utils = { path = "../tor-basic-utils", version = "0.3.0"}
tor-circmgr = { path = "../tor-circmgr", version = "0.3.0"}
diff --git a/crates/arti-client/src/builder.rs b/crates/arti-client/src/builder.rs
index f99a22f1473bdb0829fe0d8286cb54dfeb1f68dc..5771eb564f945113ae50bf21d326b170b43903dc 100644
--- a/crates/arti-client/src/builder.rs
+++ b/crates/arti-client/src/builder.rs
@@ -51,6 +51,8 @@ pub struct TorClientBuilder<R: Runtime> {
/// How the client should behave when it is asked to do something on the Tor
/// network before `bootstrap()` is called.
bootstrap_behavior: BootstrapBehavior,
+ /// How the client should decide which file permissions to trust.
+ fs_mistrust: Option<fs_mistrust::Mistrust>,
/// Optional object to construct a DirProvider.
///
/// Wrapped in an Arc so that we don't need to force DirProviderBuilder to
@@ -70,6 +72,7 @@ impl<R: Runtime> TorClientBuilder<R> {
runtime,
config: TorClientConfig::default(),
bootstrap_behavior: BootstrapBehavior::default(),
+ fs_mistrust: None,
dirmgr_builder: Arc::new(DirMgrBuilder {}),
#[cfg(feature = "dirfilter")]
dirfilter: None,
@@ -93,6 +96,31 @@ impl<R: Runtime> TorClientBuilder<R> {
self
}
+ /// Build an [`TorClient`] that will not validate permissions and
+ /// ownership on the filesystem.
+ ///
+ /// By default, these checks are enabled, unless the
+ /// `ARTI_FS_DISABLE_PERMISSION_CHECKS` environment variable has been set or
+ /// this method has been called.
+ pub fn disable_fs_permission_checks(mut self) -> Self {
+ let mut mistrust = fs_mistrust::Mistrust::new();
+ mistrust.dangerously_trust_everyone();
+ self.fs_mistrust = Some(mistrust);
+ self
+ }
+
+ /// Build an [`TorClient`] that will always validate permissions and
+ /// ownership on the filesystem.
+ ///
+ /// By default, these checks are enabled, unless the
+ /// `ARTI_FS_DISABLE_PERMISSION_CHECKS` environment variable has been set or
+ /// [`disable_fs_permission_checks`](Self::disable_fs_permission_checks)
+ /// method has been called.
+ pub fn enable_fs_permission_checks(mut self) -> Self {
+ self.fs_mistrust = Some(fs_mistrust::Mistrust::new());
+ self
+ }
+
/// Override the default function used to construct the directory provider.
///
/// Only available when compiled with the `experimental-api` feature: this
@@ -146,6 +174,8 @@ impl<R: Runtime> TorClientBuilder<R> {
self.runtime,
self.config,
self.bootstrap_behavior,
+ self.fs_mistrust
+ .unwrap_or_else(crate::config::default_fs_mistrust),
self.dirmgr_builder.as_ref(),
dirmgr_extensions,
)
diff --git a/crates/arti-client/src/client.rs b/crates/arti-client/src/client.rs
index 36da9ce3844dfd19aaba4457e8716f39e759c520..e328fbde3dd833b308f80d8062cffd3041ed068a 100644
--- a/crates/arti-client/src/client.rs
+++ b/crates/arti-client/src/client.rs
@@ -88,6 +88,9 @@ pub struct TorClient<R: Runtime> {
/// Shared boolean for whether we're currently in "dormant mode" or not.
dormant: Arc<AtomicBool>,
+
+ /// Settings for how we perform permissions checks on the filesystem.
+ fs_mistrust: fs_mistrust::Mistrust,
}
/// Preferences for whether a [`TorClient`] should bootstrap on its own or not.
@@ -350,15 +353,17 @@ impl<R: Runtime> TorClient<R> {
runtime: R,
config: TorClientConfig,
autobootstrap: BootstrapBehavior,
+ mistrust: fs_mistrust::Mistrust,
dirmgr_builder: &dyn crate::builder::DirProviderBuilder<R>,
dirmgr_extensions: tor_dirmgr::config::DirMgrExtensions,
) -> StdResult<Self, ErrorDetail> {
let dir_cfg = {
- let mut c: tor_dirmgr::DirMgrConfig = (&config).try_into()?;
+ let mut c: tor_dirmgr::DirMgrConfig = config.dir_mgr_config(mistrust.clone())?;
c.extensions = dirmgr_extensions;
c
};
- let statemgr = FsStateMgr::from_path(config.storage.expand_state_dir()?)?;
+ let statemgr =
+ FsStateMgr::from_path_and_mistrust(config.storage.expand_state_dir()?, &mistrust)?;
let addr_cfg = config.address_filter.clone();
let (status_sender, status_receiver) = postage::watch::channel();
@@ -415,6 +420,7 @@ impl<R: Runtime> TorClient<R> {
should_bootstrap: autobootstrap,
periodic_task_handles,
dormant: Arc::new(AtomicBool::new(false)),
+ fs_mistrust: mistrust,
})
}
@@ -543,7 +549,9 @@ impl<R: Runtime> TorClient<R> {
_ => {}
}
- let dir_cfg = new_config.try_into().map_err(wrap_err)?;
+ let dir_cfg = new_config
+ .dir_mgr_config(self.fs_mistrust.clone())
+ .map_err(wrap_err)?;
let state_cfg = new_config.storage.expand_state_dir().map_err(wrap_err)?;
let addr_cfg = &new_config.address_filter;
let timeout_cfg = &new_config.stream_timeouts;
diff --git a/crates/arti-client/src/config.rs b/crates/arti-client/src/config.rs
index 8ceb52b2b8920bcc9c3f659080d6a6e12e888d1b..2ec4907cbd14471765ba300c9ad882e050df182e 100644
--- a/crates/arti-client/src/config.rs
+++ b/crates/arti-client/src/config.rs
@@ -321,20 +321,19 @@ impl TorClientConfig {
pub fn builder() -> TorClientConfigBuilder {
TorClientConfigBuilder::default()
}
-}
-
-impl TryInto<dir::DirMgrConfig> for &TorClientConfig {
- type Error = ConfigBuildError;
+ /// Try to create a DirMgrConfig corresponding to this object.
#[rustfmt::skip]
- fn try_into(self) -> Result<dir::DirMgrConfig, ConfigBuildError> {
+ pub(crate) fn dir_mgr_config(&self, mistrust: fs_mistrust::Mistrust) -> Result<dir::DirMgrConfig, ConfigBuildError> {
Ok(dir::DirMgrConfig {
network: self.tor_network .clone(),
schedule: self.download_schedule .clone(),
cache_path: self.storage.expand_cache_dir()?,
+ cache_trust: mistrust,
override_net_params: self.override_net_params.clone(),
extensions: Default::default(),
})
+
}
}
@@ -356,6 +355,21 @@ impl TorClientConfigBuilder {
}
}
+/// Return a default value for our fs_mistrust configuration.
+///
+/// This is based on the environment rather on the configuration, since we may
+/// want to use it to determine whether configuration files are safe to read.
+//
+// TODO: This function should probably go away and become part of our storage
+// configuration code, once we have made Mistrust configurable via Deserialize.
+pub(crate) fn default_fs_mistrust() -> fs_mistrust::Mistrust {
+ let mut mistrust = fs_mistrust::Mistrust::new();
+ if std::env::var_os("ARTI_FS_DISABLE_PERMISSION_CHECKS").is_some() {
+ mistrust.dangerously_trust_everyone();
+ }
+ mistrust
+}
+
#[cfg(test)]
mod test {
#![allow(clippy::unwrap_used)]
diff --git a/crates/arti/Cargo.toml b/crates/arti/Cargo.toml
index c05f405ad852581983fe2a2cf894f9d982418e4a..80895940d49bf0d4a16b94782da7ef1e26be22b6 100644
--- a/crates/arti/Cargo.toml
+++ b/crates/arti/Cargo.toml
@@ -24,6 +24,7 @@ journald = ["tracing-journald"]
[dependencies]
safelog = { path = "../safelog", version = "0.1.0" }
+fs-mistrust = { path = "../fs-mistrust", version = "0.1.0" }
arti-client = { package = "arti-client", path = "../arti-client", version = "0.3.0", default-features = false }
tor-config = { path = "../tor-config", version = "0.3.0"}
tor-error = { path = "../tor-error", version = "0.3.0", default-features = false }
diff --git a/crates/arti/src/lib.rs b/crates/arti/src/lib.rs
index 302a641d5eccce7f926817430f682317209a13ce..eb48c2c228e977aba96ba4ffd74f62ad952b5ed1 100644
--- a/crates/arti/src/lib.rs
+++ b/crates/arti/src/lib.rs
@@ -152,15 +152,21 @@ pub async fn run<R: Runtime>(
config_sources: arti_config::ConfigurationSources,
arti_config: ArtiConfig,
client_config: TorClientConfig,
+ fs_mistrust_disabled: bool,
) -> Result<()> {
// Using OnDemand arranges that, while we are bootstrapping, incoming connections wait
// for bootstrap to complete, rather than getting errors.
use arti_client::BootstrapBehavior::OnDemand;
use futures::FutureExt;
- let client = TorClient::with_runtime(runtime.clone())
+ let mut client_builder = TorClient::with_runtime(runtime.clone())
.config(client_config)
- .bootstrap_behavior(OnDemand)
- .create_unbootstrapped()?;
+ .bootstrap_behavior(OnDemand);
+ if fs_mistrust_disabled {
+ client_builder = client_builder.disable_fs_permission_checks();
+ } else {
+ client_builder = client_builder.enable_fs_permission_checks();
+ }
+ let client = client_builder.create_unbootstrapped()?;
if arti_config.application().watch_configuration {
watch_cfg::watch_for_config_changes(config_sources, arti_config, client.clone())?;
}
@@ -205,6 +211,15 @@ pub async fn run<R: Runtime>(
)
}
+/// Return true if the environment has been set up to disable FS mistrust.
+//
+// TODO(nickm): This is duplicate logic from arti_client config. When we make
+// fs_mistrust configurable via deserialize, as a real part of our configuration
+// logic, we should unify all this code.
+fn fs_mistrust_disabled_via_env() -> bool {
+ std::env::var_os("ARTI_FS_DISABLE_PERMISSION_CHECKS").is_some()
+}
+
/// Inner function to allow convenient error handling
///
/// # Panics
@@ -259,6 +274,14 @@ pub fn main_main() -> Result<()> {
.value_name("LEVEL")
.help("Override the log level (usually one of 'trace', 'debug', 'info', 'warn', 'error')."),
)
+ .arg(
+ Arg::with_name("disable-fs-permission-checks")
+ .long("disable-fs-permission-checks")
+ .takes_value(false)
+ .value_name("FILE")
+ .global(true)
+ .help("Don't check permissions on the files we use."),
+ )
.subcommand(
SubCommand::with_name("proxy")
.about(
@@ -282,16 +305,38 @@ pub fn main_main() -> Result<()> {
.setting(AppSettings::SubcommandRequiredElseHelp)
.get_matches();
+ let fs_mistrust_disabled =
+ fs_mistrust_disabled_via_env() | matches.is_present("disable-fs-permission-checks");
+
+ let mistrust = {
+ // TODO: This is duplicate code from arti_client::config. When we make
+ // fs_mistrust configurable via deserialize, as a real part of our configuration
+ // logic, we should unify this check.
+ let mut mistrust = fs_mistrust::Mistrust::new();
+ if fs_mistrust_disabled {
+ mistrust.dangerously_trust_everyone();
+ }
+ mistrust
+ };
+
let cfg_sources = {
let mut cfg_sources = arti_config::ConfigurationSources::new();
let config_files = matches.values_of_os("config-files").unwrap_or_default();
if config_files.len() == 0 {
if let Some(default) = default_config_file() {
+ match mistrust.verifier().require_file().check(&default) {
+ Ok(()) => {}
+ Err(fs_mistrust::Error::NotFound(_)) => {}
+ Err(e) => return Err(e.into()),
+ }
cfg_sources.push_optional_file(default);
}
} else {
- config_files.for_each(|f| cfg_sources.push_file(f));
+ for f in config_files {
+ mistrust.verifier().require_file().check(f)?;
+ cfg_sources.push_file(f);
+ }
}
matches
@@ -356,6 +401,7 @@ pub fn main_main() -> Result<()> {
cfg_sources,
config,
client_config,
+ fs_mistrust_disabled,
))?;
Ok(())
} else {
diff --git a/crates/fs-mistrust/src/dir.rs b/crates/fs-mistrust/src/dir.rs
index f799622633b7a883ab42bb4783ed6ae9d3749afe..28f9e0efac976e84aa813dd5af36e11d656841df 100644
--- a/crates/fs-mistrust/src/dir.rs
+++ b/crates/fs-mistrust/src/dir.rs
@@ -3,6 +3,7 @@
use std::{
fs::{File, OpenOptions},
+ io::{Read, Write},
path::{Path, PathBuf},
};
@@ -30,6 +31,7 @@ use std::os::unix::fs::OpenOptionsExt;
/// APIs.
///
/// See also the crate-level [Limitations](crate#limitations) section.
+#[derive(Debug, Clone)]
pub struct CheckedDir {
/// The `Mistrust` object whose rules we apply to members of this directory.
mistrust: Mistrust,
@@ -102,9 +104,7 @@ impl CheckedDir {
options.custom_flags(libc::O_NOFOLLOW);
}
- let file = options
- .open(&path)
- .map_err(|e| Error::inspecting(e, &path))?;
+ let file = options.open(&path).map_err(|e| Error::io(e, &path))?;
let meta = file.metadata().map_err(|e| Error::inspecting(e, &path))?;
if let Some(error) = self
@@ -128,6 +128,90 @@ impl CheckedDir {
self.location.as_path()
}
+ /// Return a new [`PathBuf`] containing this directory's path, with `path`
+ /// appended to it.
+ ///
+ /// Return an error if `path` has any components that could take us outside
+ /// of this directory.
+ pub fn join<P: AsRef<Path>>(&self, path: P) -> Result<PathBuf> {
+ let path = path.as_ref();
+ self.check_path(path)?;
+ Ok(self.location.join(path))
+ }
+
+ /// Read the contents of the file at `path` within this directory, as a
+ /// String, if possible.
+ ///
+ /// Return an error if `path` is absent, if its permissions are incorrect,
+ /// if it has any components that could take us outside of this directory,
+ /// or if its contents are not UTF-8.
+ pub fn read_to_string<P: AsRef<Path>>(&self, path: P) -> Result<String> {
+ let path = path.as_ref();
+ let mut file = self.open(path, OpenOptions::new().read(true))?;
+ let mut result = String::new();
+ file.read_to_string(&mut result)
+ .map_err(|e| Error::io(e, path))?;
+ Ok(result)
+ }
+
+ /// Read the contents of the file at `path` within this directory, as a
+ /// vector of bytes, if possible.
+ ///
+ /// Return an error if `path` is absent, if its permissions are incorrect,
+ /// or if it has any components that could take us outside of this
+ /// directory.
+ pub fn read<P: AsRef<Path>>(&self, path: P) -> Result<Vec<u8>> {
+ let path = path.as_ref();
+ let mut file = self.open(path, OpenOptions::new().read(true))?;
+ let mut result = Vec::new();
+ file.read_to_end(&mut result)
+ .map_err(|e| Error::io(e, path))?;
+ Ok(result)
+ }
+
+ /// Store `contents` into the file located at `path` within this directory.
+ ///
+ /// We won't write to `path` directly: instead, we'll write to a temporary
+ /// file in the same directory as `path`, and then replace `path` with that
+ /// temporary file if we were successful. (This isn't truly atomic on all
+ /// file systems, but it's closer than many alternatives.)
+ ///
+ /// # Limitations
+ ///
+ /// This function will clobber any existing files with the same name as
+ /// `path` but with the extension `tmp`. (That is, if you are writing to
+ /// "foo.txt", it will replace "foo.tmp" in the same directory.)
+ ///
+ /// This function may give incorrect behavior if multiple threads or
+ /// processes are writing to the same file at the same time: it is the
+ /// programmer's responsibility to use appropriate locking to avoid this.
+ pub fn write_and_replace<P: AsRef<Path>, C: AsRef<[u8]>>(
+ &self,
+ path: P,
+ contents: C,
+ ) -> Result<()> {
+ let path = path.as_ref();
+ self.check_path(path)?;
+
+ let tmp_name = path.with_extension("tmp");
+ let mut tmp_file = self.open(
+ &tmp_name,
+ OpenOptions::new().create(true).truncate(true).write(true),
+ )?;
+
+ // Write the data.
+ tmp_file
+ .write_all(contents.as_ref())
+ .map_err(|e| Error::io(e, &tmp_name))?;
+ // Flush and close.
+ drop(tmp_file);
+
+ // Replace the old file.
+ std::fs::rename(self.location.join(tmp_name), self.location.join(path))
+ .map_err(|e| Error::io(e, path))?;
+ Ok(())
+ }
+
/// Helper: create a [`Verifier`] with the appropriate rules for this
/// `CheckedDir`.
fn verifier(&self) -> Verifier<'_> {
@@ -142,7 +226,10 @@ impl CheckedDir {
/// guaranteed to stay within this directory.
fn check_path(&self, p: &Path) -> Result<()> {
use std::path::Component;
- if p.is_absolute() {}
+ // This check should be redundant, but let's be certain.
+ if p.is_absolute() {
+ return Err(Error::InvalidSubdirectory);
+ }
for component in p.components() {
match component {
@@ -239,4 +326,42 @@ mod test {
sd.make_directory("hello/world").unwrap();
}
+
+ #[test]
+ fn read_and_write() {
+ let d = Dir::new();
+ d.dir("a");
+ d.chmod("a", 0o700);
+ let mut m = Mistrust::new();
+ m.ignore_prefix(d.canonical_root()).unwrap();
+
+ let checked = m.verifier().secure_dir(d.path("a")).unwrap();
+
+ // Simple case: write and read.
+ checked
+ .write_and_replace("foo.txt", "this is incredibly silly")
+ .unwrap();
+
+ let s1 = checked.read_to_string("foo.txt").unwrap();
+ let s2 = checked.read("foo.txt").unwrap();
+ assert_eq!(s1, "this is incredibly silly");
+ assert_eq!(s1.as_bytes(), &s2[..]);
+
+ // Trickier: write when the preferred temporary already has content.
+ checked
+ .open("bar.tmp", OpenOptions::new().create(true).write(true))
+ .unwrap()
+ .write_all("be the other guy".as_bytes())
+ .unwrap();
+ assert!(checked.join("bar.tmp").unwrap().exists());
+
+ checked
+ .write_and_replace("bar.txt", "its hard and nobody understands")
+ .unwrap();
+
+ // Temp file should be gone.
+ assert!(!checked.join("bar.tmp").unwrap().exists());
+ let s4 = checked.read_to_string("bar.txt").unwrap();
+ assert_eq!(s4, "its hard and nobody understands");
+ }
}
diff --git a/crates/fs-mistrust/src/err.rs b/crates/fs-mistrust/src/err.rs
index cf699d09cb6a59b500ff9b36791b2f6f739a16d3..79e3846f6102fb86c7cdcd97d1f727c7cceb1e0f 100644
--- a/crates/fs-mistrust/src/err.rs
+++ b/crates/fs-mistrust/src/err.rs
@@ -83,15 +83,23 @@ pub enum Error {
#[error("Unable to list directory")]
Listing(#[source] Arc<walkdir::Error>),
- /// We were unable to open a file with [`CheckedDir::open`](crate::CheckedDir::open)
-
/// Tried to use an invalid path with a [`CheckedDir`](crate::CheckedDir),
#[error("Path was not valid for use with CheckedDir.")]
InvalidSubdirectory,
+
+ /// We encountered an error while attempting an IO operation on a file.
+ #[error("IO error on {0}")]
+ Io(PathBuf, #[source] Arc<IoError>),
+
+ /// We could not create an unused temporary path when trying to write a
+ /// file.
+ #[error("Could not name temporary file for {0}")]
+ NoTempFile(PathBuf),
}
impl Error {
- /// Create an error from an IoError object.
+ /// Create an error from an IoError encountered while inspecting permissions
+ /// on an object.
pub(crate) fn inspecting(err: IoError, fname: impl Into<PathBuf>) -> Self {
match err.kind() {
IoErrorKind::NotFound => Error::NotFound(fname.into()),
@@ -99,6 +107,15 @@ impl Error {
}
}
+ /// Create an error from an IoError encountered while performing IO (open,
+ /// read, write) on an object.
+ pub(crate) fn io(err: IoError, fname: impl Into<PathBuf>) -> Self {
+ match err.kind() {
+ IoErrorKind::NotFound => Error::NotFound(fname.into()),
+ _ => Error::Io(fname.into(), Arc::new(err)),
+ }
+ }
+
/// Return the path, if any, associated with this error.
pub fn path(&self) -> Option<&Path> {
Some(
@@ -108,6 +125,8 @@ impl Error {
Error::BadOwner(pb, _) => pb,
Error::BadType(pb) => pb,
Error::CouldNotInspect(pb, _) => pb,
+ Error::Io(pb, _) => pb,
+ Error::NoTempFile(pb) => pb,
Error::Multiple(_) => return None,
Error::StepsExceeded => return None,
Error::CurrentDirectory(_) => return None,
@@ -120,6 +139,30 @@ impl Error {
)
}
+ /// Return true iff this error indicates a problem with filesystem
+ /// permissions.
+ ///
+ /// (Other errors typically indicate an IO problem, possibly one preventing
+ /// us from looking at permissions in the first place)
+ pub fn is_bad_permission(&self) -> bool {
+ match self {
+ Error::BadPermission(_, _) | Error::BadOwner(_, _) | Error::BadType(_) => true,
+
+ Error::NotFound(_)
+ | Error::CouldNotInspect(_, _)
+ | Error::StepsExceeded
+ | Error::CurrentDirectory(_)
+ | Error::CreatingDir(_)
+ | Error::Listing(_)
+ | Error::InvalidSubdirectory
+ | Error::Io(_, _)
+ | Error::NoTempFile(_) => false,
+
+ Error::Multiple(errs) => errs.iter().any(|e| e.is_bad_permission()),
+ Error::Content(err) => err.is_bad_permission(),
+ }
+ }
+
/// Return an iterator over all of the errors contained in this Error.
///
/// If this is a singleton, the iterator returns only a single element.
diff --git a/crates/fs-mistrust/src/imp.rs b/crates/fs-mistrust/src/imp.rs
index 462830ff33f81cccd6a7f209c398f4c7b5b20c58..d960d4d4f91575709deac9f11401c38214723ad1 100644
--- a/crates/fs-mistrust/src/imp.rs
+++ b/crates/fs-mistrust/src/imp.rs
@@ -182,18 +182,21 @@ impl<'a> super::Verifier<'a> {
if uid != 0 && Some(uid) != self.mistrust.trust_uid {
errors.push(Error::BadOwner(path.into(), uid));
}
- let mut forbidden_bits = if !self.readable_okay
- && (path_type == PathType::Final || path_type == PathType::Content)
- {
- // If this is the target or a content object, and it must not be
- // readable, then we forbid it to be group-rwx and all-rwx.
+ let mut forbidden_bits = if !self.readable_okay && path_type == PathType::Final {
+ // If this is the target object, and it must not be readable, then
+ // we forbid it to be group-rwx and all-rwx.
+ //
+ // (We allow _content_ to be globally readable even if readable_okay
+ // is false, since we check that the Final directory is itself
+ // unreadable. This is okay unless the content has hard links: see
+ // the Limitations section of the crate-level documentation.)
0o077
} else {
- // If this is the target object and it may be readable, or if
- // this is _any parent directory_, then we typically forbid the
- // group-write and all-write bits. (Those are the bits that
- // would allow non-trusted users to change the object, or change
- // things around in a directory.)
+ // If this is the target object and it may be readable, or if this
+ // is _any parent directory_ or any content, then we typically
+ // forbid the group-write and all-write bits. (Those are the bits
+ // that would allow non-trusted users to change the object, or
+ // change things around in a directory.)
if meta.is_dir() && meta.mode() & STICKY_BIT != 0 && path_type == PathType::Intermediate
{
// This is an intermediate directory and this sticky bit is
diff --git a/crates/fs-mistrust/src/lib.rs b/crates/fs-mistrust/src/lib.rs
index bc1f3b46254de7512142ef4e0869f2feb080252e..d1ed1529a85c529baa929d0d64904fd73950c11b 100644
--- a/crates/fs-mistrust/src/lib.rs
+++ b/crates/fs-mistrust/src/lib.rs
@@ -200,6 +200,17 @@
//! * SELinux capabilities
//! * POSIX (and other) ACLs.
//!
+//! We use a somewhat inaccurate heuristic when we're checking the permissions
+//! of items _inside_ a target directory (using [`Verifier::check_content`] or
+//! [`CheckedDir`]): we continue to forbid untrusted-writeable directories and
+//! files, but we still allow readable ones, even if we insisted that the target
+//! directory itself was required to to be unreadable. This is too permissive
+//! in the case of readable objects with hard links: if there is a hard link to
+//! the file somewhere else, then an untrusted user can read it. It is also too
+//! restrictive in the case of writeable objects _without_ hard links: if
+//! untrusted users have no path to those objects, they can't actually write
+//! them.
+//!
//! On Windows, we accept all file permissions and owners.
//!
//! We don't check for mount-points and the privacy of filesystem devices
@@ -232,7 +243,6 @@
// POSSIBLY TODO:
// - Cache information across runs.
-// - Add a way to recursively check the contents of a directory.
#![deny(missing_docs)]
#![warn(noop_method_call)]
@@ -907,7 +917,7 @@ mod test {
d.chmod("a", 0o700);
d.chmod("a/b", 0o700);
d.chmod("a/b/c", 0o755);
- d.chmod("a/b/c/d", 0o644);
+ d.chmod("a/b/c/d", 0o666);
let mut m = Mistrust::new();
m.ignore_prefix(d.canonical_root()).unwrap();
@@ -915,15 +925,18 @@ mod test {
// A check should work...
m.check_directory(d.path("a/b")).unwrap();
- // But we get errors if we check the contents.
+ // But we get an error if we check the contents.
let e = m
.verifier()
.all_errors()
.check_content()
.check(d.path("a/b"))
.unwrap_err();
+ assert_eq!(1, e.errors().count());
- assert_eq!(2, e.errors().count());
+ // We only expect an error on the _writable_ contents: the _readable_
+ // a/b/c is okay.
+ assert_eq!(e.path().unwrap(), d.path("a/b/c/d"));
}
#[test]
diff --git a/crates/tor-dirmgr/Cargo.toml b/crates/tor-dirmgr/Cargo.toml
index eeb967bac4ccbd74ca1bfd546e9cd532e960985e..aa316d7985b85f4de43b1d1ea36ee5617515ab62 100644
--- a/crates/tor-dirmgr/Cargo.toml
+++ b/crates/tor-dirmgr/Cargo.toml
@@ -26,6 +26,7 @@ dirfilter = []
experimental-api = []
[dependencies]
+fs-mistrust = { path = "../fs-mistrust", version = "0.1.0" }
tor-basic-utils = { path = "../tor-basic-utils", version = "0.3.0"}
retry-error = { path = "../retry-error", version = "0.2.0"}
tor-checkable = { path = "../tor-checkable", version = "0.3.0"}
diff --git a/crates/tor-dirmgr/src/config.rs b/crates/tor-dirmgr/src/config.rs
index 65b984b4c953cd3e243948119a23353ebc95990d..fb7b3b63ddd870522cb60a85fb36db147852c489 100644
--- a/crates/tor-dirmgr/src/config.rs
+++ b/crates/tor-dirmgr/src/config.rs
@@ -189,6 +189,9 @@ pub struct DirMgrConfig {
/// Cannot be changed on a running Arti client.
pub cache_path: PathBuf,
+ /// Rules for whether to trust the the permissions on the cache_path.
+ pub cache_trust: fs_mistrust::Mistrust,
+
/// Configuration information about the network.
pub network: NetworkConfig,
@@ -228,10 +231,13 @@ impl DirMgrConfig {
/// Note that each time this is called, a new store object will be
/// created: you probably only want to call this once.
pub(crate) fn open_store(&self, readonly: bool) -> Result<DynStore> {
- Ok(Box::new(crate::storage::SqliteStore::from_path(
- &self.cache_path,
- readonly,
- )?))
+ Ok(Box::new(
+ crate::storage::SqliteStore::from_path_and_mistrust(
+ &self.cache_path,
+ &self.cache_trust,
+ readonly,
+ )?,
+ ))
}
/// Return a slice of the configured authorities
@@ -251,6 +257,7 @@ impl DirMgrConfig {
pub(crate) fn update_from_config(&self, new_config: &DirMgrConfig) -> DirMgrConfig {
DirMgrConfig {
cache_path: self.cache_path.clone(),
+ cache_trust: self.cache_trust.clone(),
network: NetworkConfig {
fallback_caches: new_config.network.fallback_caches.clone(),
authorities: self.network.authorities.clone(),
diff --git a/crates/tor-dirmgr/src/err.rs b/crates/tor-dirmgr/src/err.rs
index a7f16297b78f9da7d7df1a9794585c2a3f284f97..633eb7577c630050fcf9d727c45535a223468399 100644
--- a/crates/tor-dirmgr/src/err.rs
+++ b/crates/tor-dirmgr/src/err.rs
@@ -83,6 +83,9 @@ pub enum Error {
/// An attempt was made to bootstrap a `DirMgr` created in offline mode.
#[error("cannot bootstrap offline DirMgr")]
OfflineMode,
+ /// A problem with file permissions on our cache directory.
+ #[error("Bad permissions in cache directory")]
+ CachePermissions(#[from] fs_mistrust::Error),
/// Unable to spawn task
#[error("unable to spawn {spawning}")]
@@ -154,6 +157,7 @@ impl Error {
// These errors cannot come from a directory cache.
Error::NoDownloadSupport
| Error::CacheCorruption(_)
+ | Error::CachePermissions(_)
| Error::SqliteError(_)
| Error::UnrecognizedSchema
| Error::BadNetworkConfig(_)
@@ -209,6 +213,13 @@ impl HasKind for Error {
E::Unwanted(_) => EK::TorProtocolViolation,
E::NoDownloadSupport => EK::NotImplemented,
E::CacheCorruption(_) => EK::CacheCorrupted,
+ E::CachePermissions(e) => {
+ if e.is_bad_permission() {
+ EK::FsPermissions
+ } else {
+ EK::CacheAccessFailed
+ }
+ }
E::SqliteError(e) => sqlite_error_kind(e),
E::UnrecognizedSchema => EK::CacheCorrupted,
E::BadNetworkConfig(_) => EK::InvalidConfig,
diff --git a/crates/tor-dirmgr/src/state.rs b/crates/tor-dirmgr/src/state.rs
index 46701bddb1acb035d56109d9a953d129108b5890..a53087aba4c20b65f39c0fd511550625873e7ecc 100644
--- a/crates/tor-dirmgr/src/state.rs
+++ b/crates/tor-dirmgr/src/state.rs
@@ -995,7 +995,12 @@ mod test {
fn temp_store() -> (TempDir, Mutex<DynStore>) {
let tempdir = TempDir::new().unwrap();
- let store = crate::storage::SqliteStore::from_path(tempdir.path(), false).unwrap();
+ let store = crate::storage::SqliteStore::from_path_and_mistrust(
+ tempdir.path(),
+ fs_mistrust::Mistrust::new().dangerously_trust_everyone(),
+ false,
+ )
+ .unwrap();
(tempdir, Mutex::new(Box::new(store)))
}
diff --git a/crates/tor-dirmgr/src/storage.rs b/crates/tor-dirmgr/src/storage.rs
index 0359f8e841613ad8595d666368be669cfc8a75b2..860d2b4fbbc7f8013b6e15194f3ae97f1354738e 100644
--- a/crates/tor-dirmgr/src/storage.rs
+++ b/crates/tor-dirmgr/src/storage.rs
@@ -17,8 +17,9 @@ use crate::docmeta::{AuthCertMeta, ConsensusMeta};
use crate::{Error, Result};
use std::cell::RefCell;
use std::collections::HashMap;
+use std::fs::File;
+use std::str::Utf8Error;
use std::time::SystemTime;
-use std::{path::Path, str::Utf8Error};
use time::Duration;
pub(crate) mod sqlite;
@@ -124,18 +125,15 @@ impl InputString {
}
}
}
-
- /// Construct a new InputString from a file on disk, trying to
- /// memory-map the file if possible.
- pub(crate) fn load<P: AsRef<Path>>(path: P) -> Result<Self> {
- let f = std::fs::File::open(path)?;
+ /// DOCDOC
+ pub(crate) fn load(file: File) -> Result<Self> {
#[cfg(feature = "mmap")]
{
let mapping = unsafe {
// I'd rather have a safe option, but that's not possible
// with mmap, since other processes could in theory replace
// the contents of the file while we're using it.
- memmap2::Mmap::map(&f)
+ memmap2::Mmap::map(&file)
};
if let Ok(bytes) = mapping {
return Ok(InputString::MappedBytes {
@@ -145,7 +143,7 @@ impl InputString {
}
}
use std::io::{BufReader, Read};
- let mut f = BufReader::new(f);
+ let mut f = BufReader::new(file);
let mut result = String::new();
f.read_to_string(&mut result)?;
Ok(InputString::Utf8(result))
@@ -308,13 +306,9 @@ mod test {
fn files() {
let td = tempdir().unwrap();
- let absent = td.path().join("absent");
- let s = InputString::load(&absent);
- assert!(s.is_err());
-
let goodstr = td.path().join("goodstr");
std::fs::write(&goodstr, "This is a reasonable file.\n").unwrap();
- let s = InputString::load(&goodstr);
+ let s = InputString::load(File::open(goodstr).unwrap());
let s = s.unwrap();
assert_eq!(s.as_str().unwrap(), "This is a reasonable file.\n");
assert_eq!(s.as_str().unwrap(), "This is a reasonable file.\n");
@@ -322,7 +316,7 @@ mod test {
let badutf8 = td.path().join("badutf8");
std::fs::write(&badutf8, b"Not good \xff UTF-8.\n").unwrap();
- let s = InputString::load(&badutf8);
+ let s = InputString::load(File::open(badutf8).unwrap());
assert!(s.is_err() || s.unwrap().as_str().is_err());
}
diff --git a/crates/tor-dirmgr/src/storage/sqlite.rs b/crates/tor-dirmgr/src/storage/sqlite.rs
index 41337db5b23a168d0a0ae11e90bd1997fdc20ad2..4bc8b0d1aa22c6ed64061e2336ed9a171d5b29e4 100644
--- a/crates/tor-dirmgr/src/storage/sqlite.rs
+++ b/crates/tor-dirmgr/src/storage/sqlite.rs
@@ -8,6 +8,7 @@ use crate::docmeta::{AuthCertMeta, ConsensusMeta};
use crate::storage::{InputString, Store};
use crate::{Error, Result};
+use fs_mistrust::CheckedDir;
use tor_netdoc::doc::authcert::AuthCertKeyIds;
use tor_netdoc::doc::microdesc::MdDigest;
use tor_netdoc::doc::netstatus::{ConsensusFlavor, Lifetime};
@@ -15,16 +16,14 @@ use tor_netdoc::doc::netstatus::{ConsensusFlavor, Lifetime};
use tor_netdoc::doc::routerdesc::RdDigest;
use std::collections::HashMap;
-use std::path::{self, Path, PathBuf};
+use std::fs::OpenOptions;
+use std::path::{Path, PathBuf};
use std::time::SystemTime;
use rusqlite::{params, OpenFlags, OptionalExtension, Transaction};
use time::OffsetDateTime;
use tracing::trace;
-#[cfg(target_family = "unix")]
-use std::os::unix::fs::DirBuilderExt;
-
/// Local directory cache using a Sqlite3 connection.
pub(crate) struct SqliteStore {
/// Connection to the sqlite3 database.
@@ -32,7 +31,7 @@ pub(crate) struct SqliteStore {
/// Location for the sqlite3 database; used to reopen it.
sql_path: Option<PathBuf>,
/// Location to store blob files.
- path: PathBuf,
+ blob_dir: CheckedDir,
/// Lockfile to prevent concurrent write attempts from different
/// processes.
///
@@ -61,19 +60,36 @@ impl SqliteStore {
/// _per process_. Therefore, you might get unexpected results if
/// two SqliteStores are created in the same process with the
/// path.
- pub(crate) fn from_path<P: AsRef<Path>>(path: P, mut readonly: bool) -> Result<Self> {
+ pub(crate) fn from_path_and_mistrust<P: AsRef<Path>>(
+ path: P,
+ mistrust: &fs_mistrust::Mistrust,
+ mut readonly: bool,
+ ) -> Result<Self> {
let path = path.as_ref();
let sqlpath = path.join("dir.sqlite3");
let blobpath = path.join("dir_blobs/");
let lockpath = path.join("dir.lock");
- if !readonly {
- let mut builder = std::fs::DirBuilder::new();
- #[cfg(target_family = "unix")]
- builder.mode(0o700);
- builder.recursive(true).create(&blobpath).map_err(|err| {
- Error::StorageError(format!("Creating directory at {:?}: {}", &blobpath, err))
- })?;
+ let verifier = mistrust.verifier().permit_readable().check_content();
+
+ let blob_dir = if readonly {
+ verifier.secure_dir(blobpath)?
+ } else {
+ verifier.make_secure_dir(blobpath)?
+ };
+
+ // Check permissions on the sqlite and lock files; don't require them to
+ // exist.
+ for p in [&lockpath, &sqlpath] {
+ match mistrust
+ .verifier()
+ .permit_readable()
+ .require_file()
+ .check(p)
+ {
+ Ok(()) | Err(fs_mistrust::Error::NotFound(_)) => {}
+ Err(e) => return Err(e.into()),
+ }
}
let mut lockfile = fslock::LockFile::open(&lockpath)?;
@@ -86,7 +102,7 @@ impl SqliteStore {
OpenFlags::SQLITE_OPEN_READ_WRITE | OpenFlags::SQLITE_OPEN_CREATE
};
let conn = rusqlite::Connection::open_with_flags(&sqlpath, flags)?;
- let mut store = SqliteStore::from_conn(conn, &blobpath)?;
+ let mut store = SqliteStore::from_conn(conn, blob_dir)?;
store.sql_path = Some(sqlpath);
store.lockfile = Some(lockfile);
Ok(store)
@@ -96,14 +112,10 @@ impl SqliteStore {
/// for blob files.
///
/// Used for testing with a memory-backed database.
- pub(crate) fn from_conn<P>(conn: rusqlite::Connection, path: P) -> Result<Self>
- where
- P: AsRef<Path>,
- {
- let path = path.as_ref().to_path_buf();
+ pub(crate) fn from_conn(conn: rusqlite::Connection, blob_dir: CheckedDir) -> Result<Self> {
let mut result = SqliteStore {
conn,
- path,
+ blob_dir,
lockfile: None,
sql_path: None,
};
@@ -153,36 +165,21 @@ impl SqliteStore {
Ok(())
}
- /// Return the correct filename for a given blob, based on the filename
- /// from the ExtDocs table.
- fn blob_fname<P>(&self, path: P) -> Result<PathBuf>
- where
- P: AsRef<Path>,
- {
- let path = path.as_ref();
- if !path
- .components()
- .all(|c| matches!(c, path::Component::Normal(_)))
- {
- return Err(Error::CacheCorruption("Invalid path in database"));
- }
-
- let mut result = self.path.clone();
- result.push(path);
- Ok(result)
- }
-
/// Read a blob from disk, mapping it if possible.
fn read_blob<P>(&self, path: P) -> Result<InputString>
where
P: AsRef<Path>,
{
let path = path.as_ref();
- let full_path = self.blob_fname(path)?;
- InputString::load(&full_path).map_err(|err| {
+
+ let file = self.blob_dir.open(path, OpenOptions::new().read(true))?;
+
+ InputString::load(file).map_err(|err| {
Error::StorageError(format!(
"Loading blob {:?} from storage at {:?}: {}",
- path, full_path, err
+ path,
+ self.blob_dir.as_path().join(path),
+ err
))
})
}
@@ -202,10 +199,10 @@ impl SqliteStore {
let digest = hex::encode(digest);
let digeststr = format!("{}-{}", dtype, digest);
let fname = format!("{}_{}", doctype, digeststr);
- let full_path = self.blob_fname(&fname)?;
+ let full_path = self.blob_dir.join(&fname)?;
let unlinker = Unlinker::new(&full_path);
- std::fs::write(full_path, contents)?;
+ self.blob_dir.write_and_replace(&fname, contents)?;
let tx = self.conn.unchecked_transaction()?;
tx.execute(INSERT_EXTDOC, params![digeststr, expires, dtype, fname])?;
@@ -301,7 +298,7 @@ impl Store for SqliteStore {
tx.execute(DROP_OLD_ROUTERDESCS, [now - expiration.router_descs])?;
tx.commit()?;
for name in expired_blobs {
- let fname = self.blob_fname(name);
+ let fname = self.blob_dir.join(name);
if let Ok(fname) = fname {
let _ignore = std::fs::remove_file(fname);
}
@@ -864,7 +861,12 @@ mod test {
let tmp_dir = tempdir().unwrap();
let sql_path = tmp_dir.path().join("db.sql");
let conn = rusqlite::Connection::open(&sql_path)?;
- let store = SqliteStore::from_conn(conn, &tmp_dir)?;
+ let blob_dir = fs_mistrust::Mistrust::new()
+ .dangerously_trust_everyone()
+ .verifier()
+ .secure_dir(&tmp_dir)
+ .unwrap();
+ let store = SqliteStore::from_conn(conn, blob_dir)?;
Ok((tmp_dir, store))
}
@@ -872,46 +874,50 @@ mod test {
#[test]
fn init() -> Result<()> {
let tmp_dir = tempdir().unwrap();
+ let blob_dir = fs_mistrust::Mistrust::new()
+ .dangerously_trust_everyone()
+ .verifier()
+ .secure_dir(&tmp_dir)
+ .unwrap();
let sql_path = tmp_dir.path().join("db.sql");
// Initial setup: everything should work.
{
let conn = rusqlite::Connection::open(&sql_path)?;
- let _store = SqliteStore::from_conn(conn, &tmp_dir)?;
+ let _store = SqliteStore::from_conn(conn, blob_dir.clone())?;
}
// Second setup: shouldn't need to upgrade.
{
let conn = rusqlite::Connection::open(&sql_path)?;
- let _store = SqliteStore::from_conn(conn, &tmp_dir)?;
+ let _store = SqliteStore::from_conn(conn, blob_dir.clone())?;
}
// Third setup: shouldn't need to upgrade.
{
let conn = rusqlite::Connection::open(&sql_path)?;
conn.execute_batch("UPDATE TorSchemaMeta SET version = 9002;")?;
- let _store = SqliteStore::from_conn(conn, &tmp_dir)?;
+ let _store = SqliteStore::from_conn(conn, blob_dir.clone())?;
}
// Fourth: this says we can't read it, so we'll get an error.
{
let conn = rusqlite::Connection::open(&sql_path)?;
conn.execute_batch("UPDATE TorSchemaMeta SET readable_by = 9001;")?;
- let val = SqliteStore::from_conn(conn, &tmp_dir);
+ let val = SqliteStore::from_conn(conn, blob_dir);
assert!(val.is_err());
}
Ok(())
}
#[test]
- fn bad_blob_fnames() -> Result<()> {
+ fn bad_blob_fname() -> Result<()> {
let (_tmp_dir, store) = new_empty()?;
- assert!(store.blob_fname("abcd").is_ok());
- assert!(store.blob_fname("abcd..").is_ok());
- assert!(store.blob_fname("..abcd..").is_ok());
- assert!(store.blob_fname(".abcd").is_ok());
+ assert!(store.blob_dir.join("abcd").is_ok());
+ assert!(store.blob_dir.join("abcd..").is_ok());
+ assert!(store.blob_dir.join("..abcd..").is_ok());
+ assert!(store.blob_dir.join(".abcd").is_ok());
- assert!(store.blob_fname(".").is_err());
- assert!(store.blob_fname("..").is_err());
- assert!(store.blob_fname("../abcd").is_err());
- assert!(store.blob_fname("/abcd").is_err());
+ assert!(store.blob_dir.join("..").is_err());
+ assert!(store.blob_dir.join("../abcd").is_err());
+ assert!(store.blob_dir.join("/abcd").is_err());
Ok(())
}
@@ -943,13 +949,13 @@ mod test {
fname1,
"greeting_sha1-7b502c3a1f48c8609ae212cdfb639dee39673f5e"
);
- assert_eq!(store.blob_fname(&fname1)?, tmp_dir.path().join(&fname1));
+ assert_eq!(store.blob_dir.join(&fname1)?, tmp_dir.path().join(&fname1));
assert_eq!(
- &std::fs::read(store.blob_fname(&fname1)?)?[..],
+ &std::fs::read(store.blob_dir.join(&fname1)?)?[..],
b"Hello world"
);
assert_eq!(
- &std::fs::read(store.blob_fname(&fname2)?)?[..],
+ &std::fs::read(store.blob_dir.join(&fname2)?)?[..],
b"Goodbye, dear friends"
);
@@ -964,10 +970,10 @@ mod test {
// Now expire: the second file should go away.
store.expire_all(&EXPIRATION_DEFAULTS)?;
assert_eq!(
- &std::fs::read(store.blob_fname(&fname1)?)?[..],
+ &std::fs::read(store.blob_dir.join(&fname1)?)?[..],
b"Hello world"
);
- assert!(std::fs::read(store.blob_fname(&fname2)?).is_err());
+ assert!(std::fs::read(store.blob_dir.join(&fname2)?).is_err());
let n: u32 = store
.conn
.query_row("SELECT COUNT(filename) FROM ExtDocs", [], |row| row.get(0))?;
@@ -1182,15 +1188,20 @@ mod test {
#[test]
fn from_path_rw() -> Result<()> {
let tmp = tempdir().unwrap();
+ let mistrust = {
+ let mut m = fs_mistrust::Mistrust::new();
+ m.dangerously_trust_everyone();
+ m
+ };
// Nothing there: can't open read-only
- let r = SqliteStore::from_path(tmp.path(), true);
+ let r = SqliteStore::from_path_and_mistrust(tmp.path(), &mistrust, true);
assert!(r.is_err());
assert!(!tmp.path().join("dir_blobs").exists());
// Opening it read-write will crate the files
{
- let mut store = SqliteStore::from_path(tmp.path(), false)?;
+ let mut store = SqliteStore::from_path_and_mistrust(tmp.path(), &mistrust, false)?;
assert!(tmp.path().join("dir_blobs").is_dir());
assert!(store.lockfile.is_some());
assert!(!store.is_readonly());
@@ -1199,7 +1210,7 @@ mod test {
// At this point, we can successfully make a read-only connection.
{
- let mut store2 = SqliteStore::from_path(tmp.path(), true)?;
+ let mut store2 = SqliteStore::from_path_and_mistrust(tmp.path(), &mistrust, true)?;
assert!(store2.is_readonly());
// Nobody else is locking this, so we can upgrade.
diff --git a/crates/tor-error/src/lib.rs b/crates/tor-error/src/lib.rs
index bbd371741354467da6867f46676a50aec3b86e12..0731d12fd660164d9d1549b7ee18305623d3a794 100644
--- a/crates/tor-error/src/lib.rs
+++ b/crates/tor-error/src/lib.rs
@@ -158,6 +158,15 @@ pub enum ErrorKind {
#[display(fmt = "could not read/write persistent state")]
PersistentStateAccessFailed,
+ /// We encountered a problem with filesystem permissions.
+ ///
+ /// This is likeliest to be caused by permissions on a file or directory
+ /// being too permissive; the next likeliest cause is that we were unable to
+ /// check the permissions on the file or directory, or on one of its
+ /// ancestors.
+ #[display(fmt = "problem with filesystem permissions")]
+ FsPermissions,
+
/// Tor client's persistent state has been corrupted
///
/// This could be because of a bug in the Tor code, or because something
diff --git a/crates/tor-persist/Cargo.toml b/crates/tor-persist/Cargo.toml
index d54eb496139e3b9f0eaea3431eae07612ce9ae40..22e9fcfd377b5363365e432dd4eb09d5718e96b4 100644
--- a/crates/tor-persist/Cargo.toml
+++ b/crates/tor-persist/Cargo.toml
@@ -17,6 +17,7 @@ repository = "https://gitlab.torproject.org/tpo/core/arti.git/"
testing = []
[dependencies]
+fs-mistrust = { path = "../fs-mistrust", version = "0.1.0", features = ["walkdir"] }
serde = { version = "1.0.103", features = ["derive"] }
serde_json = "1.0.50"
sanitize-filename = "0.3.0"
diff --git a/crates/tor-persist/src/fs.rs b/crates/tor-persist/src/fs.rs
index 91f999932e74c4812cf7ca9084eab64b47d88c89..82455c561d114495c7bfaf81600c6b15fbc609ef 100644
--- a/crates/tor-persist/src/fs.rs
+++ b/crates/tor-persist/src/fs.rs
@@ -4,15 +4,13 @@ mod clean;
use crate::{load_error, store_error};
use crate::{Error, LockStatus, Result, StateMgr};
+use fs_mistrust::CheckedDir;
use serde::{de::DeserializeOwned, Serialize};
use std::path::{Path, PathBuf};
use std::sync::{Arc, Mutex};
use std::time::SystemTime;
use tracing::{info, warn};
-#[cfg(target_family = "unix")]
-use std::os::unix::fs::DirBuilderExt;
-
/// Implementation of StateMgr that stores state as JSON files on disk.
///
/// # Locking
@@ -48,7 +46,7 @@ pub struct FsStateMgr {
#[derive(Debug)]
struct FsStateMgrInner {
/// Directory in which we store state files.
- statepath: PathBuf,
+ statepath: CheckedDir,
/// Lockfile to achieve exclusive access to state files.
lockfile: Mutex<fslock::LockFile>,
}
@@ -58,17 +56,20 @@ impl FsStateMgr {
///
/// This function will try to create `path` if it does not already
/// exist.
- pub fn from_path<P: AsRef<Path>>(path: P) -> Result<Self> {
+ ///
+ /// All files must be "private" according to the rules specified in `mistrust`.
+ pub fn from_path_and_mistrust<P: AsRef<Path>>(
+ path: P,
+ mistrust: &fs_mistrust::Mistrust,
+ ) -> Result<Self> {
let path = path.as_ref();
let statepath = path.join("state");
- let lockpath = path.join("state.lock");
- {
- let mut builder = std::fs::DirBuilder::new();
- #[cfg(target_family = "unix")]
- builder.mode(0o700);
- builder.recursive(true).create(&statepath)?;
- }
+ let statepath = mistrust
+ .verifier()
+ .check_content()
+ .make_secure_dir(statepath)?;
+ let lockpath = statepath.join("state.lock")?;
let lockfile = Mutex::new(fslock::LockFile::open(&lockpath)?);
@@ -79,20 +80,32 @@ impl FsStateMgr {
}),
})
}
- /// Return a filename to use for storing data with `key`.
+ /// Like from_path_and_mistrust, but do not verify permissions.
+ ///
+ /// Testing only.
+ #[cfg(test)]
+ pub fn from_path<P: AsRef<Path>>(path: P) -> Result<Self> {
+ Self::from_path_and_mistrust(
+ path,
+ fs_mistrust::Mistrust::new().dangerously_trust_everyone(),
+ )
+ }
+
+ /// Return a filename, relative to the top of this directory, to use for
+ /// storing data with `key`.
///
/// See "Limitations" section on [`FsStateMgr`] for caveats.
- fn filename(&self, key: &str) -> PathBuf {
- self.inner
- .statepath
- .join(sanitize_filename::sanitize(key) + ".json")
+ fn rel_filename(&self, key: &str) -> PathBuf {
+ (sanitize_filename::sanitize(key) + ".json").into()
}
/// Return the top-level directory for this storage manager.
///
- /// (This is the same directory passed to [`FsStateMgr::from_path`].)
+ /// (This is the same directory passed to
+ /// [`FsStateMgr::from_path_and_mistrust`].)
pub fn path(&self) -> &Path {
self.inner
.statepath
+ .as_path()
.parent()
.expect("No parent directory even after path.join?")
}
@@ -101,7 +114,7 @@ impl FsStateMgr {
///
/// Requires that we hold the lock.
fn clean(&self) {
- for fname in clean::files_to_delete(&self.inner.statepath, SystemTime::now()) {
+ for fname in clean::files_to_delete(self.inner.statepath.as_path(), SystemTime::now()) {
info!("Deleting obsolete file {}", fname.display());
if let Err(e) = std::fs::remove_file(&fname) {
warn!("Unable to delete {}: {}", fname.display(), e);
@@ -149,17 +162,12 @@ impl StateMgr for FsStateMgr {
where
D: DeserializeOwned,
{
- let fname = self.filename(key);
-
- let string = match std::fs::read_to_string(fname) {
- Ok(s) => s,
- Err(e) => {
- if e.kind() == std::io::ErrorKind::NotFound {
- return Ok(None);
- } else {
- return Err(e.into());
- }
- }
+ let rel_fname = self.rel_filename(key);
+
+ let string = match self.inner.statepath.read_to_string(rel_fname) {
+ Ok(string) => string,
+ Err(fs_mistrust::Error::NotFound(_)) => return Ok(None),
+ Err(e) => return Err(e.into()),
};
Ok(Some(serde_json::from_str(&string).map_err(load_error)?))
@@ -173,13 +181,11 @@ impl StateMgr for FsStateMgr {
return Err(Error::NoLock);
}
- let fname = self.filename(key);
+ let rel_fname = self.rel_filename(key);
let output = serde_json::to_string_pretty(val).map_err(store_error)?;
- let fname_tmp = fname.with_extension("tmp");
- std::fs::write(&fname_tmp, &output)?;
- std::fs::rename(fname_tmp, fname)?;
+ self.inner.statepath.write_and_replace(rel_fname, output)?;
Ok(())
}
diff --git a/crates/tor-persist/src/lib.rs b/crates/tor-persist/src/lib.rs
index 196ea40fa7c6c04a4597331c516c41abcca85f73..8163d68dda0c136ca0f889c84fa14dc2792d1815 100644
--- a/crates/tor-persist/src/lib.rs
+++ b/crates/tor-persist/src/lib.rs
@@ -5,7 +5,7 @@
//! implement [Tor](https://www.torproject.org/) in Rust.
//!
//! For now, users should construct storage objects directly with (for
-//! example) [`FsStateMgr::from_path()`], but use them primarily via the
+//! example) [`FsStateMgr::from_path_and_mistrust()`], but use them primarily via the
//! interfaces of the [`StateMgr`] trait.
#![deny(missing_docs)]
@@ -142,6 +142,10 @@ pub enum Error {
#[error("IO error")]
IoError(#[source] Arc<std::io::Error>),
+ /// Permissions on a file or path were incorrect
+ #[error("Invalid permissions on state file")]
+ Permissions(#[from] fs_mistrust::Error),
+
/// Tried to save without holding an exclusive lock.
//
// TODO This error seems to actually be sometimes used to make store a no-op.
@@ -166,6 +170,11 @@ impl tor_error::HasKind for Error {
use tor_error::ErrorKind as K;
match self {
E::IoError(..) => K::PersistentStateAccessFailed,
+ E::Permissions(e) => if e.is_bad_permission() {
+ K::FsPermissions
+ } else {
+ K::PersistentStateAccessFailed
+ }
E::NoLock => K::BadApiUsage,
E::Serialize(..) => K::Internal,
E::Deserialize(..) => K::PersistentStateCorrupted,
diff --git a/doc/semver_status.md b/doc/semver_status.md
index 488a230f560f3188ea57c173d8f8d1b176341ade..f45ed5443f47fd5a971e6254723134a8473c82be 100644
--- a/doc/semver_status.md
+++ b/doc/semver_status.md
@@ -18,3 +18,19 @@ We can delete older sections here after we bump the releases.
## Since Arti 0.3.0
+### arti-client
+
+MODIFIED: Code to configure fs-mistrust.
+BREAKING: TorConfig no longer implements TryInto<DirMgrConfig>
+
+### fs-mistrust
+
+MODIFIED: New APIs for CachedDir, Error.
+
+### tor-dirmgr
+
+BREAKING: Added new cache_trust element to DirMgrConfig.
+
+### tor-persist
+
++BREAKING: Replaced from_path with from_path_and_mistrust