[PATCH 0/4] Improve mount-flatpak

[PATCH 0/4] Improve mount-flatpak

[Demi Marie Obenour]

Patches 1 through 3 are all cleanups.  Patch 4 adds a bunch of
additional checks to improve error messages.  It is also less clean code
than the others.  However, it is optional.

Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
---
Demi Marie Obenour (4):
      tools: Use with_resolver_flags instead of mutation
      tools/mount-flatpak: Create utility function to open a subdirectory
      tools/mount-flatpak: Move more code to mount_commit()
      tools/mount-flatpak: Reasonableness checks on application IDs and paths

 tools/mount-flatpak/src/main.rs | 190 ++++++++++++++++++++++++++++------------
 1 file changed, 134 insertions(+), 56 deletions(-)
---
base-commit: 92e219e7c08c479d216a46d2736ea9d229ff034d
change-id: 20251202-better-mount-flatpak-cdf8db2ce7f4

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

[PATCH 1/4] tools: Use with_resolver_flags instead of mutation

[Demi Marie Obenour]

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

Re: [PATCH 1/4] tools: Use with_resolver_flags instead of mutation

[Alyssa Ross]

This patch has been committed as 8a6e4f03951d80382e5dcf1a159d37dec8376a0b,
which can be viewed online at
https://spectrum-os.org/git/spectrum/commit/?id=8a6e4f03951d80382e5dcf1a159d37dec8376a0b.

This is an automated message.  Send comments/questions/requests to:
Alyssa Ross <hi@alyssa.is>

[PATCH 2/4] tools/mount-flatpak: Create utility function to open a subdirectory

[Demi Marie Obenour]

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

Re: [PATCH 2/4] tools/mount-flatpak: Create utility function to open a subdirectory

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> writes:

> 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(-)

Why do we care?  It'll fail either way, right?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 2/4] tools/mount-flatpak: Create utility function to open a subdirectory

[Demi Marie Obenour]

[-- Attachment #1.1.1: Type: text/plain, Size: 576 bytes --]

On 12/22/25 09:31, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> writes:
> 
>> 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(-)
> 
> Why do we care?  It'll fail either way, right?

Better to fail sooner, especially in the case of a possible attack.
It might not help, but it can't hurt either.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/4] tools/mount-flatpak: Move more code to mount_commit()

[Demi Marie Obenour]

Make mount_commit() responsible for not only mounting the commit tree,
but also for opening the source subdirectory and creating the
destination subdirectory.

Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
---
 tools/mount-flatpak/src/main.rs | 82 ++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 43 deletions(-)

diff --git a/tools/mount-flatpak/src/main.rs b/tools/mount-flatpak/src/main.rs
index 68a1f6ea308387a9471682c927d3a71439ab69e4..b40402b1b15729bdd2228025850efe8d87a48e3b 100644
--- a/tools/mount-flatpak/src/main.rs
+++ b/tools/mount-flatpak/src/main.rs
@@ -43,7 +43,7 @@ mod metadata;
 
 use std::borrow::Cow;
 use std::env::{ArgsOs, args_os};
-use std::ffi::OsStr;
+use std::ffi::{OsStr, OsString};
 use std::io;
 use std::os::unix::prelude::*;
 use std::path::{Path, PathBuf};
@@ -67,12 +67,21 @@ fn open_subdir(root: &Root, path: &Path) -> Result<Root, Error> {
 }
 
 fn mount_commit(
-    source_commit: &dyn AsFd,
+    source_installation: &Root,
+    source_path: &Path,
     target_installation: &Root,
-    path: &Path,
-) -> Result<(), String> {
+    target_path: &Path,
+    kind: &str,
+) -> Result<(Root, OsString), String> {
+    let source_commit_parent_dir = open_subdir(source_installation, source_path)
+        .map_err(|e| format!("opening source {kind} commit parent: {e}"))?;
+    let commit = source_commit_parent_dir
+        .readlink("active")
+        .map_err(|e| format!("reading active {kind} commit: {e}"))?;
+    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(
-        source_commit,
+        &source_root,
         "",
         OpenTreeFlags::AT_EMPTY_PATH
             | OpenTreeFlags::OPEN_TREE_CLONE
@@ -81,7 +90,7 @@ fn mount_commit(
     )
     .map_err(|e| format!("cloning source commit tree: {e}"))?;
     let target_commit_dir = target_installation
-        .mkdir_all(path, &PermissionsExt::from_mode(0o700))
+        .mkdir_all(target_path, &PermissionsExt::from_mode(0o700))
         .map_err(|e| format!("creating target commit directory: {e}"))?;
     move_mount(
         source_commit_tree,
@@ -90,7 +99,8 @@ fn mount_commit(
         "",
         MoveMountFlags::MOVE_MOUNT_F_EMPTY_PATH | MoveMountFlags::MOVE_MOUNT_T_EMPTY_PATH,
     )
-    .map_err(|e| format!("mounting commit: {e}"))
+    .map_err(|e| format!("mounting commit: {e}"))?;
+    Ok((source_root, commit.as_os_str().to_owned()))
 }
 
 fn run(mut args: ArgsOs) -> Result<(), String> {
@@ -129,11 +139,13 @@ 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 full_app_path = installation_path.join("app");
-    full_app_path.push(&app);
-    full_app_path.push("current");
-    let arch_and_branch = source_installation_dir
-        .readlink(&full_app_path)
+    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 arch_and_branch = source_app_dir
+        .readlink("current")
         .map_err(|e| format!("reading current app arch and branch: {e}"))?;
     let mut components = arch_and_branch.components();
     let arch = components.next().unwrap().as_os_str();
@@ -142,21 +154,16 @@ fn run(mut args: ArgsOs) -> Result<(), String> {
         return Err("can't infer branch from \"current\" link".to_string());
     }
 
-    full_app_path.pop();
-    full_app_path.push(&arch_and_branch);
-    full_app_path.push("active");
-    let commit = source_installation_dir
-        .readlink(&full_app_path)
-        .map_err(|e| format!("reading active app commit: {e}"))?
-        .into_os_string();
-
-    full_app_path.pop();
-    full_app_path.push(&commit);
-    let source_app_dir = open_subdir(&source_installation_dir, &full_app_path)
-        .map_err(|e| format!("opening source app directory: {e}"))?;
+    let (source_commit_dir, commit) = mount_commit(
+        &source_app_dir,
+        &app_path.join(&arch_and_branch),
+        &target_installation_dir,
+        &arch_and_branch,
+        "app",
+    )?;
 
-    let metadata = source_installation_dir
-        .resolve(&full_app_path.join("metadata"))
+    let metadata = source_commit_dir
+        .resolve("metadata")
         .map_err(|e| format!("resolving app metadata: {e}"))?;
 
     let metadata_stat =
@@ -172,25 +179,14 @@ fn run(mut args: ArgsOs) -> Result<(), String> {
 
     let runtime =
         extract_runtime(metadata).map_err(|e| format!("reading runtime from metadata: {e}"))?;
+    let runtime_path = Path::new("runtime").join(runtime);
 
-    let mut full_runtime_path = installation_path.join("runtime");
-    full_runtime_path.push(runtime);
-    full_runtime_path.push("active");
-    let runtime_commit = source_installation_dir
-        .readlink(&full_runtime_path)
-        .map_err(|e| format!("reading active runtime commit: {e}"))?
-        .into_os_string();
-
-    full_runtime_path.pop();
-    full_runtime_path.push(&runtime_commit);
-    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)?;
-    mount_commit(
-        &source_runtime_dir,
+    let (_, runtime_commit) = mount_commit(
+        &source_installation_dir,
+        &runtime_path,
         &target_installation_dir,
-        &full_runtime_path,
+        &runtime_path,
+        "runtime",
     )?;
 
     target_installation_dir

-- 
2.52.0

[PATCH 4/4] tools/mount-flatpak: Reasonableness checks on application IDs and paths

[Demi Marie Obenour]

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