Demi Marie Obenour <demiobenour@gmail.com> writes:
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:
+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.
From what I can tell mount-flatpak works fine with relative paths — are you talking about the cd in run-flatpak? I don't think it makes sense for one program to reject valid inputs that might be invalid to a higher layer that runs it, but we could consider moving the cd into mount-flatpak by adding yet another argument for the destination directory.
+ 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.
Will add.
+ 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.
Will do.
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.
I don't see why it needs to be. The cgroup will have to be created by run-flatpak, so why wouldn't it be able to run mount-flatpak in that cgroup?