diff --git a/platform/dr-plat/src/diagnostics.rs b/platform/dr-plat/src/diagnostics.rs index 1edad86..bffe4a7 100644 --- a/platform/dr-plat/src/diagnostics.rs +++ b/platform/dr-plat/src/diagnostics.rs @@ -23,8 +23,15 @@ //! it was, which matters because losing it while debugging would make this a //! downgrade. [`install`] is the whole of the wiring. //! -//! Three properties, each of which is a test below: +//! Four properties, each of which is a test below — the first of them only +//! half-testable off a device, and the test says which half: //! +//! * **Retrievable.** Created with a mode that the way *this* platform gets a +//! file off the machine can actually open: `FILE_MODE`, the one number here +//! that differs by platform, and the only one whose reasoning is about the +//! directory above the file rather than about the file. Get it wrong and the +//! other three properties are worth nothing, because nobody ever reads the +//! log. //! * **Capped.** Two files of [`MAX_FILE_BYTES`], so the worst case is stated //! rather than discovered on a full phone. //! * **Redacted at the sink** ([`redact`]), not at the call sites. A rule @@ -284,20 +291,64 @@ impl LogFile { } } +/// The mode the log is created with, on the desktop. +/// +/// `$XDG_STATE_HOME/darkroom` is inside a home directory on a machine that may +/// have other accounts on it, and this file names the user's photographs and +/// the shape of their library. Nothing about the directory above it stops +/// another local user reading a world-readable file, so the owner is the only +/// reader who has any business with it. +#[cfg(all(unix, not(target_os = "android")))] +const FILE_MODE: u32 = 0o600; + +/// The mode the log is created with, on Android — deliberately not the +/// desktop's, because "who may read this" has a different answer here. +/// +/// **The directory is the access control, and the file being readable does not +/// widen it.** `/sdcard/Android/data` is `drwxrws--x media_rw:ext_data_rw`: no +/// other application can enumerate or enter this application's subdirectory, +/// and anyone who can traverse it is holding the unlocked tablet, which +/// already gets them the photographs the log merely names. +/// +/// What the group and other read bits buy is the entire reason +/// [`crate::state`] puts the file on external storage rather than in +/// `/data/data`: **`adb pull` runs as `shell`**, which can traverse a `--x` +/// directory but must then open the file as *other*. At `0o600` it cannot — +/// and `run-as` is refused on a build that is not debuggable, which a release +/// build is not — so every route off the device is closed at once and the log +/// is a diagnostic nobody can be sent. That is the one outcome the choice of +/// directory exists to prevent. +#[cfg(target_os = "android")] +const FILE_MODE: u32 = 0o644; + fn open_append(path: &Path) -> io::Result { let mut options = OpenOptions::new(); options.create(true).append(true); - // The log names the user's photographs and the shape of their library, - // which is nobody else's business on a shared machine. Applied at - // creation only, which is all that is needed, and ignored by the - // FAT-derived filesystem Android presents as external storage — where the - // directory is already scoped to this application. #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt as _; - options.mode(0o600); + options.mode(FILE_MODE); } - options.open(path) + let file = options.open(path)?; + + // And then again, explicitly, because the line above is a *request*: the + // kernel ANDs it with the process umask, and an Android application + // process inherits `0o077` from the zygote. Asking for `0o644` there + // creates `0o600`, silently, and `OpenOptions::mode` is ignored outright + // on a file that already exists — between them, that is how a log written + // for `adb pull` ended up unreadable by it. `fchmod` is subject to + // neither. + // + // Ignored on failure rather than refused. A file this process does not + // own, or a filesystem with no modes to set, is a reason to log without + // the mode; it is not a reason to log nowhere. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + let _ = file.set_permissions(fs::Permissions::from_mode(FILE_MODE)); + } + + Ok(file) } /// One record as one line. @@ -418,6 +469,78 @@ mod tests { format!("{n:04} scanning /home/duncan/Pictures/2026/Iceland for images") } + /// The permission bits of a file, as an octal number to compare against. + #[cfg(unix)] + fn mode_of(path: &Path) -> u32 { + use std::os::unix::fs::PermissionsExt as _; + fs::metadata(path).expect("a log").permissions().mode() & 0o777 + } + + #[cfg(unix)] + #[test] + fn the_log_is_created_with_the_mode_its_platform_can_retrieve_it_by() { + let dir = a_log_dir("mode"); + let log = LogFile::open_in(&dir, MAX_FILE_BYTES).expect("opens"); + log.write_line("a line the user will be asked for"); + + // Written as a literal rather than compared against `FILE_MODE`: a + // test that reads the same constant the code writes agrees with + // whatever the constant says, which is not a test of anything. + // + // **Only one of these two arms can ever run.** The Android one is the + // one that matters — `0o600` there closes `adb pull` and `run-as` at + // once, which is the failure this test exists for — and it cannot run + // under `cargo test`, because the behaviour is a property of a + // filesystem and a umask that only exist on a device. What the desktop + // arm does prove is the other half, and it is not nothing: the log + // must not become group- or world-readable in a home directory + // because somebody made one mode serve both platforms. + #[cfg(target_os = "android")] + assert_eq!( + mode_of(&dir.join(FILE_NAME)), + 0o644, + "a log only the app can read cannot be sent to anyone (NFR-OPS-1)" + ); + #[cfg(not(target_os = "android"))] + assert_eq!( + mode_of(&dir.join(FILE_NAME)), + 0o600, + "the log names the user's library, and every other account on \ + this machine can now read it" + ); + } + + #[cfg(unix)] + #[test] + fn a_log_left_by_an_older_build_is_reopened_with_this_mode() { + // The regression test for the actual bug, and the only one that can + // fail on the host: `OpenOptions::mode` applies at *creation* and is + // ANDed with the umask even then, so on its own it neither fixes an + // existing file nor delivers what it asked for in a process whose + // umask is `0o077`, which every Android application process inherits + // from the zygote. + // + // The explicit `set_permissions` is what makes the mode the one the + // platform needs rather than the one the umask happened to allow, and + // deleting it makes this assertion fail: the file already exists, so + // nothing else here can change it. + use std::os::unix::fs::PermissionsExt as _; + + let dir = a_log_dir("mode-existing"); + let path = dir.join(FILE_NAME); + fs::write(&path, "written by a build that chose a different mode\n").expect("writes"); + fs::set_permissions(&path, fs::Permissions::from_mode(0o666)).expect("chmods"); + + let log = LogFile::open_in(&dir, MAX_FILE_BYTES).expect("opens"); + log.write_line("this session"); + + assert_eq!( + mode_of(&path), + FILE_MODE, + "the mode of an existing log is left as whoever created it left it" + ); + } + #[test] fn the_log_stays_under_its_stated_cap() { // **This is NFR-OPS-1's "size-capped".** Without rotation a phone left