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

[Demi Marie Obenour <demiobenour@gmail.com>]

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