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 0AE5A1D872; Fri, 05 Dec 2025 16:23:15 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id 8F11B1D7DB; Fri, 05 Dec 2025 16:23:07 +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.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DMARC_PASS,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=4.0.1 Received: from mail-yw1-x1134.google.com (mail-yw1-x1134.google.com [IPv6:2607:f8b0:4864:20::1134]) by atuin.qyliss.net (Postfix) with ESMTPS id 2F0AB1D7B6 for ; Fri, 05 Dec 2025 16:23:03 +0000 (UTC) Received: by mail-yw1-x1134.google.com with SMTP id 00721157ae682-78ab039ddb4so23335557b3.3 for ; Fri, 05 Dec 2025 08:23:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1764951781; x=1765556581; darn=spectrum-os.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=kRoxmfqyCT5+SlFcW5IZ37zH6XMC+dYDb7HQa2Zd5OY=; b=Bk0a13oStdhQMl8pVO7XM7nQKquIjQUh96yvdWK8UXpuZKoqJrE2R9fF8h+v1TihG1 hdYHfqGamJ3ow74aakjAqdVGY5FGKRba/1sHRLETnwobk8+v6EFDdy2Aom1Os2BfRevE MOnYq4ZVaDOy8RNY39gOzxdKuG905vvb2b3cuzSmC31YeFrzycbp/IiDaP995QqHhtgD 24QyeNDgDAichJ/z56D7OK6+ZrOodoS0IqulwHLNG3qXaPzO9Gz1gcq1vTDQ8ARHvD1N 2wfi7FWMkOxIYRvoKSO0//P6uarNTO+m37OmfZAJMDOB28Ee6mMvPCLDgRi8elFStoUL ZAww== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764951781; x=1765556581; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-gg:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=kRoxmfqyCT5+SlFcW5IZ37zH6XMC+dYDb7HQa2Zd5OY=; b=VCAfLgp6vQrCSG8SK7pD3YGQCH9rVeIO8g06bSDIoAp6jB+hUNBDf4iAE9Y2a0crgx HcOadQG6CgkTIgGyD6nM1OFkaUB8Wg5xRU7Tc3cdJFLJdKO5J2xcM9DHBRbRlYCMOxw9 Y8HxBFsvJfXNboVG58XqTFRO6mwBJzy6uXOHnfQbtyjaJvseVyDIz2n2PMaoPx0zWqjD uBOeUfgjH57DGcuep4OI61+Zu73gENiU88JRxCquFtwWkHF0qYX9OJvaDh4cfq3BTsAj xD+LnwHgXPY7sU4AqOqpwmSyOJBPcWUKdzs4oq58V0hAjJk08teKVgLrshOk6iQi+b+J vsdA== X-Gm-Message-State: AOJu0YxWVzh9YGvaS8kQkkl4T+jvTNQ9r07qBwhOvQ6zqw0I1klkrdgG IBzmJEgN3t2CI1e6A/bSl/DYrsbGI24d6F7LLZ0f1FYHaeHoQsDKJiLhJvkGng== X-Gm-Gg: ASbGncuF920aooFtV3aiEmIKEVCU6TAriewB2r2bR2lzeNUapqJE90PdSTd5JbVKnsX 9ckuNpJJ+dx4MypyG41HSQ05ex4NK2DSQUHuJhyHiIg7h61I0yGXskLvIT7bKDAU+EpQjEmaByo R59C8gve8RonQwlKcm+RFFt2qlZb50dun6kBiCCiM+9mjP7WL0B+NhP4byftrfin4en49eKRNFF i23TyL3MO1SGozyvVsYeUPKs/NXUbrCE+oS0RgF4AitFttrRShIL76dipNNR1o+2QJkqZML+ovZ m+Z8m/7qc2uNpWD+Pfkq/MGJBqznvbvMnR0nT0Z/s/yZP7keZjvYhdhqqd4jxGj8NTTXTrkdxLM Lp5zVzqyrtM/VKz42FKqSQkssG3mbJVd8GtJVtclVjWwoecWGSXwUvVMLTxPiHIdKNEyElaQYdn nPRo25zInxtt2f7fEZtORW4v3qw5IbDFnmL6FF3FvpG/RPHuJpfq3KpPEdFp+qK5I5lnV87l6/f P+NXEqle4LEjMV3ucYZlziQUhAr31+xYyM= X-Google-Smtp-Source: AGHT+IETRW3u3CADrXBRQrYRTopY+uQODp5s24MobsOUpUFuUFN+yF6nI2Y4glI9kaHcqtyIxahiuQ== X-Received: by 2002:a05:690c:608a:b0:78c:2f46:1e1c with SMTP id 00721157ae682-78c2f4620b4mr9216167b3.28.1764951780393; Fri, 05 Dec 2025 08:23:00 -0800 (PST) Received: from localhost.localdomain (h96-60-249-169.cncrtn.broadband.dynamic.tds.net. [96.60.249.169]) by smtp.gmail.com with UTF8SMTPSA id 00721157ae682-78c1b7795a7sm18153477b3.34.2025.12.05.08.22.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 05 Dec 2025 08:22:59 -0800 (PST) From: Demi Marie Obenour Date: Fri, 05 Dec 2025 11:22:04 -0500 Subject: [PATCH 4/4] tools/mount-flatpak: Reasonableness checks on application IDs and paths MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20251205-better-mount-flatpak-v1-4-229a81366091@gmail.com> References: <20251205-better-mount-flatpak-v1-0-229a81366091@gmail.com> In-Reply-To: <20251205-better-mount-flatpak-v1-0-229a81366091@gmail.com> To: Spectrum OS Development X-Mailer: b4 0.14.3 X-Developer-Signature: v=1; a=ed25519-sha256; t=1764951720; l=8464; i=demiobenour@gmail.com; s=20250729; h=from:subject:message-id; bh=wi3bkJGs8VyPc1nDMAW8BpKhXtpKYSWNXoDO3hqhkvQ=; b=hiv0V7wBjwinTxSH7x9GFj9ugNM3cLr9blBEp3dRs1b6fp9qzYNoFYJGk7in7aZpR60K1drYJ fFthHtrefGXAl6BU73cY7eeyz49JIOAH1Ou3OU7L/fwNwepWe7PteI+ X-Developer-Key: i=demiobenour@gmail.com; a=ed25519; pk=X57Q4/YQDj9t4SBeKaDwvXYKB6quZJVx/DE2Ly2out0= Message-ID-Hash: WLBLCOCC7TMICYA6DW44YRUMJRAQCAPH X-Message-ID-Hash: WLBLCOCC7TMICYA6DW44YRUMJRAQCAPH X-MailFrom: demiobenour@gmail.com 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: Demi Marie Obenour , Alyssa Ross 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: This is not a security problem, because libpathrs prevents directory traversal far better than mere string validation ever could. Instead, it serves to prevent excruciatingly unhelpful error messages. In particular, the application ID is user input and humans, being humans, will likely get it wrong at some point. Catching the error early allows the problem to be clearly explained, rather than continuing with nonsensical input and producing a nonsensical result. Malformed application IDs are rejected with an error message explaining what the problem is. Attempting to run an application from something that obviously isn't a flatpak repository returns an error explaining that this is the problem. Attempting to run an application that isn't installed tells the user exactly that, rather than a "no such file or directory" error. Problems that indicate a corrupt Flatpak repository are also detected. The restrictions on application IDs are future-proof because application IDs must be valid D-Bus well-known names, and the rules for what is a valid D-Bus well-known name are set in stone. Signed-off-by: Demi Marie Obenour --- tools/mount-flatpak/src/main.rs | 98 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 90 insertions(+), 8 deletions(-) diff --git a/tools/mount-flatpak/src/main.rs b/tools/mount-flatpak/src/main.rs index b40402b1b15729bdd2228025850efe8d87a48e3b..c22c3febf1ad8a574deee8d7371710c0de647ca1 100644 --- a/tools/mount-flatpak/src/main.rs +++ b/tools/mount-flatpak/src/main.rs @@ -50,7 +50,7 @@ use std::path::{Path, PathBuf}; use std::process::exit; use pathrs::flags::{OpenFlags, ResolverFlags}; -use pathrs::{Root, error::Error}; +use pathrs::{Root, error::Error, error::ErrorKind}; use rustix::fs::{CWD, FileType, fstat}; use rustix::mount::{MoveMountFlags, OpenTreeFlags, move_mount, open_tree}; @@ -66,6 +66,40 @@ fn open_subdir(root: &Root, path: &Path) -> Result { .map(|f| Root::from_fd(f).with_resolver_flags(ResolverFlags::NO_SYMLINKS)) } +fn check_name(slice: &[u8], expected_slashes: usize, kind: &str) -> Result<(), String> { + // Application ID is provided by a human. Give useful error messages if it is wrong. + let mut components = 0usize; + for component in slice.split(|&s| s == b'/') { + components += 1; + match component { + // Leading, trailing, and repeated '/' introduce empty slices + // into the result, which are caught here. + &[] | &[b'.'] | &[b'.', b'.'] => { + return Err(format!("Path component {components} is {component:?}. Your Flatpak repository is corrupt.")); + } + _ => {} + } + if component.len() > 255 { + return Err(format!("{kind} has component with length over 255 bytes. Your Flatpak repository is corrupt")); + } + for c in component { + match c { + // This can happen for bad metadata files. + b'\0' => return Err(format!("{kind} contains a NUL byte. Your Flatpak metadata is bad.")), + _ => {} + } + } + } + components -= 1; + if expected_slashes != components { + return Err(format!( + "{kind} should have {expected_slashes} '/' characters, \ + but it has {components}. Your Flatpak repository is corrupt." + )); + } + Ok(()) +} + fn mount_commit( source_installation: &Root, source_path: &Path, @@ -78,6 +112,7 @@ fn mount_commit( let commit = source_commit_parent_dir .readlink("active") .map_err(|e| format!("reading active {kind} commit: {e}"))?; + check_name(commit.as_os_str().as_bytes(), 0, "commit")?; let source_root = open_subdir(&source_commit_parent_dir, &commit) .map_err(|e| format!("opening source {kind} commit: {e}"))?; let source_commit_tree = open_tree( @@ -103,6 +138,33 @@ fn mount_commit( Ok((source_root, commit.as_os_str().to_owned())) } +// Check that the application ID is reasonable. Problems with it +// are almost always human error, and it is possible to provide +// vastly better error messages this way. Furthermore, checking the +// application ID here means that if it is okay, it can be printed in +// logs without further sanitization. +fn check_app_id(app: &[u8]) -> Result<(), String> { + let app_len = app.len(); + if app_len > 255 { + // Avoid confusing ENAMETOOLONG error + return Err(format!("Application ID is {app_len} bytes, but the limit is 255")); + } + match app.get(0) { + None => return Err("Application ID can't be empty".to_string()), + Some(a) => match a { + b'.' | b'-' | b'/' => return Err(format!("Application ID can't start with '{a}'")), + _ => {} + }, + }; + for c in app { + match c { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'.' | b'-' | b'_' => {} + _ => return Err(format!("Application ID has forbidden characters. Only A-Z, a-z, 0-9, ., _, and - are allowed.")), + } + } + Ok(()) +} + fn run(mut args: ArgsOs) -> Result<(), String> { let Some(user_data_path) = args.next().map(PathBuf::from) else { ex_usage(); @@ -116,6 +178,7 @@ fn run(mut args: ArgsOs) -> Result<(), String> { if args.next().is_some() { ex_usage(); } + check_app_id(app.as_bytes())?; let user_data = Root::open(&user_data_path) .map_err(|e| format!("opening user data partition: {e}"))? @@ -139,24 +202,42 @@ fn run(mut args: ArgsOs) -> Result<(), String> { let target_installation_dir = Root::from_fd(target_installation_dir).with_resolver_flags(ResolverFlags::NO_SYMLINKS); - let mut app_path = PathBuf::new(); - app_path.push("app"); - app_path.push(&app); - let source_app_dir = open_subdir(&source_installation_dir, &app_path) - .map_err(|e| format!("opening source flatpak app: {e}"))?; + let source_app_dir = { + let app_root_dir = match open_subdir(&source_installation_dir, &Path::new("app")) { + Ok(d) => d, + Err(e) => match e.kind() { + ErrorKind::OsError(Some(libc::ENOENT)) => { + return Err("Can't find the \"app\" directory. This probably isn't a flatpak repository".to_string()) + } + _ => return Err(format!("opening source app directory: {e}")), + } + }; + match open_subdir(&app_root_dir, app.as_ref()) { + Ok(d) => d, + Err(e) => match e.kind() { + ErrorKind::OsError(Some(libc::ENOENT)) => { + // app was santized above, so no need to redact it from logs + // also, this must be valid UTF-8 + let app = str::from_utf8(app.as_bytes()).unwrap(); + return Err(format!("Application {app} isn't installed.")) + } + _ => return Err(format!("opening source per-app directory: {e}")), + } + } + }; let arch_and_branch = source_app_dir .readlink("current") .map_err(|e| format!("reading current app arch and branch: {e}"))?; + check_name(arch_and_branch.as_os_str().as_bytes(), 1, "arch/branch")?; 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()); } - let (source_commit_dir, commit) = mount_commit( &source_app_dir, - &app_path.join(&arch_and_branch), + &PathBuf::new().join("app").join(&app).join(&arch_and_branch), &target_installation_dir, &arch_and_branch, "app", @@ -179,6 +260,7 @@ fn run(mut args: ArgsOs) -> Result<(), String> { let runtime = extract_runtime(metadata).map_err(|e| format!("reading runtime from metadata: {e}"))?; + check_name(runtime.as_bytes(), 2, "runtime/architecture/version")?; let runtime_path = Path::new("runtime").join(runtime); let (_, runtime_commit) = mount_commit( -- 2.52.0