From 1c7055f0cf0b3f183e65373112f23425c14871ae Mon Sep 17 00:00:00 2001 From: hu13-test Date: Tue, 27 Oct 2026 15:33:24 +0000 Subject: [PATCH] refuse forge frontmatter paths that escape out/ (#60) Issue/PR id and release version become path segments (issues/.html, releases/.html, releases//), so a '../../tmp/x' value wrote outside the build output. Validate ids ([A-Za-z0-9_-]) and versions (single safe segment) at load time, guard write_page() with a lexical relative-path check, and refuse unsafe release versions in render too. Refused entries warn and are skipped; the build still succeeds. --- src/forge.rs | 13 +++++ src/main.rs | 1 + src/paths.rs | 74 ++++++++++++++++++++++++ src/releases.rs | 43 ++++++++------ tests/path_safety.rs | 134 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 248 insertions(+), 17 deletions(-) create mode 100644 src/paths.rs create mode 100644 tests/path_safety.rs diff --git a/src/forge.rs b/src/forge.rs index 6044b16..dda2cf3 100644 --- a/src/forge.rs +++ b/src/forge.rs @@ -579,6 +579,10 @@ fn load_issues(root: &Path) -> Vec { let Some(fm): Option = parse_frontmatter(yaml, &label) else { continue; }; + if !crate::paths::is_safe_id(&fm.id) { + eprintln!("hu13: forge: skipping {label}: unsafe id {:?}", fm.id); + continue; + } let comments = load_comments( &dir.join(&uuid).join("comments"), &format!("issues/{uuid}/comments"), @@ -620,6 +624,10 @@ fn load_prs(root: &Path) -> Vec { let Some(fm): Option = parse_frontmatter(yaml, &label) else { continue; }; + if !crate::paths::is_safe_id(&fm.id) { + eprintln!("hu13: forge: skipping {label}: unsafe id {:?}", fm.id); + continue; + } let patch = std::fs::read(dir.join(&uuid).join("patch.diff")) .ok() .map(|bytes| String::from_utf8_lossy(&bytes).into_owned()); @@ -1278,6 +1286,11 @@ fn render_ci_index( } fn write_page(out: &Path, rel: &str, html: &str) -> Result<(), String> { + if !crate::paths::is_safe_relative(rel) { + return Err(format!( + "hu13: refusing to write unsafe output path `{rel}`" + )); + } let dest = out.join(rel); if let Some(parent) = dest.parent() { std::fs::create_dir_all(parent).map_err(|e| format!("hu13: create {rel} parent: {e}"))?; diff --git a/src/main.rs b/src/main.rs index 148c678..d2ee742 100644 --- a/src/main.rs +++ b/src/main.rs @@ -8,6 +8,7 @@ mod gitx; mod inbox; mod layout; mod logo; +mod paths; mod releases; mod serve; mod wiki; diff --git a/src/paths.rs b/src/paths.rs new file mode 100644 index 0000000..437caaa --- /dev/null +++ b/src/paths.rs @@ -0,0 +1,74 @@ +//! Output-path safety (f0rge #60). +//! +//! Every path that becomes a file under the build output dir must be +//! checked here first: `.forge/` frontmatter (`id`, release `version`) is +//! untrusted input, and `out.join("issues/{id}.html")` with +//! `id = "../../tmp/x"` writes outside `out/`. +//! +//! Rules: no absolute paths, no `\` or NUL, no empty/`.`/`..` segments. +//! The checks are lexical on purpose — they run before anything is +//! created, so there is nothing to canonicalize yet. + +/// `true` when `segment` is a single safe path component. +#[must_use] +pub fn is_safe_segment(segment: &str) -> bool { + !segment.is_empty() && segment != "." && segment != ".." && !segment.contains(['/', '\\', '\0']) +} + +/// `true` when `rel` is a relative path made only of safe segments. +#[must_use] +pub fn is_safe_relative(rel: &str) -> bool { + !rel.is_empty() + && !rel.starts_with('/') + && !rel.contains(['\\', '\0']) + && rel.split('/').all(is_safe_segment) +} + +/// `true` when `id` is safe both as a path segment and as a displayed +/// forge id: ASCII alphanumerics, `-`, `_` only. +#[must_use] +pub fn is_safe_id(id: &str) -> bool { + is_safe_segment(id) + && id + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_') +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn rejects_traversal_and_absolute() { + for bad in [ + "..", + "../x", + "a/../../b", + "/etc/passwd", + "a\\b", + "a\0b", + "", + "a//b", + "./a", + "a/", + ] { + assert!(!is_safe_relative(bad), "{bad:?} must be rejected"); + } + } + + #[test] + fn accepts_normal_relative_paths() { + for ok in ["a", "a/b", "a-b_c.html", "issues/1.html"] { + assert!(is_safe_relative(ok), "{ok:?} must be accepted"); + } + } + + #[test] + fn ids_are_strict() { + assert!(is_safe_id("06b5602f-4327-4377-99f6-3dca67b5bd75")); + assert!(is_safe_id("PWNED2")); + for bad in ["../../x", "a/b", "a b", "a.b", "..", "", "a\\b"] { + assert!(!is_safe_id(bad), "{bad:?} must be rejected"); + } + } +} diff --git a/src/releases.rs b/src/releases.rs index 03ca8e5..12fbf5a 100644 --- a/src/releases.rs +++ b/src/releases.rs @@ -261,20 +261,23 @@ pub fn load(repo: &Path) -> Vec { continue; } }; + // `version` becomes a path segment (`releases/.html`, + // `releases//`): untrusted frontmatter, so reject traversal + // loudly instead of writing outside the output dir (f0rge #60). + if !crate::paths::is_safe_segment(&fm.version) { + eprintln!( + "hu13: releases: skipping {label}: unsafe version {:?}", + fm.version + ); + continue; + } // Artifacts: present only when `out//` exists at build. // Absent -> notes-only, no warning. - let artifacts = if fm.version.contains('/') - || fm.version.contains('\\') - || fm.version.split('/').any(|p| p == "..") - { - Vec::new() + let out_dir = root.join("out").join(&fm.version); + let artifacts = if out_dir.is_dir() { + collect_artifacts(&out_dir) } else { - let out_dir = root.join("out").join(&fm.version); - if out_dir.is_dir() { - collect_artifacts(&out_dir) - } else { - Vec::new() - } + Vec::new() }; out.push(Release { version: fm.version, @@ -424,13 +427,19 @@ pub fn render_loaded( std::fs::write(&dest, index).map_err(|e| format!("hu13: write releases index: {e}"))?; for rel in releases { + // `load` already rejected unsafe versions; keep the guard here + // too so a future caller cannot bypass it (defense in depth). + if !crate::paths::is_safe_segment(&rel.version) { + return Err(format!( + "hu13: refusing to write release with unsafe version `{}`", + rel.version + )); + } // Copy the whole out dir when present (no size cap). - if !rel.version.contains('/') && !rel.version.contains('\\') { - let src_dir = root.join("out").join(&rel.version); - if src_dir.is_dir() { - let dest_dir = out.join("releases").join(&rel.version); - copy_tree(&src_dir, &dest_dir)?; - } + let src_dir = root.join("out").join(&rel.version); + if src_dir.is_dir() { + let dest_dir = out.join("releases").join(&rel.version); + copy_tree(&src_dir, &dest_dir)?; } let html = render_release_page(rel, site_name, nav, branch, short_sha, description); let page_dest = out.join("releases").join(format!("{}.html", rel.version)); diff --git a/tests/path_safety.rs b/tests/path_safety.rs new file mode 100644 index 0000000..d56023f --- /dev/null +++ b/tests/path_safety.rs @@ -0,0 +1,134 @@ +//! f0rge #60: `.forge` frontmatter is untrusted and must never escape the +//! output dir. Issue/PR `id` and release `version` become path segments +//! (`issues/.html`, `releases/.html`, `releases//`), +//! so a `../../..` value used to write outside `-o`. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +fn git(repo: &Path, args: &[&str]) { + let st = Command::new("git") + .arg("-C") + .arg(repo) + .args(args) + .status() + .expect("spawn git"); + assert!(st.success(), "git {args:?} failed"); +} + +fn fresh_dir(prefix: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "hu13-paths-{prefix}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0) + )); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +fn write(repo: &Path, rel: &str, content: &str) { + let dest = repo.join(rel); + std::fs::create_dir_all(dest.parent().expect("parent")).unwrap(); + std::fs::write(&dest, content).unwrap(); +} + +#[test] +fn hostile_frontmatter_cannot_write_outside_out() { + let repo = fresh_dir("repo"); + let out = fresh_dir("out"); + let marker = format!( + "hu13-pwn-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0) + ); + let escaped = |name: &str| PathBuf::from(format!("/tmp/{marker}-{name}.html")); + + git(&repo, &["init", "-b", "main"]); + git(&repo, &["config", "user.name", "hu13-test"]); + git(&repo, &["config", "user.email", "hu13@test.local"]); + git(&repo, &["config", "commit.gpgsign", "false"]); + write(&repo, "README.md", "# path safety\n"); + + // Issue whose id escapes 6 levels (out lives in /tmp, so /../../.. > /). + write( + &repo, + ".forge/issues/aaaaaaaa-1111-4111-8111-111111111111/issue.md", + &format!( + "---\nid: ../../../../../../tmp/{marker}-issue\ntitle: pwn\n\ + state: open\ncreated: 2026-09-10T03:00:00Z\n---\nbody\n" + ), + ); + // PR whose id escapes. + write( + &repo, + ".forge/prs/bbbbbbbb-2222-4222-8222-222222222222/pr.md", + &format!( + "---\nid: ../../../../../../tmp/{marker}-pr\ntitle: pwn\n\ + state: open\nbase: main\nhead: x\ncreated: 2026-09-10T03:00:00Z\n---\nd\n" + ), + ); + // Release whose version is absolute / traversal. + write( + &repo, + ".forge/releases/r1.md", + &format!( + "---\nversion: /tmp/{marker}-release\ntitle: r1\n\ + created: 2026-09-10T03:00:00Z\n---\nnotes\n" + ), + ); + write( + &repo, + ".forge/releases/r2.md", + "---\nversion: ../../../../../../tmp/{marker}-dotdot\ntitle: r2\n\ + created: 2026-09-10T03:00:00Z\n---\nnotes\n", + ); + // A valid release still has to render. + write( + &repo, + ".forge/releases/r3.md", + "---\nversion: v1.0.0\ntitle: good\ncreated: 2026-09-10T04:00:00Z\n---\nnotes\n", + ); + + git(&repo, &["add", "-A"]); + git(&repo, &["commit", "-m", "hostile fixture"]); + + let produced = Command::new(env!("CARGO_BIN_EXE_hu13")) + .arg("build") + .arg(&repo) + .arg("-o") + .arg(&out) + .output() + .expect("spawn hu13 build"); + let stderr = String::from_utf8_lossy(&produced.stderr); + assert!( + produced.status.success(), + "hostile frontmatter must not fail the build: {stderr}" + ); + + for name in ["issue", "pr", "release", "dotdot"] { + let p = escaped(name); + assert!( + !p.exists(), + "build wrote outside out/: {} exists", + p.display() + ); + } + // Nothing landed at /tmp directly either (traversal is blocked, not + // merely clamped). + assert!(!Path::new(&format!("/tmp/{marker}-issue.html")).exists()); + + // Refused entries are warned about, naming the bad value. + assert!(stderr.contains("unsafe id"), "stderr: {stderr}"); + assert!(stderr.contains("unsafe version"), "stderr: {stderr}"); + + // Valid content survives. + assert!(out.join("releases/v1.0.0.html").is_file()); + // Build still completes with an output tree. + assert!(out.join("index.html").is_file()); +} -- 2.56.0