diff --git a/hyperdb-mcp/CHANGELOG.md b/hyperdb-mcp/CHANGELOG.md index 509dfef..747f17c 100644 --- a/hyperdb-mcp/CHANGELOG.md +++ b/hyperdb-mcp/CHANGELOG.md @@ -100,6 +100,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/). `bar_orientation`, `label_values`, `show_legend`, and positive-only `y_scale`, while preserving the public Rust `ChartOptions` surface and existing rendering defaults. +- **`daemon::state_perms` module, with `state_perms::ensure_owner_only_dir`.** + New public surface on the library target: the single place that decides how + the daemon's state directory and the files in it are created and tightened + (see Security, below). It is public because `hyperdb-mcp`'s `[[bin]]` is a + separate crate to Cargo and sets up the daemon's log directory itself; + `pub(crate)` would not reach it. Like the rest of `daemon::*` it is plumbing + for the binary rather than an API to build on — the library target "is not a + documented API surface" — so it may be narrowed without a breaking change. ### Changed @@ -280,6 +288,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ### Fixed +- **On Windows, the daemon state directory now resolves from `%USERPROFILE%` + before `HOME`.** `discovery::state_dir` documented `~` as "`HOME` on Unix, + `USERPROFILE` on Windows" but `home_dir()` tried `HOME` first on every + platform. MSYS2, Cygwin and Git Bash routinely set `HOME` to a path of their + own outside the user profile, so under those shells the state directory + landed outside `%USERPROFILE%` and did not inherit its ACL — the only thing + restricting these files on Windows — without the user having set + `HYPERDB_STATE_DIR` or otherwise asked for it. Windows now prefers + `USERPROFILE`, keeps `HOME` as a last resort so a machine that resolved + before still resolves, and matches `paths::persistent_home_dir`. Unix + resolution is unchanged. **Behaviour change on Windows:** a shell that sets + both to different paths now gets the profile-relative state directory, so a + daemon started before this change may not be discovered by a client started + after it until the old one is stopped. - **Daemon port `0` is now rejected at both entry points, instead of quietly multiplying daemons.** `--port` promises an exact bind and `"0".parse::()` succeeds, so `0` passed validation at both the flag and @@ -632,6 +654,50 @@ and this project adheres to [Semantic Versioning](https://semver.org/). the file has not committed. Fixes [issue #284](https://github.com/tableau/hyper-api-rust/issues/284). +### Security + +- **Daemon state files are now restricted to the owning user.** The state + directory (`~/.hyperdb`, or `HYPERDB_STATE_DIR`) is created `0700` and + `daemon.json` `0600` on Unix, where previously both took their mode from the + process umask — commonly `0755` and `0644`. `daemon.json` names the `hyperd` + endpoint, so it is owner-only from the moment it exists: the mode is set on + the atomic write's temp file *before* any content is written, and the + subsequent `rename` replaces the target's inode, which also tightens a record + an earlier release left readable. `logs/` gets the same `0700` treatment, + since `hyperd` writes its own diagnostic logs there under its own umask and + those records name the endpoint too — restricting the directory covers a + file this process does not own and has not seen yet. Directories and the + regular files directly inside them are both corrected when an earlier run + left them loose, rather than accepted as-is: closing the directory does + nothing about a log file already in it, and `logs/` is where the daemon's own + log and `hyperd`'s rotated logs sit. The sweep is one level deep, skips + anything that is not a regular file, and warns rather than failing, so it + cannot follow a link out of the directory or walk into whatever + `HYPERDB_STATE_DIR` names. +- **The discovery file's temp file is created exclusively, so the mode always + applies to a file of our own.** `write_discovery_record` wrote + `daemon.json.tmp` with a plain create, which opens whatever already bears + that name and leaves the intended mode dependent on what that turns out to + be — a leftover temp file kept its own mode, and a name that resolved + somewhere else was written through. It is now unlinked and recreated with + `O_CREAT | O_EXCL` and `O_NOFOLLOW`, so the record always lands on a regular + file inside the state directory, created with the mode it is meant to have; + if the name cannot be created cleanly the write fails rather than publishing + anyway. +- **A filesystem that cannot represent Unix modes no longer costs the daemon + its discovery file.** `chmod` is refused outright on a mount whose + permissions come from `fmask`/`dmask` rather than from each file — SMB or + NFS, exFAT or vfat, some container bind-mounts. Tightening the *directory* + already warned and carried on there, but setting the mode on `daemon.json` + propagated the refusal, so on exactly those filesystems the daemon warned + about the directory and then could not publish a record at all — turning a + working-if-loose setup into a startup failure. A refusal is now reconciled + against the mode actually on disk: if the file already reads back with + nothing granted to group or other, the record is protected and is published; + if it reads back wider, the error stands and nothing is published. Windows + relies on the ACL that `%USERPROFILE%` subdirectories inherit, which already + excludes other interactive users. + ## [0.5.0] - 2026-06-07 ### Added diff --git a/hyperdb-mcp/DEVELOPMENT.md b/hyperdb-mcp/DEVELOPMENT.md index 3c91b0c..6fe598b 100644 --- a/hyperdb-mcp/DEVELOPMENT.md +++ b/hyperdb-mcp/DEVELOPMENT.md @@ -234,7 +234,11 @@ Logs land next to the persistent file when one is supplied (so users find them i `Engine::new` defaults to *daemon mode* — it tries `daemon::spawn::ensure_daemon(resolve_port_scan())` first, which discovers an existing daemon via `~/.hyperdb/daemon.json` (overridable via -`HYPERDB_STATE_DIR`), else scans the port range for a running daemon, else +`HYPERDB_STATE_DIR` — see `daemon::state_perms`, which restricts that directory +and the files in it to the owning user, and which needs the override to name a +path that can carry those permissions: inside `%USERPROFILE%` on Windows, on a +mode-supporting filesystem on Unix), else scans the port range for a running +daemon, else auto-spawns one on the first free port as a detached background process. The Engine then connects via TCP (`Connection::connect(endpoint, …)`) without owning any `HyperProcess`, and records the daemon's `health_port` so the diff --git a/hyperdb-mcp/README.md b/hyperdb-mcp/README.md index fb394de..ace99eb 100644 --- a/hyperdb-mcp/README.md +++ b/hyperdb-mcp/README.md @@ -286,6 +286,15 @@ hyperdb-mcp daemon # Run as a daemon explicitly (rarely needed) `status` and `stop` locate the running daemon automatically (reading `daemon.json`, then scanning the port range), so they work even if the daemon scanned onto a non-default port. Pass `--port ` to target a specific port explicitly. State files live at `~/.hyperdb/` by default (override with `HYPERDB_STATE_DIR`). +They record the `hyperd` endpoint, so the daemon restricts them to your own +account: `0700` on the directories and `0600` on `daemon.json` on Unix, and on +Windows the ACL a `%USERPROFILE%` subdirectory inherits. If you override the +location, keep it somewhere that can carry those permissions — under your user +profile on Windows, and on a filesystem that supports Unix modes on Unix (a +network share or a FAT/exFAT volume takes its modes from mount options +instead). The daemon warns rather than refusing to start when it cannot tighten +the directory, but it will not publish `daemon.json` into a file it cannot keep +readable by you alone. For installation and configuration diagnostics that also work before MCP can start, use the native doctor command: @@ -970,7 +979,9 @@ Environment: HYPERD_PATH Hyperd executable or containing directory; when absent or non-UTF-8, walk upward for .hyperd/current/hyperd (no PATH lookup) HYPERDB_PERSISTENT_DB Override the default persistent-db path - HYPERDB_STATE_DIR Override daemon state directory (default ~/.hyperdb/) + HYPERDB_STATE_DIR Override daemon state directory (default ~/.hyperdb/); keep it + under your user profile on Windows and on a filesystem with Unix + modes on Unix, or it cannot be restricted to your account HYPERDB_DAEMON_PORT Pin auto-spawn discovery to one health/lock candidate; foreground startup binds this configured/base port exactly HYPERDB_DAEMON_IDLE_TIMEOUT Opt into idle shutdown (seconds); default: stay resident diff --git a/hyperdb-mcp/src/daemon/discovery.rs b/hyperdb-mcp/src/daemon/discovery.rs index 259bac0..1eea80c 100644 --- a/hyperdb-mcp/src/daemon/discovery.rs +++ b/hyperdb-mcp/src/daemon/discovery.rs @@ -296,22 +296,25 @@ pub(super) fn write_enriched_discovery_file(info: &DaemonInfo) -> io::Result<()> fn write_discovery_record(record: &(impl Serialize + ?Sized)) -> io::Result<()> { let dir = state_dir()?; - std::fs::create_dir_all(&dir)?; + super::state_perms::ensure_owner_only_dir(&dir)?; let path = dir.join("daemon.json"); let tmp_path = dir.join("daemon.json.tmp"); let json = serde_json::to_string_pretty(record).map_err(|e| io::Error::other(e.to_string()))?; - std::fs::write(&tmp_path, json.as_bytes())?; - // `std::fs::rename` already replaces an existing target atomically on - // both Unix (`rename(2)`) and Windows (`MoveFileExW` with + // Writes into `tmp_path` and renames it onto `path`. `std::fs::rename` + // already replaces an existing target atomically on both Unix + // (`rename(2)`) and Windows (`MoveFileExW` with // `MOVEFILE_REPLACE_EXISTING`, falling back to `SetFileInformationByHandle` - // with `FILE_RENAME_FLAG_REPLACE_IF_EXISTS`). Pre-deleting the target here + // with `FILE_RENAME_FLAG_REPLACE_IF_EXISTS`). Pre-deleting the target // would reintroduce exactly the window this function's doc comment // promises not to have: a concurrent `discover()` could observe the file // as `Missing` mid-restart (see `try_restart_hyperd`, which rewrites this // file on every `hyperd` restart). - std::fs::rename(&tmp_path, &path)?; - Ok(()) + // + // The rename is also what tightens a record an earlier release left + // world-readable, because it replaces the target's inode rather than + // rewriting it in place. + super::state_perms::write_owner_only_atomic(&path, &tmp_path, json.as_bytes()) } /// Read the discovery file and validate that the daemon is still alive. @@ -451,8 +454,22 @@ pub fn resolve_port() -> u16 { } /// Cross-platform home directory resolution. +/// +/// On Windows the user profile is consulted first. MSYS2, Cygwin and Git Bash +/// commonly set `HOME` to a path of their own outside `%USERPROFILE%`, and +/// preferring it would put the state directory outside the profile — losing the +/// inherited ACL that is the whole of the Windows protection for these files +/// (see [`super::state_perms`]) without the user having asked for it. `HOME` +/// stays as a last resort there, so a machine that resolved before still +/// resolves. This is also the order the documentation above already described, +/// and it matches `crate::paths::persistent_home_dir`. fn home_dir() -> Option { - // Try HOME (Unix) then USERPROFILE (Windows) + if cfg!(windows) { + return std::env::var_os("USERPROFILE") + .filter(|profile| !profile.is_empty()) + .or_else(|| std::env::var_os("HOME")) + .map(PathBuf::from); + } std::env::var_os("HOME") .or_else(|| std::env::var_os("USERPROFILE")) .map(PathBuf::from) @@ -1561,4 +1578,106 @@ mod tests { failures.join("\n") ); } + + /// The state directory and the discovery file in it both name the `hyperd` + /// endpoint, so both must be restricted to the owning user — including when + /// an earlier release already created them under the process umask. + /// + /// Asserts the mode actually on disk rather than that a `chmod` was + /// attempted, since only the former is what another local account sees. + #[cfg(unix)] + fn run_state_permissions_scenario() { + use std::os::unix::fs::PermissionsExt as _; + + assert!( + std::env::var_os("HYPERDB_STATE_DIR").is_some(), + "child scenario requires an isolated state directory" + ); + + fn mode_of(path: &Path) -> u32 { + std::fs::metadata(path) + .unwrap_or_else(|error| panic!("{} should exist: {error}", path.display())) + .permissions() + .mode() + & 0o777 + } + + let dir = state_dir().unwrap(); + let path = discovery_file_path().unwrap(); + let mut failures = Vec::new(); + + // A state directory and discovery file created from nothing. + write_discovery_file(&legacy_info()).unwrap(); + let fresh_dir_mode = mode_of(&dir); + if fresh_dir_mode != 0o700 { + failures.push(format!( + "a newly created state directory was left at {fresh_dir_mode:04o} instead of \ + 0700, so another local account can traverse it" + )); + } + let fresh_file_mode = mode_of(&path); + if fresh_file_mode != 0o600 { + failures.push(format!( + "a newly written discovery file was left at {fresh_file_mode:04o} instead of \ + 0600, so it is readable by other local accounts" + )); + } + + // Permissions left loose by an earlier release must be corrected on the + // next write rather than accepted as they are. + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + assert_eq!( + (mode_of(&dir), mode_of(&path)), + (0o755, 0o644), + "fixture must start world-readable or it does not exercise the correction" + ); + + write_discovery_file(&legacy_info()).unwrap(); + let corrected_dir_mode = mode_of(&dir); + if corrected_dir_mode != 0o700 { + failures.push(format!( + "a pre-existing world-readable state directory stayed at {corrected_dir_mode:04o} \ + instead of being tightened to 0700" + )); + } + let corrected_file_mode = mode_of(&path); + if corrected_file_mode != 0o600 { + failures.push(format!( + "a pre-existing world-readable discovery file stayed at {corrected_file_mode:04o} \ + instead of being tightened to 0600" + )); + } + + // The record must still be readable and intact afterwards: tightening + // permissions is worthless if it breaks the daemon's own discovery. + match serde_json::from_slice::(&std::fs::read(&path).unwrap()) { + Ok(parsed) if parsed == legacy_info() => {} + Ok(_) => failures.push("the restricted record did not round-trip".to_string()), + Err(error) => { + failures.push(format!("the restricted record was unreadable: {error}")); + } + } + + assert!( + failures.is_empty(), + "daemon state permission failures:\n{}", + failures.join("\n") + ); + } + + #[cfg(unix)] + #[test] + fn state_directory_and_discovery_file_are_owner_only() { + const CHILD_SENTINEL_ENV: &str = "HYPERDB_MCP_STATE_PERMISSIONS_CHILD"; + const TEST_NAME: &str = + "daemon::discovery::tests::state_directory_and_discovery_file_are_owner_only"; + + if let Some(marker) = std::env::var_os(CHILD_SENTINEL_ENV) { + std::fs::write(std::path::PathBuf::from(marker), b"started").unwrap(); + run_state_permissions_scenario(); + return; + } + run_discovery_compatibility_child(TEST_NAME, CHILD_SENTINEL_ENV); + } } diff --git a/hyperdb-mcp/src/daemon/mod.rs b/hyperdb-mcp/src/daemon/mod.rs index bdc2a31..187e65c 100644 --- a/hyperdb-mcp/src/daemon/mod.rs +++ b/hyperdb-mcp/src/daemon/mod.rs @@ -7,6 +7,7 @@ pub mod discovery; pub mod health; pub mod run; pub mod spawn; +pub mod state_perms; /// Default base TCP port for the daemon health listener. When no env var is set, /// the daemon scans `[base, base + DAEMON_PORT_SCAN_SPAN)` to find a free port. diff --git a/hyperdb-mcp/src/daemon/run.rs b/hyperdb-mcp/src/daemon/run.rs index 4dc9155..6d3481f 100644 --- a/hyperdb-mcp/src/daemon/run.rs +++ b/hyperdb-mcp/src/daemon/run.rs @@ -196,8 +196,16 @@ pub fn try_record_restart_attempt(history: &mut Vec, now: Instant) -> R /// Build the Parameters used for every hyperd spawn (initial start and restarts). fn build_params() -> std::io::Result { - let log_dir = discovery::state_dir()?.join("logs"); - std::fs::create_dir_all(&log_dir)?; + // The state directory holds `daemon.json`; `logs/` holds `hyperd`'s own + // diagnostic logs, which name the endpoint just as `daemon.json` does. + // `hyperd` is a separate process writing under its own umask, so + // restricting the directory is what covers those files. Both levels are + // restricted here so the daemon's own startup establishes the invariant + // instead of it depending on the later discovery-file write. + let state_dir = discovery::state_dir()?; + super::state_perms::ensure_owner_only_dir(&state_dir)?; + let log_dir = state_dir.join("logs"); + super::state_perms::ensure_owner_only_dir(&log_dir)?; let mut params = Parameters::new(); params.set("log_file_max_count", "2"); diff --git a/hyperdb-mcp/src/daemon/state_perms.rs b/hyperdb-mcp/src/daemon/state_perms.rs new file mode 100644 index 0000000..9089f8c --- /dev/null +++ b/hyperdb-mcp/src/daemon/state_perms.rs @@ -0,0 +1,696 @@ +// Copyright (c) 2026, Salesforce, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 OR MIT + +//! Owner-only permissions for the daemon state directory and the files in it. +//! +//! The state directory (`~/.hyperdb`, or `HYPERDB_STATE_DIR`) holds the +//! daemon's connection details: `daemon.json` names the `hyperd` endpoint, and +//! `logs/` records it too. Those are the owning user's business, so this module +//! is the single place that decides how they are created. +//! +//! Created with [`std::fs::create_dir_all`] and [`std::fs::write`], both would +//! take their mode from the process umask instead — commonly `0755` and `0644`. +//! The helpers here pin the directory to `0700` and files to `0600`. +//! +//! Restricting the *directory* is the main lever on `logs/`: `hyperd` is a +//! separate process writing its own log files under its own umask, so the mode +//! it gives a freshly rotated log is not ours to choose, and a `0700` directory +//! settles the path to every file inside, whoever wrote it. Files already +//! present are tightened too, best effort — closing the directory does nothing +//! about a descriptor or hard link taken while it was still open, which is the +//! state an upgrade actually meets. +//! +//! # Platform behaviour +//! +//! Unix modes have no Windows equivalent, so the mode calls compile out there. +//! Windows leans on ACL inheritance instead: the default state directory sits +//! under `%USERPROFILE%`, whose ACL grants the owning user, `SYSTEM`, and +//! administrators — and *not* other interactive users — and a new subdirectory +//! inherits it. +//! +//! ACL inheritance is therefore the whole of the Windows protection, and it is +//! positional: a state directory outside the profile does not get it. Two +//! things can move it there, and they are not equivalent. Setting +//! `HYPERDB_STATE_DIR` is the user's own call, so it is documented as such +//! beside the variable in `README.md` rather than second-guessed here. +//! Inheriting a `HOME` from an MSYS2, Cygwin or Git Bash shell is *not* a +//! choice about this directory at all, so +//! [`super::discovery::state_dir`]'s resolution prefers `%USERPROFILE%` on +//! Windows and keeps `HOME` only as a last resort. + +use std::io; +use std::path::Path; + +/// Directory mode for state directories: owner-only, including traversal. +#[cfg(unix)] +const STATE_DIR_MODE: u32 = 0o700; + +/// File mode for state files: readable and writable only by the owner. +const STATE_FILE_MODE: u32 = 0o600; + +/// Create `dir`, and any parents this call creates, restricted to the owning +/// user. +/// +/// A directory this call creates is restricted from the start, at every level. +/// A *pre-existing* `dir` — from a release that created it under the process +/// umask — is tightened in place, as are the regular files directly inside it; +/// a pre-existing **parent** is left alone, so a caller that cares about an +/// ancestor must pass it too. Both call sites do, naming the state directory +/// and the `logs/` directory in it explicitly. +/// +/// # Errors +/// Returns an error if the directory cannot be created. Failing to *tighten* an +/// existing directory or file is logged as a warning and tolerated, because +/// `chmod` can be refused for reasons unrelated to the contents — a state +/// directory on a filesystem with no Unix modes, say — and refusing to start +/// there would be a worse outcome than the permissions it was trying to fix. +pub fn ensure_owner_only_dir(dir: &Path) -> io::Result<()> { + create_owner_only_dir(dir)?; + restrict_existing_dir(dir); + restrict_existing_files(dir); + Ok(()) +} + +#[cfg(unix)] +fn create_owner_only_dir(dir: &Path) -> io::Result<()> { + use std::os::unix::fs::DirBuilderExt as _; + + std::fs::DirBuilder::new() + .recursive(true) + .mode(STATE_DIR_MODE) + .create(dir) +} + +#[cfg(not(unix))] +fn create_owner_only_dir(dir: &Path) -> io::Result<()> { + std::fs::create_dir_all(dir) +} + +/// Tighten an existing directory to owner-only access, best effort. +/// +/// Tolerating failure here is deliberate. `chmod` can fail for reasons that +/// have nothing to do with this directory's contents: `HYPERDB_STATE_DIR` may +/// name a path on a filesystem with no Unix modes at all (an SMB or NFS mount, +/// exFAT, some container bind-mounts), where the call is refused however the +/// directory is actually protected. Refusing to start would trade a +/// confidentiality gap the user already had for a total loss of the MCP on +/// those setups — a strictly worse outcome for a local developer tool. +/// +/// The file we are about to *publish* an endpoint into is held to more than +/// this: [`write_owner_only_atomic`] tolerates the same refusal only when the +/// mode already on disk is owner-only anyway, and fails loudly when it is not. +/// The two halves have to agree, because the filesystems that refuse `chmod` +/// on a directory refuse it on the file inside as well — tolerating one and +/// hard-failing the other would leave the daemon warning about the directory +/// and then unable to publish a record at all. +fn restrict_existing_dir(dir: &Path) { + #[cfg(unix)] + { + if let Err(error) = restrict_existing_unix(dir, STATE_DIR_MODE) { + tracing::warn!( + path = %dir.display(), + %error, + "could not restrict the daemon state directory to the current user; \ + its existing permissions are left in place" + ); + } + } + #[cfg(not(unix))] + { + let _ = dir; + } +} + +/// Tighten the regular files directly inside `dir` to owner-only, best effort. +/// +/// Restricting the directory closes the path to a *new* reader, but a file +/// already inside keeps the mode it was created with, and anything that +/// reached it before the directory was tightened — an open descriptor, a hard +/// link — still reaches it afterwards. An upgrade over a state directory an +/// earlier release created under the umask therefore has files to correct and +/// not just directories: the daemon's own log and `hyperd`'s rotated logs name +/// the endpoint, as `daemon.json` does. +/// +/// One level, and regular files only. Both call sites pass `logs/` as well as +/// the state directory, so there is nothing deeper that this module puts the +/// endpoint into, and descending further would take a permission sweep into +/// whatever a user pointed `HYPERDB_STATE_DIR` at. Symlinks are skipped rather +/// than followed, since the target is not this directory's business. +/// +/// Failure is tolerated exactly as in [`restrict_existing_dir`], and for the +/// same reason. +fn restrict_existing_files(dir: &Path) { + #[cfg(unix)] + { + let entries = match std::fs::read_dir(dir) { + Ok(entries) => entries, + Err(error) => { + tracing::warn!( + path = %dir.display(), + %error, + "could not list the daemon state directory to restrict the files in it; \ + their existing permissions are left in place" + ); + return; + } + }; + for entry in entries.flatten() { + // From the directory read, so this reports a symlink as a symlink + // rather than as whatever it points at. + if !entry.file_type().is_ok_and(|kind| kind.is_file()) { + continue; + } + let path = entry.path(); + if let Err(error) = restrict_existing_unix(&path, STATE_FILE_MODE) { + tracing::warn!( + path = %path.display(), + %error, + "could not restrict a file in the daemon state directory to the current \ + user; its existing permissions are left in place" + ); + } + } + } + #[cfg(not(unix))] + { + let _ = dir; + } +} + +/// Reset an existing path's permission bits to `mode`. +/// +/// Skips the `chmod` when the mode already matches, so the steady state costs +/// one `stat`. `permissions().mode()` carries the file-type bits too, hence the +/// mask before comparing. +#[cfg(unix)] +fn restrict_existing_unix(path: &Path, mode: u32) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt as _; + + let metadata = std::fs::metadata(path)?; + if metadata.permissions().mode() & 0o777 == mode { + return Ok(()); + } + std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)) +} + +/// Write `contents` to `path` atomically, restricted to the owning user. +/// +/// `tmp_path` receives the content and is then renamed onto `path`, preserving +/// the existing guarantee that a concurrent reader sees either the old record +/// or the new one, never a partial write. The mode is set on `tmp_path` *before +/// any content is written*, so the endpoint is never on disk in a +/// world-readable file — not even briefly. +/// +/// Because `rename` replaces the target's inode rather than its contents, the +/// published file carries this mode even if `path` already existed with looser +/// permissions. That is what corrects a `daemon.json` left behind by an earlier +/// release. +/// +/// `tmp_path` is unlinked and created exclusively rather than reopened, so the +/// file the mode lands on is always one this call made — not a temp file an +/// interrupted earlier write left behind, and not another entry that happens +/// to bear the name. +/// +/// # Errors +/// Returns an error if the temp file cannot be created with the intended mode, +/// cannot be written, or cannot be renamed onto `path`. Unlike a directory +/// that merely already exists, this file is one this process is about to +/// publish an endpoint into, so a record left readable beyond its owner is not +/// published at all — see [`restrict_open_state_file`] for the one refusal +/// that is tolerated, and why it does not weaken that. +pub(crate) fn write_owner_only_atomic( + path: &Path, + tmp_path: &Path, + contents: &[u8], +) -> io::Result<()> { + write_owner_only_atomic_with(path, tmp_path, contents, chmod_descriptor) +} + +/// How an open state file's mode is set. +/// +/// A parameter only so the mode-less-filesystem branch of +/// [`restrict_open_state_file`] can be exercised without one of the exotic +/// filesystems it exists for. Production always passes [`chmod_descriptor`]. +type ChmodFn = fn(&std::fs::File, u32) -> io::Result<()>; + +#[cfg(unix)] +fn chmod_descriptor(file: &std::fs::File, mode: u32) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt as _; + + file.set_permissions(std::fs::Permissions::from_mode(mode)) +} + +// `allow` rather than `expect`: this is the only configuration in which the +// lint fires, and it is not one the Unix development host can compile, so an +// unfulfilled expectation could not be caught here. +#[cfg(not(unix))] +#[allow( + clippy::unnecessary_wraps, + reason = "the signature is fixed by `ChmodFn`, whose Unix implementation can fail; there \ + is no mode to set here, so this reports success" +)] +fn chmod_descriptor(_file: &std::fs::File, _mode: u32) -> io::Result<()> { + Ok(()) +} + +fn write_owner_only_atomic_with( + path: &Path, + tmp_path: &Path, + contents: &[u8], + chmod: ChmodFn, +) -> io::Result<()> { + use std::io::Write as _; + + // An interrupted earlier write can leave `tmp_path` behind. Unlink it + // instead of reopening it, so the exclusive create below always gets a + // file this call made and `OpenOptions::mode` always applies. `remove_file` + // unlinks the name, so a leftover symlink goes rather than its target. + match std::fs::remove_file(tmp_path) { + Ok(()) => {} + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => return Err(error), + } + + let mut options = std::fs::OpenOptions::new(); + // `create_new` is `O_CREAT | O_EXCL`, which fails on an existing entry of + // this name rather than opening it — a symlink included, since `O_EXCL` + // refuses one even when it dangles. `O_NOFOLLOW` states the same intent + // outright. Should something reappear at the name between the unlink and + // the open, the open is what fails, so the record is not published rather + // than published somewhere else. + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.mode(STATE_FILE_MODE).custom_flags(libc::O_NOFOLLOW); + } + + let mut file = options.open(tmp_path)?; + + // `OpenOptions::mode` is a request, not a guarantee: the process umask + // clears bits from it, and a filesystem that stores no modes ignores it. + // Restricting the descriptor pins the result, and does so before any + // content is written. + restrict_open_state_file(&file, chmod)?; + + file.write_all(contents)?; + // Close before renaming: Windows refuses to rename a file that is still + // open for writing. + drop(file); + + std::fs::rename(tmp_path, path) +} + +/// Restrict an open state file to [`STATE_FILE_MODE`], distinguishing "the +/// mode could not be set" from "this filesystem does not have modes to set". +/// +/// A filesystem whose permissions come from mount options rather than from +/// each file — an SMB or NFS mount, exFAT or vfat, some container bind-mounts +/// — refuses `chmod` outright while still reporting a mode, taken from +/// `fmask`. Propagating that refusal would leave the daemon unable to publish +/// `daemon.json` at all on those setups, which is the total loss of the MCP +/// that [`restrict_existing_dir`] declines to cause one directory earlier: the +/// same filesystem fails both calls, so warning about the directory and then +/// failing on the file would be the worst of both policies. +/// +/// So a refusal is reconciled against what is actually on disk. If the +/// descriptor already reads back owner-only, the record is protected — the +/// filesystem simply reports a fixed mode — and the write proceeds. If it +/// reads back wider, nothing has protected the record, the error stands, and +/// it is not published. That keeps the invariant the loud failure existed for +/// without breaking a configuration that was already safe. +fn restrict_open_state_file(file: &std::fs::File, chmod: ChmodFn) -> io::Result<()> { + match chmod(file, STATE_FILE_MODE) { + Ok(()) => Ok(()), + Err(error) => reconcile_refused_chmod(file, error), + } +} + +#[cfg(unix)] +fn reconcile_refused_chmod(file: &std::fs::File, error: io::Error) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt as _; + + let observed = file.metadata()?.permissions().mode() & 0o777; + if !is_owner_only(observed) { + return Err(error); + } + tracing::warn!( + %error, + mode = format!("{observed:04o}"), + "could not set the mode on the daemon state file; the filesystem reports it as \ + owner-only already, so the record is published with the mode it has" + ); + Ok(()) +} + +#[cfg(not(unix))] +fn reconcile_refused_chmod(_file: &std::fs::File, error: io::Error) -> io::Result<()> { + Err(error) +} + +/// Whether `mode` grants nothing to group or to other. +/// +/// Only the group and other bits are consulted: owner-execute makes no +/// difference to who besides the owner can read the record, and a filesystem +/// reporting a fixed mode may well set it. +#[cfg(unix)] +#[expect( + clippy::verbose_bit_mask, + reason = "`trailing_zeros() >= 6` is the same test but says nothing about permissions; \ + the octal mask names the group and other bits it clears" +)] +fn is_owner_only(mode: u32) -> bool { + mode & 0o077 == 0 +} + +#[cfg(all(test, unix))] +mod tests { + use std::os::unix::fs::PermissionsExt as _; + + use tempfile::TempDir; + + use super::*; + + fn mode_of(path: &Path) -> u32 { + std::fs::metadata(path) + .expect("fixture path should exist") + .permissions() + .mode() + & 0o777 + } + + /// Stands in for a filesystem that refuses `chmod` outright but already + /// reports the file as owner-only, the way a mount whose `fmask` denies + /// group and other does. + fn refuse_chmod(_file: &std::fs::File, _mode: u32) -> io::Result<()> { + Err(io::Error::from_raw_os_error(libc::EPERM)) + } + + /// Stands in for a filesystem that refuses `chmod` and reports a mode + /// wider than the one asked for. + fn refuse_chmod_over_a_wider_mode(file: &std::fs::File, _mode: u32) -> io::Result<()> { + file.set_permissions(std::fs::Permissions::from_mode(0o644)) + .expect("the fixture filesystem must support widening the temp file"); + Err(io::Error::from_raw_os_error(libc::EPERM)) + } + + #[test] + fn state_directory_is_created_owner_only() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join("state"); + + ensure_owner_only_dir(&dir).unwrap(); + + assert_eq!( + mode_of(&dir), + 0o700, + "a freshly created state directory must not be reachable by other users" + ); + } + + #[test] + fn nested_state_directory_is_created_owner_only_at_every_level() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path().join("state"); + let nested = root.join("logs"); + + ensure_owner_only_dir(&nested).unwrap(); + + assert_eq!( + mode_of(&nested), + 0o700, + "the leaf state directory must be owner-only" + ); + assert_eq!( + mode_of(&root), + 0o700, + "an intermediate state directory must be owner-only too, or the leaf is still \ + reachable through it" + ); + } + + #[test] + fn a_pre_existing_world_readable_state_directory_is_tightened() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join("state"); + std::fs::create_dir(&dir).unwrap(); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert_eq!( + mode_of(&dir), + 0o755, + "fixture must start world-readable or it does not exercise the correction" + ); + + ensure_owner_only_dir(&dir).unwrap(); + + assert_eq!( + mode_of(&dir), + 0o700, + "a state directory left loose by an earlier run must be corrected, not accepted" + ); + } + + /// The shape an upgrade actually meets: both the state directory and the + /// `logs/` directory inside it already exist, world-readable, from a + /// release that created them under the process umask. + #[test] + fn a_pre_existing_world_readable_directory_tree_is_tightened_at_every_level() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path().join("state"); + let nested = root.join("logs"); + std::fs::create_dir_all(&nested).unwrap(); + for dir in [&root, &nested] { + std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert_eq!( + mode_of(dir), + 0o755, + "fixture must start world-readable or it does not exercise the correction" + ); + } + + // Mirrors the daemon startup order: the state directory, then the log + // directory inside it. + ensure_owner_only_dir(&root).unwrap(); + ensure_owner_only_dir(&nested).unwrap(); + + assert_eq!( + mode_of(&root), + 0o700, + "an existing state directory must be tightened on the next daemon start" + ); + assert_eq!( + mode_of(&nested), + 0o700, + "an existing log directory must be tightened too — it holds hyperd's own logs, \ + which name the endpoint" + ); + } + + /// Tightening the directory closes the path to a new reader, but says + /// nothing about a file already inside it or about anything already + /// holding that file open. The log files an earlier release left in + /// `logs/` name the endpoint, so they are corrected too. + #[test] + fn pre_existing_files_in_a_loose_state_directory_are_tightened() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join("logs"); + std::fs::create_dir(&dir).unwrap(); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + let group_readable = dir.join("hyperd-0.log"); + let world_readable = dir.join("hyperdb-daemon.log"); + for (file, mode) in [(&group_readable, 0o660), (&world_readable, 0o644)] { + std::fs::write(file, b"endpoint").unwrap(); + std::fs::set_permissions(file, std::fs::Permissions::from_mode(mode)).unwrap(); + assert_eq!( + mode_of(file), + mode, + "fixture must start readable beyond its owner or it does not exercise the \ + correction" + ); + } + + ensure_owner_only_dir(&dir).unwrap(); + + assert_eq!(mode_of(&dir), 0o700); + for file in [&group_readable, &world_readable] { + assert_eq!( + mode_of(file), + 0o600, + "a log file left readable beyond its owner must be corrected, not just \ + covered by the directory" + ); + assert_eq!( + std::fs::read(file).unwrap(), + b"endpoint", + "tightening a log file must not disturb its contents" + ); + } + } + + /// The sweep stays one level deep and leaves anything that is not a + /// regular file alone, so it cannot walk out of the state directory + /// through a link or descend into whatever `HYPERDB_STATE_DIR` names. + #[test] + fn the_file_sweep_does_not_follow_links_or_descend() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join("state"); + let nested = dir.join("nested"); + std::fs::create_dir_all(&nested).unwrap(); + let outside = tmp.path().join("outside.txt"); + std::fs::write(&outside, b"unrelated").unwrap(); + std::fs::set_permissions(&outside, std::fs::Permissions::from_mode(0o644)).unwrap(); + std::os::unix::fs::symlink(&outside, dir.join("link.json")).unwrap(); + let deeper = nested.join("deeper.log"); + std::fs::write(&deeper, b"deeper").unwrap(); + std::fs::set_permissions(&deeper, std::fs::Permissions::from_mode(0o644)).unwrap(); + + ensure_owner_only_dir(&dir).unwrap(); + + assert_eq!( + mode_of(&outside), + 0o644, + "the sweep must not follow a link out of the state directory" + ); + assert_eq!( + mode_of(&deeper), + 0o644, + "the sweep must not descend; callers name each directory they care about" + ); + } + + #[test] + fn state_file_is_written_owner_only() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("daemon.json"); + let tmp_path = tmp.path().join("daemon.json.tmp"); + + write_owner_only_atomic(&path, &tmp_path, b"{}").unwrap(); + + assert_eq!( + mode_of(&path), + 0o600, + "a state file naming the endpoint must be readable only by its owner" + ); + assert_eq!(std::fs::read(&path).unwrap(), b"{}"); + assert!( + !tmp_path.exists(), + "the temp file should have been renamed onto the target" + ); + } + + #[test] + fn replacing_a_world_readable_state_file_tightens_it() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("daemon.json"); + let tmp_path = tmp.path().join("daemon.json.tmp"); + std::fs::write(&path, b"stale").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + assert_eq!( + mode_of(&path), + 0o644, + "fixture must start world-readable or it does not exercise the correction" + ); + + write_owner_only_atomic(&path, &tmp_path, b"{}").unwrap(); + + assert_eq!( + mode_of(&path), + 0o600, + "rewriting a state file left loose by an earlier release must tighten it" + ); + } + + #[test] + fn a_world_readable_leftover_temp_file_does_not_leak_the_new_record() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("daemon.json"); + let tmp_path = tmp.path().join("daemon.json.tmp"); + // An interrupted earlier write leaves the temp file behind, carrying + // whatever mode it was created with. Writing into that file as-is + // would put the new record in a world-readable file. + std::fs::write(&tmp_path, b"interrupted").unwrap(); + std::fs::set_permissions(&tmp_path, std::fs::Permissions::from_mode(0o644)).unwrap(); + + write_owner_only_atomic(&path, &tmp_path, b"{}").unwrap(); + + assert_eq!( + mode_of(&path), + 0o600, + "a leftover temp file must not carry its loose mode into the published record" + ); + } + + /// The temp file must be one this call created, not whatever its name + /// already resolves to. A stale link of that name is replaced rather than + /// followed: otherwise the record would be written wherever the link + /// points — outside the state directory, where none of this module's + /// permissions apply — and `daemon.json` would end up as a link instead of + /// the record. + #[test] + fn a_symlinked_temp_path_is_replaced_rather_than_followed() { + let tmp = TempDir::new().unwrap(); + let state = tmp.path().join("state"); + std::fs::create_dir(&state).unwrap(); + let unrelated = tmp.path().join("unrelated.txt"); + std::fs::write(&unrelated, b"unrelated contents").unwrap(); + + let path = state.join("daemon.json"); + let tmp_path = state.join("daemon.json.tmp"); + std::os::unix::fs::symlink(&unrelated, &tmp_path).unwrap(); + + write_owner_only_atomic(&path, &tmp_path, b"{}").unwrap(); + + assert_eq!( + std::fs::read(&unrelated).unwrap(), + b"unrelated contents", + "a stale link at the temp path must not redirect the record out of the state directory" + ); + assert!( + std::fs::symlink_metadata(&path) + .unwrap() + .file_type() + .is_file(), + "the published record must be a regular file, not a link left in place" + ); + assert_eq!(std::fs::read(&path).unwrap(), b"{}"); + assert_eq!(mode_of(&path), 0o600); + } + + /// A filesystem whose modes come from mount options rather than from each + /// file — an SMB or NFS mount, exFAT, some container bind-mounts — refuses + /// `chmod` while still reporting a mode. When the mode it reports is + /// already owner-only, the record is protected, and refusing to publish it + /// would cost the user the MCP over a permission that is in fact correct. + #[test] + fn a_refused_chmod_still_publishes_a_record_that_is_already_owner_only() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("daemon.json"); + let tmp_path = tmp.path().join("daemon.json.tmp"); + + write_owner_only_atomic_with(&path, &tmp_path, b"{}", refuse_chmod).expect( + "a record the filesystem already reports as owner-only must still be published", + ); + + assert_eq!(mode_of(&path), 0o600); + assert_eq!(std::fs::read(&path).unwrap(), b"{}"); + } + + /// The converse, which is the invariant the tolerance must not cost us: a + /// refused `chmod` over a mode that really is wider must not publish. + #[test] + fn a_refused_chmod_over_a_wider_mode_publishes_nothing() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("daemon.json"); + let tmp_path = tmp.path().join("daemon.json.tmp"); + + let error = + write_owner_only_atomic_with(&path, &tmp_path, b"{}", refuse_chmod_over_a_wider_mode) + .expect_err("a record whose mode could not be restricted must not be published"); + + assert_eq!(error.raw_os_error(), Some(libc::EPERM)); + assert!( + !path.exists(), + "no record must be published when its mode could not be restricted" + ); + } +} diff --git a/hyperdb-mcp/src/main.rs b/hyperdb-mcp/src/main.rs index 9d7a4ec..3e594cb 100644 --- a/hyperdb-mcp/src/main.rs +++ b/hyperdb-mcp/src/main.rs @@ -217,9 +217,13 @@ async fn run_daemon_mode( port: u16, idle_timeout: Option, ) -> Result<(), Box> { - // Daemon logs go to ~/.hyperdb/logs/ - let log_dir = discovery::state_dir()?.join("logs"); - std::fs::create_dir_all(&log_dir)?; + // Daemon logs go to ~/.hyperdb/logs/. They record the hyperd endpoint, so + // both the state directory and the log directory inside it are restricted + // to the owning user, as `daemon.json` is. + let state_dir = discovery::state_dir()?; + daemon::state_perms::ensure_owner_only_dir(&state_dir)?; + let log_dir = state_dir.join("logs"); + daemon::state_perms::ensure_owner_only_dir(&log_dir)?; let file_appender = tracing_appender::rolling::never(&log_dir, "hyperdb-daemon.log"); let (file_writer, _file_guard) = tracing_appender::non_blocking(file_appender);