Re: [PATCH v3 1/3] tools/mount-flatpak: init

[Alyssa Ross <hi@alyssa.is>]

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

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

> On 11/27/25 15:23, 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..fd2f74f
>> --- /dev/null
>> +++ b/tools/mount-flatpak/src/main.rs
>> @@ -0,0 +1,252 @@
>> +// SPDX-FileCopyrightText: 2025 Alyssa Ross <hi@alyssa.is>
>> +// SPDX-License-Identifier: EUPL-1.2+
>> +
>> +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 installation app");
>> +    exit(1);
>> +}
>
> Stale usage message.

Will fix.

>> +fn run(mut args: ArgsOs) -> Result<(), String> {
>> +    let Some(user_data_path) = args.next().map(PathBuf::from) else {
>> +        ex_usage();
>> +    };
>
> This should be an absolute path, as the working directory isn't one
> that the user should be caring about.

I don't see any reason to restrict the user like that.

>> +    let Some(installation_path) = args.next().map(PathBuf::from) else {
>> +        ex_usage();
>> +    };
>> +    let Some(app) = args.next() else {
>> +        ex_usage();
>> +    };
>
> I recommend checking that 'app' is not empty, '.' or '..' and does
> not contain '/'.

I think this is a sign we really need to be using the flatpak
installation as a root.  We need to be protecting *systematically*
against escapes from the flatpak directory to elsewhere on the user data
partition, because that's where all the interesting data is.

This comment[1] gives me the impression that it we should be okay
opening a root for the user data partition, resolving the installation
path inside the user partition, and creating a new root from that, to be
used for everything within it.

(We don't need to go any more granular than that, because it never makes
sense for individual subdirectories of the Flatpak repository to be
specifically writable by a VM, I think.  So we assume that whatever
writes the Flatpak repository already has access to the whole
repository, and is not interested in tricking the host to do things to
it that it could have just directly done itself.)

[1]: https://github.com/cyphar/libpathrs/issues/26#issuecomment-586382681

>> +    let target_installation_dir = open_tree(
>> +        CWD,
>> +        "flatpak",
>> +        OpenTreeFlags::OPEN_TREE_CLONE
>> +            | OpenTreeFlags::OPEN_TREE_CLOEXEC
>> +            | OpenTreeFlags::AT_RECURSIVE,
>
> Missing AT_SYMLINK_NOFOLLOW unless it is is implied.

We rely on nothing malicious being able to mess with the target
directory while we're running, and we do not create flatpak as a
symlink.

>> +    )
>> +    .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);
>
> Is NO_XDEV implied?  It's not supported in the API but it would be
> useful to check.

It is not implied, but there's no way to set it either.

>> +    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}"))?;
>
> This is somewhat hard to understand.  I recommend a big comment at
> the start of the file explaining the structure of the source directory, which is:
>
> VM's directory/
>    Installation root/
>       app/
>          App ID/
>             current -> arch/branch
>             arch/
>                branch/
>                   active -> actual commit hash (64 bytes lowercase hex)
>                   commit hash
>                      metadata (contains runtime info)
>       runtime/
>          Runtime ID/
>             arch/
>                branch/
>                   active -> actual commit hash (64 bytes lowercase hex)
>                   commit hash
>
> As well as that of the target directory, which I'm not sure about.

I can do that.

(The target directory is just an installation with a single app and
single runtime installed.)

>> +    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());
>> +    }
>> +
>> +    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 = source_installation_dir
>> +        .resolve(&full_app_path)
>> +        .map_err(|e| format!("opening source app directory: {e}"))?;
>> +
>> +    let metadata = source_installation_dir
>> +        .resolve(full_app_path.join("metadata"))
>> +        .map_err(|e| format!("resolving app metadata: {e}"))?;
>> +
>> +    let metadata_stat =
>> +        fstat(&metadata).map_err(|e| format!("checking app metadata is a regular file: {e}"))?;
>> +    let metadata_type = FileType::from_raw_mode(metadata_stat.st_mode);
>> +    if !metadata_type.is_file() {
>> +        let e = format!("type of app metadata is {metadata_type:?}, not RegularFile");
>> +        return Err(e);
>> +    }
>> +    let metadata = metadata
>> +        .reopen(OpenFlags::O_RDONLY)
>> +        .map_err(|e| format!("opening app metadata: {e}"))?;
>> +
>> +    let runtime =
>> +        extract_runtime(metadata).map_err(|e| format!("reading runtime from metadata: {e}"))?;
>> +
>> +    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 = 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}"))?;
>> +
>> +    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");
>
> This should always be an absolute path, and it might be good to assert that.

str::contains is not const, so it's not possible to do this in the
obvious way, but it's also not critical to validate a compile-time
constant we know we're always setting correctly.  Would consider a
patch.

>> +    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: 0,
>> +        userns_fd: 0,
>> +    };
>
> Propagation should likely be set to MS_SLAVE.  (Yeah, I know,
> terrible name.)  attr_set should also include MOUNT_ATTR_NOEXEC as
> well as the host has no business executing anything there.

The host has no business executing anything at all from the whole user
data partition, so this is not the place to set that.  There's no reason
this mount _in particular_ needs the flag set that doesn't apply to the
whole partition this mount's flags are inherited from.

>> +    let empty = b"\0";
>> +    // SAFETY: we pass a valid FD, and a valid mutable pointer with the correct size.
>> +    unsafe {
>> +        let r = libc::syscall(
>> +            libc::SYS_mount_setattr,
>> +            target_installation_dir.as_fd(),
>> +            empty.as_ptr() as *const libc::c_char,
>> +            (libc::AT_EMPTY_PATH | libc::AT_RECURSIVE) as libc::c_uint,
>> +            &mut attr as *mut libc::mount_attr,
>> +            size_of::<libc::mount_attr>() as libc::size_t,
>> +        );
>
> All of the non-pointer arguments need to be cast to libc::c_long or
> (equivalently and possibly better) core::ffi::c_long.

I wish this was better documented, but sounds right.

>> 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())?;
>
> Missing limit on the size of the file to prevent unbounded memory allocation.

There are lots of ways to cause unbounded host memory allocation.
Modifying everything to limit size is not feasible.  The only way to
prevent this is to run this stuff in a VM-scoped cgroup.

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