No functional change. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- tools/mount-flatpak/src/main.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tools/mount-flatpak/src/main.rs b/tools/mount-flatpak/src/main.rs index 6fc05569b9736e70311a7e2e20af4131a8d66303..dca42e5007e52b24c9a4b6940d5667f770a64bd0 100644 --- a/tools/mount-flatpak/src/main.rs +++ b/tools/mount-flatpak/src/main.rs @@ -102,15 +102,15 @@ fn run(mut args: ArgsOs) -> Result<(), String> { ex_usage(); } - let mut user_data = - Root::open(&user_data_path).map_err(|e| format!("opening user data partition: {e}"))?; - user_data.set_resolver_flags(ResolverFlags::NO_SYMLINKS); + let user_data = Root::open(&user_data_path) + .map_err(|e| format!("opening user data partition: {e}"))? + .with_resolver_flags(ResolverFlags::NO_SYMLINKS); - let mut source_installation_dir = user_data + let source_installation_dir = user_data .open_subpath(&installation_path, OpenFlags::O_PATH) .map(Root::from_fd) - .map_err(|e| format!("opening source flatpak installation: {e}"))?; - source_installation_dir.set_resolver_flags(ResolverFlags::NO_SYMLINKS); + .map_err(|e| format!("opening source flatpak installation: {e}"))? + .with_resolver_flags(ResolverFlags::NO_SYMLINKS); std::fs::create_dir("flatpak") .map_err(|e| format!("creating target flatpak installation: {e}"))?; @@ -124,8 +124,8 @@ fn run(mut args: ArgsOs) -> Result<(), String> { | OpenTreeFlags::AT_SYMLINK_NOFOLLOW, ) .map_err(|e| format!("opening target flatpak installation: {e}"))?; - let mut target_installation_dir = Root::from_fd(target_installation_dir); - target_installation_dir.set_resolver_flags(ResolverFlags::NO_SYMLINKS); + let target_installation_dir = + Root::from_fd(target_installation_dir).with_resolver_flags(ResolverFlags::NO_SYMLINKS); let mut full_app_path = installation_path.join("app"); full_app_path.push(&app); -- 2.52.0

This rejects non-directories where a directory is expected. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- tools/mount-flatpak/src/main.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tools/mount-flatpak/src/main.rs b/tools/mount-flatpak/src/main.rs index dca42e5007e52b24c9a4b6940d5667f770a64bd0..68a1f6ea308387a9471682c927d3a71439ab69e4 100644 --- a/tools/mount-flatpak/src/main.rs +++ b/tools/mount-flatpak/src/main.rs @@ -49,8 +49,8 @@ use std::os::unix::prelude::*; use std::path::{Path, PathBuf}; use std::process::exit; -use pathrs::Root; use pathrs::flags::{OpenFlags, ResolverFlags}; +use pathrs::{Root, error::Error}; use rustix::fs::{CWD, FileType, fstat}; use rustix::mount::{MoveMountFlags, OpenTreeFlags, move_mount, open_tree}; @@ -61,6 +61,11 @@ fn ex_usage() -> ! { exit(1); } +fn open_subdir(root: &Root, path: &Path) -> Result<Root, Error> { + root.open_subpath(path, OpenFlags::O_PATH | OpenFlags::O_DIRECTORY) + .map(|f| Root::from_fd(f).with_resolver_flags(ResolverFlags::NO_SYMLINKS)) +} + fn mount_commit( source_commit: &dyn AsFd, target_installation: &Root, @@ -106,11 +111,8 @@ fn run(mut args: ArgsOs) -> Result<(), String> { .map_err(|e| format!("opening user data partition: {e}"))? .with_resolver_flags(ResolverFlags::NO_SYMLINKS); - let source_installation_dir = user_data - .open_subpath(&installation_path, OpenFlags::O_PATH) - .map(Root::from_fd) - .map_err(|e| format!("opening source flatpak installation: {e}"))? - .with_resolver_flags(ResolverFlags::NO_SYMLINKS); + let source_installation_dir = open_subdir(&user_data, &installation_path) + .map_err(|e| format!("opening source flatpak installation: {e}"))?; std::fs::create_dir("flatpak") .map_err(|e| format!("creating target flatpak installation: {e}"))?; @@ -150,12 +152,11 @@ fn run(mut args: ArgsOs) -> Result<(), String> { full_app_path.pop(); full_app_path.push(&commit); - let source_app_dir = source_installation_dir - .resolve(&full_app_path) + let source_app_dir = open_subdir(&source_installation_dir, &full_app_path) .map_err(|e| format!("opening source app directory: {e}"))?; let metadata = source_installation_dir - .resolve(full_app_path.join("metadata")) + .resolve(&full_app_path.join("metadata")) .map_err(|e| format!("resolving app metadata: {e}"))?; let metadata_stat = @@ -182,8 +183,7 @@ fn run(mut args: ArgsOs) -> Result<(), String> { full_runtime_path.pop(); full_runtime_path.push(&runtime_commit); - let source_runtime_dir = source_installation_dir - .resolve(&full_runtime_path) + let source_runtime_dir = open_subdir(&source_installation_dir, &full_runtime_path) .map_err(|e| format!("opening source runtime directory: {e}"))?; mount_commit(&source_app_dir, &target_installation_dir, &full_app_path)?; -- 2.52.0

This is not a security problem, because libpathrs prevents directory traversal far better than mere string validation ever could. Instead, it serves to prevent excruciatingly unhelpful error messages. In particular, the application ID is user input and humans, being humans, will likely get it wrong at some point. Catching the error early allows the problem to be clearly explained, rather than continuing with nonsensical input and producing a nonsensical result. Malformed application IDs are rejected with an error message explaining what the problem is. Attempting to run an application from something that obviously isn't a flatpak repository returns an error explaining that this is the problem. Attempting to run an application that isn't installed tells the user exactly that, rather than a "no such file or directory" error. Problems that indicate a corrupt Flatpak repository are also detected. The restrictions on application IDs are future-proof because application IDs must be valid D-Bus well-known names, and the rules for what is a valid D-Bus well-known name are set in stone. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- tools/mount-flatpak/src/main.rs | 98 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 90 insertions(+), 8 deletions(-) diff --git a/tools/mount-flatpak/src/main.rs b/tools/mount-flatpak/src/main.rs index b40402b1b15729bdd2228025850efe8d87a48e3b..c22c3febf1ad8a574deee8d7371710c0de647ca1 100644 --- a/tools/mount-flatpak/src/main.rs +++ b/tools/mount-flatpak/src/main.rs @@ -50,7 +50,7 @@ use std::path::{Path, PathBuf}; use std::process::exit; use pathrs::flags::{OpenFlags, ResolverFlags}; -use pathrs::{Root, error::Error}; +use pathrs::{Root, error::Error, error::ErrorKind}; use rustix::fs::{CWD, FileType, fstat}; use rustix::mount::{MoveMountFlags, OpenTreeFlags, move_mount, open_tree}; @@ -66,6 +66,40 @@ fn open_subdir(root: &Root, path: &Path) -> Result<Root, Error> { .map(|f| Root::from_fd(f).with_resolver_flags(ResolverFlags::NO_SYMLINKS)) } +fn check_name(slice: &[u8], expected_slashes: usize, kind: &str) -> Result<(), String> { + // Application ID is provided by a human. Give useful error messages if it is wrong. + let mut components = 0usize; + for component in slice.split(|&s| s == b'/') { + components += 1; + match component { + // Leading, trailing, and repeated '/' introduce empty slices + // into the result, which are caught here. + &[] | &[b'.'] | &[b'.', b'.'] => { + return Err(format!("Path component {components} is {component:?}. Your Flatpak repository is corrupt.")); + } + _ => {} + } + if component.len() > 255 { + return Err(format!("{kind} has component with length over 255 bytes. Your Flatpak repository is corrupt")); + } + for c in component { + match c { + // This can happen for bad metadata files. + b'\0' => return Err(format!("{kind} contains a NUL byte. Your Flatpak metadata is bad.")), + _ => {} + } + } + } + components -= 1; + if expected_slashes != components { + return Err(format!( + "{kind} should have {expected_slashes} '/' characters, \ + but it has {components}. Your Flatpak repository is corrupt." + )); + } + Ok(()) +} + fn mount_commit( source_installation: &Root, source_path: &Path, @@ -78,6 +112,7 @@ fn mount_commit( let commit = source_commit_parent_dir .readlink("active") .map_err(|e| format!("reading active {kind} commit: {e}"))?; + check_name(commit.as_os_str().as_bytes(), 0, "commit")?; let source_root = open_subdir(&source_commit_parent_dir, &commit) .map_err(|e| format!("opening source {kind} commit: {e}"))?; let source_commit_tree = open_tree( @@ -103,6 +138,33 @@ fn mount_commit( Ok((source_root, commit.as_os_str().to_owned())) } +// Check that the application ID is reasonable. Problems with it +// are almost always human error, and it is possible to provide +// vastly better error messages this way. Furthermore, checking the +// application ID here means that if it is okay, it can be printed in +// logs without further sanitization. +fn check_app_id(app: &[u8]) -> Result<(), String> { + let app_len = app.len(); + if app_len > 255 { + // Avoid confusing ENAMETOOLONG error + return Err(format!("Application ID is {app_len} bytes, but the limit is 255")); + } + match app.get(0) { + None => return Err("Application ID can't be empty".to_string()), + Some(a) => match a { + b'.' | b'-' | b'/' => return Err(format!("Application ID can't start with '{a}'")), + _ => {} + }, + }; + for c in app { + match c { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'.' | b'-' | b'_' => {} + _ => return Err(format!("Application ID has forbidden characters. Only A-Z, a-z, 0-9, ., _, and - are allowed.")), + } + } + Ok(()) +} + fn run(mut args: ArgsOs) -> Result<(), String> { let Some(user_data_path) = args.next().map(PathBuf::from) else { ex_usage(); @@ -116,6 +178,7 @@ fn run(mut args: ArgsOs) -> Result<(), String> { if args.next().is_some() { ex_usage(); } + check_app_id(app.as_bytes())?; let user_data = Root::open(&user_data_path) .map_err(|e| format!("opening user data partition: {e}"))? @@ -139,24 +202,42 @@ fn run(mut args: ArgsOs) -> Result<(), String> { let target_installation_dir = Root::from_fd(target_installation_dir).with_resolver_flags(ResolverFlags::NO_SYMLINKS); - let mut app_path = PathBuf::new(); - app_path.push("app"); - app_path.push(&app); - let source_app_dir = open_subdir(&source_installation_dir, &app_path) - .map_err(|e| format!("opening source flatpak app: {e}"))?; + let source_app_dir = { + let app_root_dir = match open_subdir(&source_installation_dir, &Path::new("app")) { + Ok(d) => d, + Err(e) => match e.kind() { + ErrorKind::OsError(Some(libc::ENOENT)) => { + return Err("Can't find the \"app\" directory. This probably isn't a flatpak repository".to_string()) + } + _ => return Err(format!("opening source app directory: {e}")), + } + }; + match open_subdir(&app_root_dir, app.as_ref()) { + Ok(d) => d, + Err(e) => match e.kind() { + ErrorKind::OsError(Some(libc::ENOENT)) => { + // app was santized above, so no need to redact it from logs + // also, this must be valid UTF-8 + let app = str::from_utf8(app.as_bytes()).unwrap(); + return Err(format!("Application {app} isn't installed.")) + } + _ => return Err(format!("opening source per-app directory: {e}")), + } + } + }; let arch_and_branch = source_app_dir .readlink("current") .map_err(|e| format!("reading current app arch and branch: {e}"))?; + check_name(arch_and_branch.as_os_str().as_bytes(), 1, "arch/branch")?; let mut components = arch_and_branch.components(); let arch = components.next().unwrap().as_os_str(); let branch = components.as_path().as_os_str(); if branch.is_empty() { return Err("can't infer branch from \"current\" link".to_string()); } - let (source_commit_dir, commit) = mount_commit( &source_app_dir, - &app_path.join(&arch_and_branch), + &PathBuf::new().join("app").join(&app).join(&arch_and_branch), &target_installation_dir, &arch_and_branch, "app", @@ -179,6 +260,7 @@ fn run(mut args: ArgsOs) -> Result<(), String> { let runtime = extract_runtime(metadata).map_err(|e| format!("reading runtime from metadata: {e}"))?; + check_name(runtime.as_bytes(), 2, "runtime/architecture/version")?; let runtime_path = Path::new("runtime").join(runtime); let (_, runtime_commit) = mount_commit( -- 2.52.0