From ea70ddc10d1f9ccd1bf1444e538d509f690f49c4 Mon Sep 17 00:00:00 2001 From: Valentin Haudiquet Date: Tue, 15 Sep 2026 23:47:21 +0200 Subject: [PATCH] apt: keep the keyring cache private to the invoking user The shared world-writable /tmp/pkh-keyrings directory, combined with the skip-if-exists logic, let any local user pre-plant keyrings that pkh then trusts into the chroot's trusted.gpg.d. Use a per-uid 0700 directory instead, refuse to reuse a pre-existing directory that is not owned by the current user or is group/other-writable, and drop the now unnecessary world-accessibility chmods (mmdebstrap in unshare mode runs with the same real uid). --- src/apt/keyring.rs | 116 +++++++++++++++++++++++++++++++++++++++------ src/prune.rs | 66 ++++++++++++++++++++++---- 2 files changed, 158 insertions(+), 24 deletions(-) diff --git a/src/apt/keyring.rs b/src/apt/keyring.rs index 8a7ef1a..3a86671 100644 --- a/src/apt/keyring.rs +++ b/src/apt/keyring.rs @@ -7,6 +7,7 @@ use crate::context; use crate::distro_info; use serde::Deserialize; use std::error::Error; +use std::os::unix::fs::MetadataExt; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -47,19 +48,48 @@ pub async fn download_cache_keyrings( // Use system temp directory for keyrings since it's accessible from unshare mode // The home directory may not be accessible from mmdebstrap's unshare namespace let temp_dir = std::env::temp_dir(); - let keyring_dir = temp_dir.join("pkh-keyrings"); + // Name the cache directory per-uid: a single shared /tmp directory would + // be writable by any local user, and the skip-if-exists logic below + // trusts pre-existing keyrings, so it must never be shared. + let euid = current_euid(); + let keyring_dir = temp_dir.join(format!("pkh-keyrings-{euid}")); - // Create keyring directory if it doesn't exist - if !ctx.exists(&keyring_dir)? { - ctx.command("mkdir").arg("-p").arg(&keyring_dir).status()?; + if ctx.exists(&keyring_dir)? { + if let context::ContextConfig::Local = ctx.config { + // Cached keyrings are trusted as-is whenever they already exist, + // so refuse to reuse a directory that is not owned by the current + // user or is writable by group/others (it could have been planted + // by another local user). + let metadata = std::fs::symlink_metadata(&keyring_dir)?; + validate_keyring_dir(metadata.uid(), metadata.mode(), euid).map_err(|reason| { + format!( + "Refusing to use keyring cache directory {}: {reason}; \ + remove the directory and re-run pkh", + keyring_dir.display() + ) + })?; + } else { + // Remote contexts (e.g. ssh) have no stat/metadata access through + // the context API, so the ownership guard cannot be performed; + // keep the previous best-effort behavior of tightening the + // directory permissions instead (0700 instead of the former + // world-writable a+rwx). + ctx.command("chmod").arg("700").arg(&keyring_dir).status()?; + } + } else { + // Create the directory private to the invoking user (0700). This is + // sufficient for mmdebstrap in unshare mode: it runs with the same + // real uid (the user namespace only maps that uid to root, file + // access still happens as the real uid), so no world-accessible + // permissions are needed. + ctx.command("mkdir") + .arg("-p") + .arg("-m") + .arg("700") + .arg(&keyring_dir) + .status()?; } - // Make keyring directory world-accessible so mmdebstrap in unshare mode can access it - ctx.command("chmod") - .arg("a+rwx") - .arg(&keyring_dir) - .status()?; - for keyring_url in keyring_urls { // Extract the original filename from the keyring URL let filename = keyring_url @@ -116,9 +146,6 @@ pub async fn download_cache_keyrings( let _ = ctx.command("rm").arg("-f").arg(&download_path).status(); } - // Make the keyring file world-readable so mmdebstrap in unshare mode can access it - ctx.command("chmod").arg("a+r").arg(&binary_path).status()?; - log::info!( "Successfully downloaded keyring for {} to {}", series, @@ -129,8 +156,6 @@ pub async fn download_cache_keyrings( "Keyring already exists at {}, skipping download", binary_path.display() ); - // Ensure existing keyring is world-readable - ctx.command("chmod").arg("a+r").arg(&binary_path).status()?; } } @@ -143,6 +168,67 @@ pub async fn download_cache_keyrings( Ok(keyring_dir) } +/// Effective uid of the current process +fn current_euid() -> u32 { + unsafe { libc::geteuid() } +} + +/// Check that an existing keyring cache directory is safe to reuse +/// +/// Cached keyrings are trusted whenever the files already exist (see the +/// skip-if-exists logic in [`download_cache_keyrings`]), so the directory +/// must be owned by the current user and must not be writable by group or +/// others, otherwise another local user could plant a malicious keyring. +/// +/// Takes the directory's owner uid and permission mode (e.g. from +/// `std::fs::symlink_metadata`) so it can be unit tested without touching +/// the filesystem. +fn validate_keyring_dir(dir_uid: u32, mode: u32, euid: u32) -> Result<(), String> { + if dir_uid != euid { + return Err(format!( + "owned by uid {dir_uid}, not by the current user (uid {euid})" + )); + } + if mode & 0o022 != 0 { + return Err(format!( + "writable by group or others (permissions {:04o})", + mode & 0o7777 + )); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_validate_keyring_dir_accepts_private_dir_owned_by_current_user() { + assert!(validate_keyring_dir(1000, 0o700, 1000).is_ok()); + assert!(validate_keyring_dir(1000, 0o750, 1000).is_ok()); + assert!(validate_keyring_dir(1000, 0o1744, 1000).is_ok()); + assert!(validate_keyring_dir(0, 0o700, 0).is_ok()); + } + + #[test] + fn test_validate_keyring_dir_rejects_foreign_owner() { + let err = validate_keyring_dir(1000, 0o700, 1001).unwrap_err(); + assert!(err.contains("owned by uid 1000")); + let err = validate_keyring_dir(1001, 0o700, 1000).unwrap_err(); + assert!(err.contains("owned by uid 1001")); + } + + #[test] + fn test_validate_keyring_dir_rejects_group_or_other_writable() { + assert!(validate_keyring_dir(1000, 0o770, 1000).is_err()); + assert!(validate_keyring_dir(1000, 0o706, 1000).is_err()); + assert!(validate_keyring_dir(1000, 0o707, 1000).is_err()); + assert!(validate_keyring_dir(1000, 0o777, 1000).is_err()); + // Sticky bit does not neutralize the group/other write bits. + assert!(validate_keyring_dir(1000, 0o1777, 1000).is_err()); + } +} + /// Download and import a PPA key using Launchpad API /// /// # Arguments diff --git a/src/prune.rs b/src/prune.rs index 8f2737d..91d72b3 100644 --- a/src/prune.rs +++ b/src/prune.rs @@ -11,7 +11,8 @@ //! unmounting before they can be removed. //! - **Cached chroot tarballs** (`~/.cache/pkh/*-buildd.tar.xz`) and their //! **stale download lockfiles** (`~/.cache/pkh/*.lock`). -//! - **The shared apt keyring directory** (`/tmp/pkh-keyrings`), used by +//! - **The apt keyring cache directories** (`pkh-keyrings` and the per-uid +//! `pkh-keyrings-` under the system temp directory), used by //! mmdebstrap runs. //! - **Build logs** (`~/.cache/pkh/logs/deb-*.log`) written by `pkh deb`. By //! default only logs beyond a small retention window (the newest @@ -67,7 +68,8 @@ pub(crate) const KEEP_LOGS: usize = 10; enum Artifact { /// A residual build/chroot directory under the system temp dir. TempDir(PathBuf), - /// The shared apt keyring directory (`/tmp/pkh-keyrings`). + /// The apt keyring cache directory (`pkh-keyrings*` under the system + /// temp dir). KeyringDir(PathBuf), /// A stale chroot tarball download lockfile. LockFile(PathBuf), @@ -105,11 +107,24 @@ pub fn cache_dir() -> Option { ProjectDirs::from("com", "pkh", "pkh").map(|d| d.cache_dir().to_path_buf()) } +/// Check whether a directory entry name is the apt keyring cache directory, +/// i.e. the legacy shared `pkh-keyrings` name or the per-uid +/// `pkh-keyrings-` used by current pkh versions. +fn is_keyring_dir_name(name: &str) -> bool { + if name == "pkh-keyrings" { + return true; + } + match name.strip_prefix("pkh-keyrings-") { + Some(uid) => !uid.is_empty() && uid.bytes().all(|b| b.is_ascii_digit()), + None => false, + } +} + /// Check whether a directory entry name is a residual pkh build directory, /// i.e. it matches `pkh-` or `pkh--`. /// -/// This deliberately rejects the `pkh-keyrings` and `pkh-build-*` names so they -/// are not mistaken for residual build chroots. +/// This deliberately rejects the `pkh-keyrings*` and `pkh-build-*` names so +/// they are not mistaken for residual build chroots. fn is_residual_temp_dir(name: &str) -> bool { let Some(rest) = name.strip_prefix("pkh-") else { return false; @@ -189,7 +204,7 @@ fn discover_artifacts(temp_dir: &Path, cache_dir: Option<&Path>) -> Vec temp_dirs += 1, - Artifact::KeyringDir(_) => keyring = true, + Artifact::KeyringDir(_) => keyring_dirs += 1, Artifact::LockFile(_) => locks += 1, Artifact::Tarball(_) => tarballs += 1, Artifact::LogFile(_) => logs += 1, } } assert_eq!(temp_dirs, 2); - assert!(keyring); + assert_eq!(keyring_dirs, 2); assert_eq!(locks, 1); assert_eq!(tarballs, 2); assert_eq!(logs, 1); @@ -564,6 +596,7 @@ none /tmp/other proc rw 0 0 let dir = temp_path.join("pkh-1700000000"); fs::create_dir_all(&dir).unwrap(); fs::create_dir_all(temp_path.join("pkh-keyrings")).unwrap(); + fs::create_dir_all(temp_path.join("pkh-keyrings-1000")).unwrap(); fs::write(cache_path.join("noble-buildd.tar.xz"), "tarball").unwrap(); fs::write(cache_path.join("noble-buildd.tar.lock"), "lock").unwrap(); @@ -587,6 +620,12 @@ none /tmp/other proc rw 0 0 .iter() .any(|p| p == &temp_path.join("pkh-keyrings")) ); + assert!( + report + .removed + .iter() + .any(|p| p == &temp_path.join("pkh-keyrings-1000")) + ); assert!( report .removed @@ -604,6 +643,7 @@ none /tmp/other proc rw 0 0 // Nothing was actually removed. assert!(dir.exists()); assert!(temp_path.join("pkh-keyrings").exists()); + assert!(temp_path.join("pkh-keyrings-1000").exists()); assert!(cache_path.join("noble-buildd.tar.xz").exists()); assert!(cache_path.join("noble-buildd.tar.lock").exists()); } @@ -619,6 +659,7 @@ none /tmp/other proc rw 0 0 let cache_path = cache.path(); fs::create_dir_all(temp_path.join("pkh-keyrings")).unwrap(); + fs::create_dir_all(temp_path.join("pkh-keyrings-1000")).unwrap(); fs::write(cache_path.join("noble-buildd.tar.lock"), "lock").unwrap(); fs::write(cache_path.join("noble-buildd.tar.xz"), "tarball").unwrap(); @@ -639,6 +680,12 @@ none /tmp/other proc rw 0 0 .iter() .any(|p| p == &temp_path.join("pkh-keyrings")) ); + assert!( + report + .removed + .iter() + .any(|p| p == &temp_path.join("pkh-keyrings-1000")) + ); assert!( report .removed @@ -647,6 +694,7 @@ none /tmp/other proc rw 0 0 ); assert!(!temp_path.join("pkh-keyrings").exists()); + assert!(!temp_path.join("pkh-keyrings-1000").exists()); assert!(!cache_path.join("noble-buildd.tar.lock").exists()); // Tarball is preserved when --all is not set. assert!(cache_path.join("noble-buildd.tar.xz").exists());