Re: [PATCH v4 3/5] tools/mount-flatpak: init

[Alyssa Ross <hi@alyssa.is>]

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

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

> On 11/30/25 23:45, Alyssa Ross wrote:
>> diff --git a/tools/mount-flatpak/src/main.rs b/tools/mount-flatpak/src/main.rs
>> new file mode 100644
>> index 0000000..db246cd
>> --- /dev/null
>> +++ b/tools/mount-flatpak/src/main.rs
>> @@ -0,0 +1,296 @@
>> +// SPDX-FileCopyrightText: 2025 Alyssa Ross <hi@alyssa.is>
>> +// SPDX-License-Identifier: EUPL-1.2+
>> +
>> +//! Flatpak installations look like this:
>> +//!
>> +//! ```text
>> +//! flatpak/
>> +//! ├── app/
>> +//! │   └── org.gnome.TextEditor/
>> +//! │       ├── current -> x86_64/stable
>> +//! │       └── x86_64/
>> +//! │           └── stable/
>> +//! │               ├── 0029140121b39f5b7cf4d44fd46b0708eee67f395b5e1291628612a0358fb909/
>> +//! │               │   └── …
>> +//! │               └── active -> 0029140121b39f5b7cf4d44fd46b0708eee67f395b5e1291628612a0358fb909
>> +//! ├── db/
>> +//! ├── exports/
>> +//! │   └── …
>> +//! ├── repo
>> +//! │   ├── config
>> +//! │   ├── objects
>> +//! │   ├── tmp
>> +//! │   │   └── cache
>> +//! │   │       └── …
>> +//! │   └── …
>> +//! └── runtime
>> +//!     ├── org.gnome.Platform
>> +//!     │   └── x86_64
>> +//!     │       └── 49
>> +//!     │           ├── active -> bf6aa432cb310726f4ac0ec08cc88558619e1d4bd4b964e27e95187ecaad5400
>> +//!     │           └── bf6aa432cb310726f4ac0ec08cc88558619e1d4bd4b964e27e95187ecaad5400
>> +//!     │               └── …
>> +//!     └── …
>> +//! ```
>> +//!
>> +//! The purpose of this program is to use bind mounts to construct a
>> +//! Flatpak installation containing only a single application and
>> +//! runtime, which can be passed through to a VM without exposing
>> +//! other installed applications.
>> +
>> +mod keyfile;
>> +mod metadata;
>> +
>> +use std::borrow::Cow;
>> +use std::env::{ArgsOs, args_os};
>> +use std::ffi::OsStr;
>> +use std::io;
>> +use std::os::unix::prelude::*;
>> +use std::path::{Path, PathBuf};
>> +use std::process::exit;
>> +
>> +use pathrs::Root;
>> +use pathrs::flags::{OpenFlags, ResolverFlags};
>> +use rustix::fs::{CWD, FileType, fstat};
>> +use rustix::mount::{MoveMountFlags, OpenTreeFlags, move_mount, open_tree};
>> +
>> +use metadata::extract_runtime;
>> +
>> +fn ex_usage() -> ! {
>> +    eprintln!("Usage: mount-flatpak userdata installation app");
>> +    exit(1);
>> +}
>> +
>> +fn run(mut args: ArgsOs) -> Result<(), String> {
>> +    let Some(user_data_path) = args.next().map(PathBuf::from) else {
>> +        ex_usage();
>> +    };
>> +    let Some(installation_path) = args.next().map(PathBuf::from) else {
>> +        ex_usage();
>> +    };
>> +    let Some(app) = args.next() else {
>> +        ex_usage();
>> +    };
>> +    if args.next().is_some() {
>> +        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 mut 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);
>> +
>> +    std::fs::create_dir("flatpak")
>> +        .map_err(|e| format!("creating target flatpak installation: {e}"))?;
>> +
>> +    let target_installation_dir = open_tree(
>> +        CWD,
>> +        "flatpak",
>> +        OpenTreeFlags::OPEN_TREE_CLONE
>> +            | OpenTreeFlags::OPEN_TREE_CLOEXEC
>> +            | OpenTreeFlags::AT_RECURSIVE
>> +            | 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 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)
>> +        .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();
>
> Should this return an error instead of panicking?

As far as I know a symlink target cannot contain 0 path components.

>> +    let branch = components.as_path().as_os_str();
>> +    if branch.is_empty() {
>> +        return Err("can't infer branch from \"current\" link".to_string());
>> +    }
>
> Missing check that arch and branch are not empty, "." or "..", that
> they do not exceed NAME_MAX (255) bytes, and do not contain '/'.
> I would also check for NUL in case some weird filesystem image has it.
>
> I will call this "path component validation" below.

We do not especially care if they are empty, exceed NAME_MAX bytes, or
contain ".", "..", "/" or even NUL.  The security model here, as
previously explained[1], is that everything inside the Flatpak
repository is fair game, and that the pathrs::Root for the repository
ensures that nothing outside of it is accessed.  The more redundant
validations we add on top of this, the more difficult the code is to
follow and maintain, and the more we're hardcoding brittle assumptions
about Flatpak's internal repository format.  This also applies to your
further comments about path validation.  I want systemic solutions where
it's not necessary to repeatedly make lots of easily-forgettable checks
to achieve safety.

[1]: https://spectrum-os.org/lists/archives/spectrum-devel/03b28aa9-c2fe-49d1-8f76-83240d85ae20@gmail.com/

>> +    full_runtime_path.pop();
>> +    full_runtime_path.push(&runtime_commit);
>> +    let source_runtime_dir = source_installation_dir
>> +        .resolve(&full_runtime_path)
>> +        .map_err(|e| format!("opening source runtime directory: {e}"))?;
>> +
>> +    let target_app_dir = target_installation_dir
>> +        .mkdir_all(&full_app_path, &PermissionsExt::from_mode(0o700))
>> +        .map_err(|e| format!("creating target app directory: {e}"))?;
>> +    let target_runtime_dir = target_installation_dir
>> +        .mkdir_all(&full_runtime_path, &PermissionsExt::from_mode(0o700))
>> +        .map_err(|e| format!("creating target runtime directory: {e}"))?;
>> +
>> +    let source_app_tree = open_tree(
>> +        &source_app_dir,
>> +        "",
>> +        OpenTreeFlags::AT_EMPTY_PATH
>> +            | OpenTreeFlags::OPEN_TREE_CLONE
>> +            | OpenTreeFlags::OPEN_TREE_CLOEXEC
>> +            | OpenTreeFlags::AT_RECURSIVE,
>> +    )
>> +    .map_err(|e| format!("cloning source app tree: {e}"))?;
>> +    let source_runtime_tree = open_tree(
>> +        &source_runtime_dir,
>> +        "",
>> +        OpenTreeFlags::AT_EMPTY_PATH
>> +            | OpenTreeFlags::OPEN_TREE_CLONE
>> +            | OpenTreeFlags::OPEN_TREE_CLOEXEC
>> +            | OpenTreeFlags::AT_RECURSIVE,
>> +    )
>> +    .map_err(|e| format!("cloning source runtime tree: {e}"))?;
>> +    move_mount(
>> +        source_app_tree,
>> +        "",
>> +        target_app_dir,
>> +        "",
>> +        MoveMountFlags::MOVE_MOUNT_F_EMPTY_PATH | MoveMountFlags::MOVE_MOUNT_T_EMPTY_PATH,
>> +    )
>> +    .map_err(|e| format!("mounting app directory: {e}"))?;
>> +    move_mount(
>> +        source_runtime_tree,
>> +        "",
>> +        target_runtime_dir,
>> +        "",
>> +        MoveMountFlags::MOVE_MOUNT_F_EMPTY_PATH | MoveMountFlags::MOVE_MOUNT_T_EMPTY_PATH,
>> +    )
>> +    .map_err(|e| format!("mounting runtime directory: {e}"))?;
>
> This code is all duplicated between the app and runtime directories.
> I recommend using a common function for both.

I had that in C, but it didn't feel like a natural abstraction to me.  I
can try it in Rust though and see if it's any different.  It would be
nice not to have to repeat the open_tree(2) flags.

>> +    target_installation_dir
>> +        .mkdir_all("repo/objects", &PermissionsExt::from_mode(0o700))
>> +        .map_err(|e| format!("creating repo/objects: {e}"))?;
>> +    target_installation_dir
>> +        .mkdir_all("repo/tmp/cache", &PermissionsExt::from_mode(0o700))
>> +        .map_err(|e| format!("creating repo/tmp/cache: {e}"))?;
>> +    let config_target = target_installation_dir
>> +        .create_file(
>> +            "repo/config",
>> +            OpenFlags::O_WRONLY | OpenFlags::O_CLOEXEC,
>> +            &PermissionsExt::from_mode(0o700),
>> +        )
>> +        .map_err(|e| format!("creating repo/config: {e}"))?;
>> +    let config_source_path = env!("MOUNT_FLATPAK_CONFIG_PATH");
>> +    let config_source = open_tree(
>> +        CWD,
>> +        config_source_path,
>> +        OpenTreeFlags::OPEN_TREE_CLONE | OpenTreeFlags::OPEN_TREE_CLOEXEC,
>> +    )
>> +    .map_err(|e| format!("opening {config_source_path}: {e}"))?;
>> +    move_mount(
>> +        config_source,
>> +        "",
>> +        config_target,
>> +        "",
>> +        MoveMountFlags::MOVE_MOUNT_F_EMPTY_PATH | MoveMountFlags::MOVE_MOUNT_T_EMPTY_PATH,
>> +    )
>> +    .map_err(|e| format!("mounting config: {e}"))?;
>> +
>> +    let mut attr = libc::mount_attr {
>> +        attr_clr: libc::MOUNT_ATTR_NOSYMFOLLOW,
>> +        attr_set: libc::MOUNT_ATTR_RDONLY | libc::MOUNT_ATTR_NODEV,
>> +        propagation: libc::MS_SLAVE,
>> +        userns_fd: 0,
>> +    };
>> +    let empty = b"\0";
>> +    // SAFETY: we pass a valid FD, and a valid mutable pointer with the correct size.
>
> Nit: extra comma.  Also, there are other preconditions, like
> ‘empty’ being a valid C string.

Yes, good point.  Are there others?  I suppose passing the correct
number of arguments with the correct types.

>> +    unsafe {
>> +        let r = libc::syscall(
>> +            libc::SYS_mount_setattr,
>> +            target_installation_dir.as_fd().as_raw_fd() as libc::c_long,
>> +            empty.as_ptr() as *const libc::c_char,
>> +            (libc::AT_EMPTY_PATH | libc::AT_RECURSIVE) as libc::c_long,
>> +            &mut attr as *mut libc::mount_attr,
>> +            size_of::<libc::mount_attr>() as libc::c_long,
>> +        );
>> +        if r == -1 {
>> +            return Err(format!(
>> +                "setting target mount attributes: {}",
>> +                io::Error::last_os_error()
>> +            ));
>> +        }
>> +    }
>> +    move_mount(
>> +        target_installation_dir,
>> +        "",
>> +        CWD,
>> +        "flatpak",
>> +        MoveMountFlags::MOVE_MOUNT_F_EMPTY_PATH,
>> +    )
>> +    .map_err(|e| format!("mounting target installation dir: {e}"))?;
>
> Is this really needed?  It looks redundant.  If it isn’t, I’d
> add a comment explaining why.

Is mounting the detached tree into the filesystem so it's actually
accesible really needed?  Yes.  I'm not sure how I could write a comment
making that clearer that wouldn't just be restating what the code says.
It's a textbook use of the new mount API.

>> +    std::fs::create_dir("params").map_err(|e| format!("creating params directory: {e}"))?;
>> +    std::fs::write("params/id", app.as_bytes()).map_err(|e| format!("writing params/id: {e}"))?;
>> +    std::fs::write("params/commit", commit.as_bytes())
>> +        .map_err(|e| format!("writing params/commit: {e}"))?;
>> +    std::fs::write("params/arch", arch.as_bytes())
>> +        .map_err(|e| format!("writing params/arch: {e}"))?;
>> +    std::fs::write("params/branch", branch.as_bytes())
>> +        .map_err(|e| format!("writing params/branch: {e}"))?;
>> +    std::fs::write("params/runtime-commit", runtime_commit.as_bytes())
>> +        .map_err(|e| format!("writing params/runtime-commit: {e}"))?;
>> +
>> +    Ok(())
>> +}
>> +
>> +fn main() {
>> +    let mut args = args_os();
>> +
>> +    let prog_name = args
>> +        .next()
>> +        .as_ref()
>> +        .map(Path::new)
>> +        .and_then(Path::file_name)
>> +        .map_or(Cow::Borrowed("mount-flatpak"), OsStr::to_string_lossy)
>> +        .into_owned();
>> +
>> +    if let Err(e) = run(args) {
>> +        eprintln!("{prog_name}: {e}");
>> +        exit(1);
>> +    }
>> +}
>> diff --git a/tools/mount-flatpak/src/metadata.rs b/tools/mount-flatpak/src/metadata.rs
>> new file mode 100644
>> index 0000000..dfc05b1
>> --- /dev/null
>> +++ b/tools/mount-flatpak/src/metadata.rs
>> @@ -0,0 +1,19 @@
>> +// SPDX-License-Identifier: EUPL-1.2+
>> +// SPDX-FileCopyrightText: 2025 Alyssa Ross <hi@alyssa.is>
>> +
>> +use std::fs::File;
>> +use std::io::read_to_string;
>> +
>> +use crate::keyfile::parse;
>> +
>> +pub fn extract_runtime(mut metadata: File) -> Result<String, String> {
>> +    let metadata = read_to_string(&mut metadata).map_err(|e| e.to_string())?;
>> +    let group = parse(&metadata).map_err(|e| e.to_string())?;
>> +    let application = group
>> +        .get("Application")
>> +        .ok_or_else(|| "Application group missing".to_string())?;
>> +    Ok(application
>> +        .get("runtime")
>> +        .ok_or_else(|| "runtime property missing".to_string())?
>> +        .clone())
>> +}
>
> I'd still add a limit for the amount of data that will be read.
> Anything passed 128KiB is almost certainly a bug or attack.  Forcing
> OOM conditions is a useful attack primitive, as OOM errors may be
> mishandled in various places.
>
> Adding a cgroup is necessary, but having this code run in it isn't
> trivial, as it isn't a supervised process.

If we can't run non-supervised processes inside the cgroup we can't run
Cloud Hypervisor there, so we might as well not bother.

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