vincenzopalazzo
/
arti
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge branch 'use-fs-mistrust'

This commit is contained in:
Nick Mathewson 2022-05-09 15:27:25 -04:00
parent 0eda471aaf 5c33499f28
commit 4262e9d0ec
23 changed files with 514 additions and 153 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
.gitlab-ci.yml
View File

@ -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

4
Cargo.lock generated
View File

@ -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",

1
crates/arti-client/Cargo.toml
View File

@ -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"}

30
crates/arti-client/src/builder.rs
View File

@ -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,
)

14
crates/arti-client/src/client.rs
View File

@ -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;

24
crates/arti-client/src/config.rs
View File

@ -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)]

1
crates/arti/Cargo.toml
View File

@ -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 }

54
crates/arti/src/lib.rs
View File

@ -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 {

133
crates/fs-mistrust/src/dir.rs
View File

@ -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");
}
}

49
crates/fs-mistrust/src/err.rs
View File

@ -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.

23
crates/fs-mistrust/src/imp.rs
View File

@ -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

21
crates/fs-mistrust/src/lib.rs
View File

@ -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]

1
crates/tor-dirmgr/Cargo.toml
View File

@ -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"}

15
crates/tor-dirmgr/src/config.rs
View File

@ -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(),

11
crates/tor-dirmgr/src/err.rs
View File

@ -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,

7
crates/tor-dirmgr/src/state.rs
View File

@ -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)))
}

22
crates/tor-dirmgr/src/storage.rs
View File

@ -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());
}

145
crates/tor-dirmgr/src/storage/sqlite.rs
View File

@ -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.

9
crates/tor-error/src/lib.rs
View File

@ -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

1
crates/tor-persist/Cargo.toml
View File

@ -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"

72
crates/tor-persist/src/fs.rs
View File

@ -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 rel_fname = self.rel_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 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(())
}

11
crates/tor-persist/src/lib.rs
View File

@ -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,

16
doc/semver_status.md
View File

@ -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