From 6456f503f111be51157453cc160a015f5216827c Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Fri, 16 Oct 2026 11:58:28 +0200 Subject: [PATCH] fix(cli,core): keep secret-holding commands out of core dumps Only `pdfs daemon` marked itself non-dumpable, though `mount`, `login`, `unlock` and `logout` hold the same passphrases or tokens. All five now do. The file-manager script also passes each path after `--`, so a file named like an option stays a path (B240). --- crates/pdfs-cli/src/main.rs | 29 ++++++++++++++++++++++++++--- crates/pdfs-core/src/shell.rs | 17 ++++++++++++++++- docs/BUGS.md | 31 +++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 4 deletions(-) diff --git a/crates/pdfs-cli/src/main.rs b/crates/pdfs-cli/src/main.rs index 6986e54..6187a21 100644 --- a/crates/pdfs-cli/src/main.rs +++ b/crates/pdfs-cli/src/main.rs @@ -831,6 +831,20 @@ fn main() -> Result<()> { let cli = Cli::parse(); JSON_OUTPUT.store(cli.json, std::sync::atomic::Ordering::Relaxed); + // The commands that hold the password, the key passphrases or the session + // tokens in memory: keep them out of core dumps and away from same-user + // ptrace. Only the daemon did, though `mount` holds the same keys for as + // long as it runs. + if matches!( + cli.command, + Command::Login { .. } + | Command::Unlock { .. } + | Command::Logout + | Command::Mount { .. } + | Command::Daemon { .. } + ) { + pdfs_core::kernelkey::forbid_dumps(); + } match cli.command { Command::Login { username, remember } => cmd_login(username, remember), Command::Unlock { remember } => cmd_unlock(remember), @@ -2227,9 +2241,6 @@ where /// an external unmount triggers a remount; errors back off and retry. This is /// the entry point for the systemd user service. fn cmd_daemon(mountpoint: Option) -> Result<()> { - // The daemon holds the account's key passphrases in memory; keep them out - // of core dumps and away from same-user ptrace. - pdfs_core::kernelkey::forbid_dumps(); loop { // Wait until a session is stored. The GUI enables this service on login, // but the service may also start at boot before the user has logged in. @@ -3787,6 +3798,18 @@ mod login_wait_tests { assert_eq!(login_poll_delay(u32::MAX), Duration::from_secs(15)); } + /// The file-manager script passes each selected path after `--`, so a + /// file named like an option is still the path. + #[test] + fn a_path_after_a_double_dash_is_a_path_even_when_it_looks_like_a_flag() { + let cli = Cli::try_parse_from(["pdfs", "pin", "--", "--json"]).unwrap(); + assert!(!cli.json); + match cli.command { + Command::Pin { path } => assert_eq!(path, PathBuf::from("--json")), + _ => panic!("not a pin"), + } + } + #[test] fn a_waited_for_run_that_did_not_fully_succeed_exits_with_an_error() { assert!(finished_run("the import", Some((0, false, None))).is_ok()); diff --git a/crates/pdfs-core/src/shell.rs b/crates/pdfs-core/src/shell.rs index 22932a7..a5323a4 100644 --- a/crates/pdfs-core/src/shell.rs +++ b/crates/pdfs-core/src/shell.rs @@ -23,6 +23,11 @@ const SCRIPTS: [(&str, &str); 2] = [ /// `NAUTILUS_SCRIPT_SELECTED_FILE_PATHS`; the positional args carry the same /// selection when the script runs on a local (non-URI) folder, so we prefer the /// environment and fall back to `"$@"` for the Nemo/Caja variants. +/// +/// The `--` keeps a file named like an option, such as `--json`, a path. A +/// name with a newline in it still arrives split in two from Nautilus, whose +/// list is newline-separated; `pdfs` then fails on both halves, which touches +/// nothing. fn script_body(subcommand: &str) -> String { format!( "#!/bin/sh\n\ @@ -41,7 +46,7 @@ fn script_body(subcommand: &str) -> String { \n\ printf '%s' \"$paths\" | while IFS= read -r path; do\n\ \x20 [ -n \"$path\" ] || continue\n\ - \x20 pdfs {subcommand} \"$path\" || exit 1\n\ + \x20 pdfs {subcommand} -- \"$path\" || exit 1\n\ done\n" ) } @@ -95,3 +100,13 @@ fn write_scripts(dir: &PathBuf) -> io::Result<()> { tracing::debug!("installed file-manager scripts in {dir:?}"); Ok(()) } + +#[cfg(test)] +mod tests { + use super::script_body; + + #[test] + fn the_script_ends_options_before_each_path() { + assert!(script_body("pin").contains(r#"pdfs pin -- "$path""#)); + } +} diff --git a/docs/BUGS.md b/docs/BUGS.md index 30d333d..4694f51 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -100,6 +100,7 @@ a live account since. | Entry | Title | |---|---| +| [B240](#b240--pdfs-mount-can-be-core-dumped-with-the-keys-in-memory) | pdfs mount can be core-dumped with the keys in memory | | [B239](#b239----wait-reports-success-for-imports-and-re-dates-that-failed) | --wait reports success for imports and re-dates that failed | | [B238](#b238--draining-a-large-offline-batch-is-quadratic-in-its-size) | Draining a large offline batch is quadratic in its size | | [B237](#b237--every-reconnect-resends-refused-uploads-all-in-the-same-second) | Every reconnect resends refused uploads, all in the same second | @@ -342,6 +343,36 @@ a live account since. Newest first. +### B240 — pdfs mount can be core-dumped with the keys in memory + +**Status:** Fixed, not verified live +**Found:** 2026-10-16, architecture audit (`FINDINGS.md` section 3, core dumps and the +file-manager script). +**Where:** `main` in `crates/pdfs-cli/src/main.rs`; `script_body` in +`crates/pdfs-core/src/shell.rs` + +**Repro:** +- Run `pdfs mount` and send it `SIGSEGV`, with core dumps enabled. The dump holds the key + passphrases. +- In the mount, name a file `--json` and choose **Scripts → Keep offline** on it in Nautilus. + +**Cause.** Only `pdfs daemon` marked itself non-dumpable. `pdfs mount`, `login`, `unlock` and +`logout` hold the password, the key passphrases or the session tokens too, and could be +core-dumped or ptraced by another process of the same user. Separately, the file-manager script +ran `pdfs pin "$path"`, so a file named like an option was read as one. + +**Fix.** Those five commands mark themselves non-dumpable at start. The script passes each path +after `--`. A name with a newline still arrives split from Nautilus, whose list is +newline-separated; `pdfs` then fails on both halves and changes nothing. + +The app's own login was left as it is: a non-dumpable GUI process hides its `/proc` entries from +the desktop portal, which identifies the caller through them. + +**Test:** `tests::a_path_after_a_double_dash_is_a_path_even_when_it_looks_like_a_flag` in +`crates/pdfs-cli/src/main.rs`; `shell::tests::the_script_ends_options_before_each_path`. + +--- + ### B239 — --wait reports success for imports and re-dates that failed **Status:** Fixed, not verified live