From 9fa7508bf66d661065e86fe2dff5bc8dfd831d13 Mon Sep 17 00:00:00 2001 From: HashEngineering Date: Wed, 7 Oct 2026 22:17:30 -0700 Subject: [PATCH 1/2] fix(platform-wallet-storage): trust Android's platform and per-app identities in the parent-directory walk On Android every app's private storage sits beneath system:system 0771 directories (/data, /data/user/0, /data/data) created by init and vold, and Context.getFilesDir()/getDatabasesDir() create the app's own directories 0771 in its per-app group (gid equal to its uid). The ancestor walk rejected both, so SqlitePersister::open, the secrets vault and backup/restore could not open any app-private path on Android. On target_os = "android": - AID_SYSTEM (1000) is trusted as an ancestor owner and as the group of a group-writable ancestor. No app can run as or join system. - The app's own per-app group (the current uid) is trusted as the group of a group-writable ancestor. No other app can join it. Other-writable ancestors, ancestors writable by any other group (including the app's cache gid), and ancestors owned by another app's uid are still refused. Both trusted sets are empty on every other target, so desktop and iOS behaviour is unchanged; a user's primary group off Android (macOS staff, for one) is often shared. Verified on an Android 15 emulator, run as an app uid: files/native/dashshielded/wallet.db opens and reopens, the /data/data alias opens, and a 0777 ancestor is still refused. Reported by Edge integrating 4.2.0-beta.7. Co-Authored-By: Claude Opus 5.5 --- .../rs-platform-wallet-storage/SECRETS.md | 12 +- .../src/parent_permissions.rs | 128 ++++++++++++++++-- 2 files changed, 128 insertions(+), 12 deletions(-) diff --git a/packages/rs-platform-wallet-storage/SECRETS.md b/packages/rs-platform-wallet-storage/SECRETS.md index 3f5c23c5cc1..aabdb50620e 100644 --- a/packages/rs-platform-wallet-storage/SECRETS.md +++ b/packages/rs-platform-wallet-storage/SECRETS.md @@ -406,7 +406,17 @@ unwrapped copy is allocated. depends on both). A sticky writable directory such as `/tmp` (`0o1777`) is accepted — the sticky bit is what stops one user replacing another's entries. A read-only group-accessible ancestor (`0o750`) is accepted too - — it only leaks filenames, never the 0600-protected vault contents. The + — it only leaks filenames, never the 0600-protected vault contents. On + Android, `system` (`AID_SYSTEM`, 1000) is trusted as well, both as an + owner and as the group of a group-writable ancestor: Android creates every + app's private storage beneath `system:system` `0771` directories (`/data`, + `/data/user/0`, `/data/data`), and no app can run as or join `system`. + The app's own per-app group (gid equal to its uid) is trusted as the group + of a group-writable ancestor too, because Android creates the app's + `files` and `databases` directories `0771` in that group. Other-writable + ancestors and ancestors writable by any other group are still refused + there. The trusted sets are `PLATFORM_IDS` and `trusted_groups` in + `src/parent_permissions.rs`, empty on every other target. The walk is Unix-only; Windows ACLs are not inspected (issue #3754). Each secret is capped at `MAX_SECRET_LEN` (8176 B) at the write boundary — still ~30× any mnemonic/seed/xpriv — so a single oversized diff --git a/packages/rs-platform-wallet-storage/src/parent_permissions.rs b/packages/rs-platform-wallet-storage/src/parent_permissions.rs index 9359a21d6cb..0cd741ba223 100644 --- a/packages/rs-platform-wallet-storage/src/parent_permissions.rs +++ b/packages/rs-platform-wallet-storage/src/parent_permissions.rs @@ -63,9 +63,54 @@ pub(crate) fn insecure_ancestor_message( } } +/// Platform identities trusted as ancestor owners and as the group of a +/// group-writable ancestor, beyond the current user and root. +/// +/// Android creates every app's private storage beneath `/data`, `/data/user`, +/// `/data/user/0` and `/data/data`, all owned `system:system` (`AID_SYSTEM`, +/// 1000) and mostly `0771`. No app-private path can pass the walk without +/// trusting that identity. `AID_SYSTEM` is the platform itself — it already +/// administers every app's data — and no third-party app can run as it or join +/// its group, so a `system`-group-writable ancestor is not replaceable by +/// anything the check defends against. +#[cfg(target_os = "android")] +const PLATFORM_IDS: &[u32] = &[1000]; +#[cfg(all(unix, not(target_os = "android")))] +const PLATFORM_IDS: &[u32] = &[]; + #[cfg(unix)] -fn trusted_owner(owner: u32, current_uid: u32, root_uid: u32) -> bool { - owner == current_uid || owner == 0 || owner == root_uid +fn trusted_owner(owner: u32, current_uid: u32, root_uid: u32, platform_ids: &[u32]) -> bool { + owner == current_uid || owner == 0 || owner == root_uid || platform_ids.contains(&owner) +} + +/// Groups whose write access to an ancestor is not a replacement risk. +/// +/// On Android this is `PLATFORM_IDS` plus the app's own per-app group. Android +/// gives every app a gid equal to its uid, and `Context.getFilesDir()`, +/// `getDatabasesDir()` and friends create the app's own directories `0771` +/// in that group, so `/files` is group-writable by design. No other +/// app can be a member of that group. Everywhere else the set is empty: a +/// desktop user's primary group (macOS `staff`, for one) is commonly shared. +#[cfg(target_os = "android")] +fn trusted_groups(current_uid: u32) -> Vec { + let mut groups = PLATFORM_IDS.to_vec(); + groups.push(current_uid); + groups +} +#[cfg(all(unix, not(target_os = "android")))] +fn trusted_groups(_current_uid: u32) -> Vec { + PLATFORM_IDS.to_vec() +} + +/// Whether an identity outside the trusted set can replace entries in a +/// directory with this mode and group: other-writable, or group-writable for a +/// group outside `trusted_groups`, in both cases without the sticky bit. +#[cfg(unix)] +fn writable_without_sticky(mode: u32, gid: u32, trusted_groups: &[u32]) -> bool { + if mode & 0o1000 != 0 { + return false; + } + mode & 0o002 != 0 || (mode & 0o020 != 0 && !trusted_groups.contains(&gid)) } #[cfg(unix)] @@ -98,7 +143,7 @@ fn reject_ancestor( ) -> Result<(), ParentPermissionsError> { let reason = if writable_without_sticky { InsecureAncestor::WritableWithoutSticky { mode } - } else if !trusted_owner(uid, current_uid, root_uid) { + } else if !trusted_owner(uid, current_uid, root_uid, PLATFORM_IDS) { InsecureAncestor::UntrustedOwner { uid, current_uid } } else { return Ok(()); @@ -114,6 +159,7 @@ pub(crate) fn check_parent_perms(parent: &Path) -> Result<(), ParentPermissionsE use std::os::unix::fs::MetadataExt; let current_uid = effective_uid(); + let trusted_groups = trusted_groups(current_uid); let root_uid = std::fs::metadata("/") .map_err(ParentPermissionsError::Io)? .uid(); @@ -129,8 +175,8 @@ pub(crate) fn check_parent_perms(parent: &Path) -> Result<(), ParentPermissionsE for ancestor in absolute.ancestors() { let meta = std::fs::symlink_metadata(ancestor).map_err(ParentPermissionsError::Io)?; let mode = meta.mode() & 0o7777; - let writable_without_sticky = - !meta.file_type().is_symlink() && mode & 0o022 != 0 && mode & 0o1000 == 0; + let writable_without_sticky = !meta.file_type().is_symlink() + && writable_without_sticky(mode, meta.gid(), &trusted_groups); reject_ancestor( ancestor, meta.uid(), @@ -143,7 +189,7 @@ pub(crate) fn check_parent_perms(parent: &Path) -> Result<(), ParentPermissionsE for ancestor in canonical.ancestors() { let meta = std::fs::metadata(ancestor).map_err(ParentPermissionsError::Io)?; let mode = meta.mode() & 0o7777; - let writable_without_sticky = mode & 0o022 != 0 && mode & 0o1000 == 0; + let writable_without_sticky = writable_without_sticky(mode, meta.gid(), &trusted_groups); reject_ancestor( ancestor, meta.uid(), @@ -165,13 +211,73 @@ pub(crate) fn check_parent_perms(_parent: &Path) -> Result<(), ParentPermissions #[cfg(all(test, unix))] mod tests { - use super::trusted_owner; + use super::{trusted_owner, writable_without_sticky}; + + const ANDROID_SYSTEM: &[u32] = &[1000]; #[test] fn trusted_owner_accepts_current_and_root_identities_only() { - assert!(trusted_owner(1000, 1000, 65_534)); - assert!(trusted_owner(0, 1000, 65_534)); - assert!(trusted_owner(65_534, 1000, 65_534)); - assert!(!trusted_owner(2000, 1000, 65_534)); + assert!(trusted_owner(1000, 1000, 65_534, &[])); + assert!(trusted_owner(0, 1000, 65_534, &[])); + assert!(trusted_owner(65_534, 1000, 65_534, &[])); + assert!(!trusted_owner(2000, 1000, 65_534, &[])); + } + + #[test] + fn should_trust_android_system_owner_only_when_it_is_a_platform_identity() { + // App uid 10123 under /data/user/0 (owner system, 1000). + assert!(!trusted_owner(1000, 10_123, 0, &[])); + assert!(trusted_owner(1000, 10_123, 0, ANDROID_SYSTEM)); + // Another app's uid stays untrusted. + assert!(!trusted_owner(10_456, 10_123, 0, ANDROID_SYSTEM)); + } + + #[test] + fn should_reject_group_write_unless_group_is_a_platform_identity() { + // `/data` on Android: system:system 0771. + assert!(writable_without_sticky(0o771, 1000, &[])); + assert!(!writable_without_sticky(0o771, 1000, ANDROID_SYSTEM)); + // Group-writable for any other group is still replaceable. + assert!(writable_without_sticky(0o775, 1015, ANDROID_SYSTEM)); + assert!(writable_without_sticky(0o770, 0, ANDROID_SYSTEM)); + } + + #[test] + fn should_accept_app_files_dir_group_writable_by_the_apps_own_group() { + // `/files` on Android: app uid 10150, group 10150, 0771. + // `trusted_groups(10_150)` on Android is `[1000, 10_150]`. + let android_app: &[u32] = &[1000, 10_150]; + assert!(!writable_without_sticky(0o771, 10_150, android_app)); + // Another app's group, and the cache gid (20000 + app id), stay refused. + assert!(writable_without_sticky(0o771, 10_456, android_app)); + assert!(writable_without_sticky(0o2771, 20_150, android_app)); + // Other-write is refused even in the app's own group. + assert!(writable_without_sticky(0o777, 10_150, android_app)); + } + + #[cfg(not(target_os = "android"))] + #[test] + fn should_trust_no_extra_groups_off_android() { + assert!(super::trusted_groups(501).is_empty()); + // A user-owned, group-writable folder (macOS `staff` is gid 20) is + // still refused on desktop. + assert!(writable_without_sticky( + 0o775, + 20, + &super::trusted_groups(501) + )); + } + + #[test] + fn should_reject_other_write_regardless_of_group() { + assert!(writable_without_sticky(0o757, 1000, ANDROID_SYSTEM)); + assert!(writable_without_sticky(0o777, 1000, ANDROID_SYSTEM)); + } + + #[test] + fn should_accept_sticky_and_unwritable_directories() { + assert!(!writable_without_sticky(0o1777, 2000, &[])); + assert!(!writable_without_sticky(0o755, 2000, &[])); + assert!(!writable_without_sticky(0o700, 2000, &[])); } } From 917b18c22f00697e6094495658b4d338df7e93a6 Mon Sep 17 00:00:00 2001 From: HashEngineering Date: Fri, 23 Oct 2026 16:06:46 -0700 Subject: [PATCH 2/2] docs(platform-wallet-storage): qualify the ancestor rules for Android and explain the uid-keyed group SECRETS.md stated the group-write and owner rejections unconditionally a few lines before describing the Android exceptions. The general rule now says both have Android exceptions. trusted_groups gains a note on why the app's own group is keyed on the uid rather than getegid(): the two are equal in every app process, but only the uid-numbered gid is reserved for that app alone. Co-Authored-By: Claude Opus 5.5 --- packages/rs-platform-wallet-storage/SECRETS.md | 3 ++- .../rs-platform-wallet-storage/src/parent_permissions.rs | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/rs-platform-wallet-storage/SECRETS.md b/packages/rs-platform-wallet-storage/SECRETS.md index aabdb50620e..0cad64d2e54 100644 --- a/packages/rs-platform-wallet-storage/SECRETS.md +++ b/packages/rs-platform-wallet-storage/SECRETS.md @@ -400,7 +400,8 @@ unwrapped copy is allocated. symlink cannot hide an unsafe ancestor behind a safe-looking one. An ancestor is refused at `open` with `SecretStoreError::InsecureParentDir` when it is group/other writable (`mode & 0o022`) WITHOUT the sticky bit, - or when it is owned by neither the effective user nor a root identity: + or when it is owned by neither the effective user nor a root identity + (both rules make exceptions for the Android identities trusted below): directory write access governs rename/replace of the vault, and an untrusted owner can grant itself that access at will (the A1 guarantee depends on both). A sticky writable directory such as `/tmp` (`0o1777`) diff --git a/packages/rs-platform-wallet-storage/src/parent_permissions.rs b/packages/rs-platform-wallet-storage/src/parent_permissions.rs index 0cd741ba223..7d8dc9f60c2 100644 --- a/packages/rs-platform-wallet-storage/src/parent_permissions.rs +++ b/packages/rs-platform-wallet-storage/src/parent_permissions.rs @@ -91,6 +91,12 @@ fn trusted_owner(owner: u32, current_uid: u32, root_uid: u32, platform_ids: &[u3 /// in that group, so `/files` is group-writable by design. No other /// app can be a member of that group. Everywhere else the set is empty: a /// desktop user's primary group (macOS `staff`, for one) is commonly shared. +/// +/// The group is keyed on the uid, not `getegid()`. The two are equal in every +/// app process, but only the uid carries the guarantee: Android reserves the +/// gid numbered like an app's uid for that app alone, while the effective gid +/// is whatever group the process was started with, which outside an app +/// process can be one that other processes share. #[cfg(target_os = "android")] fn trusted_groups(current_uid: u32) -> Vec { let mut groups = PLATFORM_IDS.to_vec();