From c62edd331739910ac0885de9382d41da856cfa17 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 30 Aug 2026 17:29:42 +0200 Subject: [PATCH] Give the log a mode that the way off the device can open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The log file has been on external storage since it existed, and the reason is stated at length in two places: /data/data//files needs run-as against a debuggable build, /sdcard/Android/data//files is a plain adb pull from any build, and a log nobody can retrieve is not a diagnostic. The file was then opened 0600, which cancels that decision out. On the tablet: adb pull -> remote open failed: Permission denied adb shell cat -> Permission denied run-as -> package not debuggable Every route off the device closed at once, on a file whose whole purpose is to leave the device. The mode is now per platform, because "who may read this" has two different answers and the directory above the file is what makes them differ. On the desktop, 0600 as before: $XDG_STATE_HOME/darkroom is in a home directory on a machine that may have other accounts, and nothing about that directory stops another local user reading a world-readable file. On Android, 0644: /sdcard/Android/data is drwxrws--x media_rw:ext_data_rw, so no other app can enter this app'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 read bits buy is adb pull, which runs as shell — able to traverse a --x directory, but then obliged to open the file as other. The mode is also applied twice, and the second one is the fix rather than belt and braces. OpenOptions::mode is a request: the kernel ANDs it with the process umask, and an Android application process inherits 0o077 from the zygote, so asking for 0644 there creates 0600 and reports nothing. It is ignored outright on a file that already exists, which every launch after the first has. fchmod is subject to neither, and is what the second call makes. The comment claiming the mode was "ignored by the FAT-derived filesystem Android presents as external storage" is gone with it. The device says otherwise: the file it produced was 0600 exactly. Two tests. One pins the literal mode per platform — only the desktop arm can run under cargo test, and the comment says so rather than implying the Android number is covered. The other reopens a log left behind with the wrong mode, which is the one assertion on the host that fails if the fchmod is deleted, since OpenOptions::mode cannot touch a file that is already there. Co-Authored-By: Claude Opus 5 (1M context) --- platform/dr-plat/src/diagnostics.rs | 139 ++++++++++++++++++++++++++-- 1 file changed, 131 insertions(+), 8 deletions(-) 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