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).
This commit is contained in:
+101
-15
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user