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

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

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

On 11/29/25 14:00, Alyssa Ross wrote:
> 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.

The advantage is better error messages.  A relative path will result
in a confusing "does not exist" error at best.

Definitely should not block committing this.
>>> +    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.

I 100% agree with this.

> 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

Fair.

>>> +    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.

Still good practice anyway, but not a blocker.

>>> +    )
>>> +    .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.

That is a little bit annoying, but not a blocker.

>>> +    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.)

Thank you!

>>> +    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.

Ah, I forgot that this is a compile-time constant.

>>> +    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.

Good point, but I still think the propagation needs to be set to
MS_SLAVE.  Right now, mount flags are set manually by the user.

>>> +    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.

Technically, the pointers should be cast too, but I'm more concerned
about rustc assuming this means the data the pointers point to won't
be accessed than about something that is guaranteed to work by the ABI.

>>> 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.
It's easy to run the VM's services in a cgroup, but running commands
like this in the cgroup is harder.  Also, this command is much easier
to make robust against problems like that.

Should not block commit though.
-- 
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 --]