From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from atuin.qyliss.net (localhost [IPv6:::1]) by atuin.qyliss.net (Postfix) with ESMTP id 5295D1F9EC; Sat, 29 Nov 2025 19:00:39 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id 141291F97C; Sat, 29 Nov 2025 19:00:37 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on atuin.qyliss.net X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DMARC_MISSING,RCVD_IN_DNSWL_LOW,SPF_HELO_PASS autolearn=unavailable autolearn_force=no version=4.0.1 Received: from fhigh-b7-smtp.messagingengine.com (fhigh-b7-smtp.messagingengine.com [202.12.124.158]) by atuin.qyliss.net (Postfix) with ESMTPS id 3AF081F97B for ; Sat, 29 Nov 2025 19:00:35 +0000 (UTC) Received: from phl-compute-02.internal (phl-compute-02.internal [10.202.2.42]) by mailfhigh.stl.internal (Postfix) with ESMTP id CC94C7A016D; Sat, 29 Nov 2025 14:00:30 -0500 (EST) Received: from phl-mailfrontend-02 ([10.202.2.163]) by phl-compute-02.internal (MEProxy); Sat, 29 Nov 2025 14:00:30 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alyssa.is; h=cc :cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm3; t=1764442830; x=1764529230; bh=XbiG1kndxz 71JhvhIqJkhXS3IF71CthC+63YrqDx4m4=; b=BX2AgZzM4t3tjZAbNKGOlQlvMA AoJcbu9BT+Zeg9gCrWHFm3/h9mK+bTGw4BZlPqmSQx5S7Xb7x2xcJUsY5MdNAONv oGVe5Pd49QqttQawl9LdoUwzsxo67KXcT5svB/GG5BZ3Rz1GLIQHoVsyyWsSKZ2J B9/zGGFhE3GxceLsIOFAfb+mMsIqCZDS8YgddlkcCbNlFiXgq1YocWplA4kmAv8c eFXZPXm+Cm3BEDn0g/yeEbSlPMu8fGxY2fnWJPH9za58pMje5/gTeE0ZLJgEwg2h aYgY0Lvpo/dLXKt3MlSUgBxW5hZgAg/dhnu9BwI4/cQ2b5tLbOiJy2CdTEvQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t= 1764442830; x=1764529230; bh=XbiG1kndxz71JhvhIqJkhXS3IF71CthC+63 YrqDx4m4=; b=u4xo79P5XkF3FYrHOeX92sYX9+rMZ6T16zaYF5cLNRDiuM6g240 KTvGZzDAIKBUjglI2Xeek3kV3m/A3F4S99wnSnlWtP69LqUo5ZC1Rgz0Dml9yleb eMsuZwgbjsjq9Ax3OcgkKZ+iF9jcgG56N50tp+Gc0rJq8S0O9JAdBBXQ/L2URs2k 8tratqVM5HS8vW5VZXt0huPpSUPRdAuCx1uhgpdr5aBUy5Jm0rO4sVBe3ERmTKcw l9upSEUa1nu8JEdPLiLc/bddLqVdumrkM1ww+EuPNV0BUc4mAcjkEkOR/eL1cVDl WAfc0F1CX26T3Dvc+6+2aqDDPGyM7FdGiBw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeffedrtdeggddvheefvdegucetufdoteggodetrf dotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu rghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujf gurhephffvvefujghffffkgggtsehgtderredttddtnecuhfhrohhmpeetlhihshhsrgcu tfhoshhsuceohhhisegrlhihshhsrgdrihhsqeenucggtffrrghtthgvrhhnpefhueelgf eufedttdekfeefvdetvdelhfetjeefuefgfefhhfelveduleejgfdtffenucffohhmrghi nhepghhithhhuhgsrdgtohhmpdgrshgpphgrthhhrdgrshenucevlhhushhtvghrufhiii gvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehhihesrghlhihsshgrrdhishdpnhgs pghrtghpthhtohepvddpmhhouggvpehsmhhtphhouhhtpdhrtghpthhtohepuggvmhhioh gsvghnohhurhesghhmrghilhdrtghomhdprhgtphhtthhopeguvghvvghlsehsphgvtght rhhumhdqohhsrdhorhhg X-ME-Proxy: Feedback-ID: i12284293:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sat, 29 Nov 2025 14:00:29 -0500 (EST) Received: by fw12.qyliss.net (Postfix, from userid 1000) id 208D031BCAEE; Sat, 29 Nov 2025 20:00:28 +0100 (CET) From: Alyssa Ross To: Demi Marie Obenour Subject: Re: [PATCH v3 1/3] tools/mount-flatpak: init In-Reply-To: <01179fd8-ca6a-43f8-b61d-447d507dd5a6@gmail.com> References: <20251127202311.42422-2-hi@alyssa.is> <01179fd8-ca6a-43f8-b61d-447d507dd5a6@gmail.com> Date: Sat, 29 Nov 2025 20:00:26 +0100 Message-ID: <874iqcekad.fsf@alyssa.is> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Message-ID-Hash: PVJ4GWKHMVJXDCMI2WMNDMAKYUDOFO5E X-Message-ID-Hash: PVJ4GWKHMVJXDCMI2WMNDMAKYUDOFO5E X-MailFrom: hi@alyssa.is X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-devel.spectrum-os.org-0; header-match-devel.spectrum-os.org-1; header-match-devel.spectrum-os.org-2; header-match-devel.spectrum-os.org-3; header-match-devel.spectrum-os.org-4; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: devel@spectrum-os.org X-Mailman-Version: 3.3.9 Precedence: list List-Id: Patches and low-level development discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --=-=-= Content-Type: text/plain Demi Marie Obenour 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 >> +// 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::() 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 >> + >> +use std::fs::File; >> +use std::io::read_to_string; >> + >> +use crate::keyfile::parse; >> + >> +pub fn extract_runtime(mut metadata: File) -> Result { >> + 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. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQQGoGac7QfI+H5ZtFCZddwkt31pFQUCaStCywAKCRCZddwkt31p FabKAP9eT5f/wZ/VlYC3ThkI+o4bGIDRGCVI3Eigh3xrnOxDQQD+JJBRPlX6mVsF TD8voD4M6yJCKt0dVA3ERx3fHT6p0go= =5AvC -----END PGP SIGNATURE----- --=-=-=--