diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c7e0239..828460d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -153,6 +153,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- The config directory is now created closed to other users, and one an earlier + build left open is repaired on the next write. It holds the command history — + every command with its working directory and exit status — along with the SSH + profiles in `config.json`, the session's directories and the workspace tree, + and it was created under the process umask: 022 on a stock box, which makes + the directory 0755 and the history file 0644. On a shared machine another + account could read all of it. The daemon's own history subdirectory already + closed itself for exactly this reason, as did the unix socket's parent; the + directory holding the rest did not. - A control request whose handler fails outright is now answered with an error instead of silently dropped. Every other way out already answered — even a request the queue was too full to accept — but a panic inside the handler diff --git a/crates/tty7-core/src/core/config.rs b/crates/tty7-core/src/core/config.rs index 12446683..58c24f82 100644 --- a/crates/tty7-core/src/core/config.rs +++ b/crates/tty7-core/src/core/config.rs @@ -905,7 +905,7 @@ impl Config { return; }; if let Some(parent) = path.parent() { - let _ = std::fs::create_dir_all(parent); + let _ = ensure_private_dir(parent); } match serde_json::to_string_pretty(self) { Ok(text) => { @@ -1105,6 +1105,57 @@ fn quarantine_path(path: &std::path::Path) -> PathBuf { .unwrap_or(base) } +/// Create `dir`, and close it to this user. +/// +/// The config directory holds the command history, the SSH profiles, the +/// session's working directories and the workspace tree. Not all of that is +/// written with a private mode of its own — `history` is appended by the app +/// under the process umask, which is 022 on a stock box, so the file lands +/// 0644 — and the directory it sits in was created the same way. Between them +/// nothing was closed, and on a shared machine another account could read +/// every command you had run, with its directory and its exit status. +/// +/// `daemon::history` already says this about the subdirectory it writes: +/// "closing the directory is what makes the mode of what is inside it moot". +/// This is that rule for the directory holding it, which is where the rest of +/// the state lives. +/// +/// Applied on every call rather than only when the directory is new, so a +/// config directory an earlier build left open is closed the next time +/// anything writes to it. +pub fn ensure_private_dir(dir: &std::path::Path) -> std::io::Result<()> { + // Every directory this call brings into being, not just the last one. + // `create_dir_all("/history.d")` makes `` too, and closing + // only the leaf would leave the directory holding `config.json`, `history` + // and `session.json` exactly as open as before. + // + // Ancestors that already existed are left alone: `$HOME` and `~/.config` + // are not ours to re-permission, and on a first run they are the ones this + // walk stops at. + #[cfg(unix)] + let fresh: Vec<&std::path::Path> = dir.ancestors().take_while(|p| !p.exists()).collect(); + std::fs::create_dir_all(dir)?; + // Windows has no umask; the config directory inherits an ACL that is + // already per-user, which is the same reason `daemon::history` skips it. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + let private = std::fs::Permissions::from_mode(0o700); + for made in fresh { + let _ = std::fs::set_permissions(made, private.clone()); + } + // The config directory itself even when it was already there, so one + // an earlier build left open is repaired rather than skipped. Only + // that one: a directory this call did not create and that is not ours + // is somebody else's to permission, which is the distinction + // `transport::bind` drew with `owns_parent` before this existed. + if config_dir().is_some_and(|c| c == dir) { + let _ = std::fs::set_permissions(dir, private); + } + } + Ok(()) +} + pub fn write_atomic(path: &std::path::Path, bytes: &[u8]) -> std::io::Result<()> { write_atomic_mode(path, bytes, false) } @@ -2114,6 +2165,56 @@ mod tests { } } + #[cfg(unix)] + #[test] + fn the_config_directory_is_closed_to_other_users() { + use std::os::unix::fs::PermissionsExt as _; + let _guard = lock_config_file(); + let mode_of = + |p: &std::path::Path| std::fs::metadata(p).unwrap().permissions().mode() & 0o777; + + // Every directory the call brings into being, not only the last one. + // A stock box runs umask 022, which would leave these 0755 and the + // `history` file inside them 0644 — readable by every other account. + let scratch = std::env::temp_dir().join(format!("tty7-privdir-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&scratch); + let nested = scratch.join("history.d"); + super::ensure_private_dir(&nested).expect("the directory is created"); + for made in [&scratch, &nested] { + assert_eq!( + mode_of(made), + 0o700, + "{} is open, so anyone on this machine can read what is in it", + made.display() + ); + } + + // A directory that was already there and is not ours stays as it is: + // `$HOME` and `~/.config` are on this walk on a first run, and neither + // is this code's to re-permission. + std::fs::set_permissions(&nested, std::fs::Permissions::from_mode(0o755)).unwrap(); + super::ensure_private_dir(&nested.join("deeper")).expect("still there"); + assert_eq!( + mode_of(&nested), + 0o755, + "a directory this call did not create was re-permissioned anyway" + ); + let _ = std::fs::remove_dir_all(&scratch); + + // The config directory is the exception, so one an earlier build left + // open is repaired the next time anything writes to it rather than + // staying open for the life of the install. + pin_config_dir(); + let cfg = config_dir().expect("the pinned config dir resolves"); + std::fs::set_permissions(&cfg, std::fs::Permissions::from_mode(0o755)).unwrap(); + super::ensure_private_dir(&cfg).expect("the config dir is there"); + assert_eq!( + mode_of(&cfg), + 0o700, + "a config directory an earlier build left open stayed open" + ); + } + #[test] fn write_atomic_replaces_contents_and_leaves_no_temp() { let dir = TestDir::new("atomic"); diff --git a/crates/tty7-core/src/core/session.rs b/crates/tty7-core/src/core/session.rs index 8cef3ba2..2de7038a 100644 --- a/crates/tty7-core/src/core/session.rs +++ b/crates/tty7-core/src/core/session.rs @@ -495,7 +495,7 @@ impl WindowViews { return; }; if let Some(parent) = path.parent() - && let Err(e) = std::fs::create_dir_all(parent) + && let Err(e) = crate::core::config::ensure_private_dir(parent) { log::warn!("failed to create views dir {}: {e}", parent.display()); return; diff --git a/crates/tty7-core/src/daemon/pidfile.rs b/crates/tty7-core/src/daemon/pidfile.rs index 73d14680..85cff228 100644 --- a/crates/tty7-core/src/daemon/pidfile.rs +++ b/crates/tty7-core/src/daemon/pidfile.rs @@ -9,7 +9,7 @@ pub fn path() -> Option { pub(crate) fn write_current() { let Some(path) = path() else { return }; if let Some(parent) = path.parent() { - let _ = std::fs::create_dir_all(parent); + let _ = crate::core::config::ensure_private_dir(parent); } if let Err(e) = std::fs::write(&path, std::process::id().to_string()) { log::warn!("could not write pidfile {}: {e}", path.display()); diff --git a/crates/tty7-core/src/daemon/scrollback.rs b/crates/tty7-core/src/daemon/scrollback.rs index 270b36b1..e75b8d42 100644 --- a/crates/tty7-core/src/daemon/scrollback.rs +++ b/crates/tty7-core/src/daemon/scrollback.rs @@ -168,7 +168,7 @@ pub fn save(pane_id: u64, segments: &[Segment]) { let Some(parent) = path.parent().map(|p| p.to_path_buf()) else { return; }; - if let Err(e) = std::fs::create_dir_all(&parent) { + if let Err(e) = crate::core::config::ensure_private_dir(&parent) { log::debug!("no scrollback directory ({e}); pane {pane_id} is not persisted"); return; } diff --git a/crates/tty7-core/src/daemon/singleton.rs b/crates/tty7-core/src/daemon/singleton.rs index 0eac90d1..1691e647 100644 --- a/crates/tty7-core/src/daemon/singleton.rs +++ b/crates/tty7-core/src/daemon/singleton.rs @@ -117,7 +117,7 @@ pub fn claim() -> Claim { return Claim::Unavailable("no config directory".into()); }; if let Some(parent) = path.parent() { - let _ = std::fs::create_dir_all(parent); + let _ = crate::core::config::ensure_private_dir(parent); } match open_exclusive(&path) { Ok(Some(file)) => { diff --git a/crates/tty7-core/src/daemon/transport.rs b/crates/tty7-core/src/daemon/transport.rs index 3632f9bf..719c9eef 100644 --- a/crates/tty7-core/src/daemon/transport.rs +++ b/crates/tty7-core/src/daemon/transport.rs @@ -438,7 +438,7 @@ mod imp_windows { let path = port_path_named(file) .ok_or_else(|| anyhow::anyhow!("could not resolve {file} path (no config dir)"))?; if let Some(parent) = path.parent() { - let _ = std::fs::create_dir_all(parent); + let _ = config::ensure_private_dir(parent); } let listener = TcpListener::bind(loopback(0)) .map_err(|e| anyhow::anyhow!("bind 127.0.0.1:0 failed: {e}"))?; diff --git a/src/main.rs b/src/main.rs index f2af1c71..f8f27519 100644 --- a/src/main.rs +++ b/src/main.rs @@ -37,7 +37,7 @@ fn spawn_config_watcher(cx: &mut App) { let Some(dir) = crate::core::config::config_dir_path() else { return; }; - let _ = std::fs::create_dir_all(&dir); + let _ = crate::core::config::ensure_private_dir(&dir); const DEBOUNCE: std::time::Duration = std::time::Duration::from_millis(200); diff --git a/src/terminal/history.rs b/src/terminal/history.rs index 0e72f24c..11d1e39f 100644 --- a/src/terminal/history.rs +++ b/src/terminal/history.rs @@ -233,7 +233,7 @@ pub fn append(scope: &Scope, cmd: &str, cwd: Option<&Path>, ts: u64, exit: Optio return; }; if let Some(parent) = path.parent() { - let _ = std::fs::create_dir_all(parent); + let _ = tty7_core::core::config::ensure_private_dir(parent); } let cwd = match cwd.and_then(Path::to_str) { Some(c) if looks_absolute(c) && !c.contains(['\t', '\n', '\r']) => c, diff --git a/src/ui/presets.rs b/src/ui/presets.rs index bbac35ce..e48d3f52 100644 --- a/src/ui/presets.rs +++ b/src/ui/presets.rs @@ -721,7 +721,7 @@ pub fn to_yaml(t: &Theme) -> String { pub fn fork_to_file(t: &Theme) -> std::io::Result { let dir = themes_dir() .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "no themes directory"))?; - std::fs::create_dir_all(&dir)?; + crate::core::config::ensure_private_dir(&dir)?; let base = format!("{}-custom", t.id.trim_end_matches("-custom")); let mut stem = base.clone(); let mut n = 2;