From 01bbdb24bc27b087893afc37dff07d79862a5b97 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 22 Oct 2026 18:42:51 -0400 Subject: [PATCH] security: confine craft-fonts manifest paths and reject newline directives (CWE-22, CWE-74) (#1841) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The optional `CRAFT_FONTS_DIR` build input lets a font manifest select the file each face is embedded from. `build.rs` canonicalised that file with no containment check, so an absolute path, a `..` traversal, or a symlink inside the checkout could make the generated `include_bytes!` read an arbitrary file on the build host and expose its bytes through `CRAFT_FONTS` (CWE-22). It also printed the resolved path straight into Cargo's newline-delimited build-script protocol (`cargo::rerun-if-changed=…`). A path containing an embedded newline — reachable through a crafted symlink target — could inject a second directive such as `cargo::rustc-env=OUT_DIR=…`, redirecting the generated-source include to attacker-supplied Rust (CWE-74). - `resolve_font` rejects absolute and `..` manifest paths and requires the canonicalised result to stay inside the canonicalised checkout root. - `rerun_if_changed` refuses any path containing CR/LF before emitting the directive; `one_line` collapses CR/LF in checkout-derived diagnostics. Addresses findings rows 5 (CWE-22) and 14 (CWE-74). --- crates/fonts/build.rs | 47 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/crates/fonts/build.rs b/crates/fonts/build.rs index 7eb2a784b..658f5b7b3 100644 --- a/crates/fonts/build.rs +++ b/crates/fonts/build.rs @@ -18,9 +18,9 @@ fn main() { match craft_fonts(&dir) { Ok(entries) => src.push_str(&entries), Err(e) if std::env::var_os("CRAFT_FONTS_REQUIRED").is_some() => { - println!("cargo::error=CRAFT_FONTS_DIR={}: {e}", dir.display()); + println!("cargo::error=CRAFT_FONTS_DIR={}: {}", one_line(&dir.display().to_string()), one_line(&e)); } - Err(e) => println!("cargo::warning=building without craft-fonts: CRAFT_FONTS_DIR={}: {e}", dir.display()), + Err(e) => println!("cargo::warning=building without craft-fonts: CRAFT_FONTS_DIR={}: {}", one_line(&dir.display().to_string()), one_line(&e)), } } src.push_str("];\n"); @@ -37,7 +37,9 @@ const BARRED_FAMILIES: [&str; 5] = ["Source Han", "Source Serif", "Source Sans", /// One `CraftFont { .. }` initialiser per manifest line, in manifest order. fn craft_fonts(dir: &std::path::Path) -> Result { let manifest = dir.join("fonts/manifest.txt"); - println!("cargo::rerun-if-changed={}", manifest.display()); + rerun_if_changed(&manifest)?; + // Resolve the checkout root once so every manifest file can be confined inside it. + let root = dir.canonicalize().map_err(|e| format!("{}: {e}", dir.display()))?; let text = std::fs::read_to_string(&manifest).map_err(|e| format!("{}: {e}", manifest.display()))?; let wasm = std::env::var("CARGO_CFG_TARGET_ARCH").is_ok_and(|a| a == "wasm32"); let mut out = String::new(); @@ -55,8 +57,8 @@ fn craft_fonts(dir: &std::path::Path) -> Result { if wasm && !web_face { continue; } - let path = dir.join(file).canonicalize().map_err(|e| format!("{file}: {e}"))?; - println!("cargo::rerun-if-changed={}", path.display()); + let path = resolve_font(&root, file)?; + rerun_if_changed(&path)?; let scripts: Vec = scripts.split(',').map(|s| format!("{:?}", s.trim())).collect(); let _ = writeln!( out, @@ -67,3 +69,38 @@ fn craft_fonts(dir: &std::path::Path) -> Result { } Ok(out) } + +/// Resolve a manifest `file` field to a real path *confined to the checkout* `root` (CWE-22). +/// +/// The field is rejected if it is absolute or contains a parent (`..`) component, and the +/// canonicalised result must still live inside `root`, so a crafted manifest or a symlink in the +/// checkout cannot make the generated `include_bytes!` read an arbitrary file on the build host. +fn resolve_font(root: &std::path::Path, file: &str) -> Result { + let rel = std::path::Path::new(file); + if rel.is_absolute() || rel.components().any(|c| matches!(c, std::path::Component::ParentDir)) { + return Err(format!("{file}: manifest paths must be relative and stay inside the checkout")); + } + let path = root.join(rel).canonicalize().map_err(|e| format!("{file}: {e}"))?; + if !path.starts_with(root) { + return Err(format!("{file}: resolves outside the craft-fonts checkout")); + } + Ok(path) +} + +/// Collapse CR/LF in a diagnostic derived from checkout contents to a single space, so it cannot +/// introduce an additional Cargo directive on the newline-delimited build-script stream (CWE-74). +fn one_line(s: &str) -> String { + s.replace(['\n', '\r'], " ") +} + +/// Emit a `cargo::rerun-if-changed` directive for `path`, refusing paths that contain a line +/// separator (CWE-74): Cargo's build-script protocol is newline-delimited, so a path with an +/// embedded CR/LF could smuggle an extra directive such as `cargo::rustc-env=OUT_DIR=…`. +fn rerun_if_changed(path: &std::path::Path) -> Result<(), String> { + let shown = path.display().to_string(); + if shown.contains(['\n', '\r']) { + return Err(format!("{shown:?}: path contains a line separator")); + } + println!("cargo::rerun-if-changed={shown}"); + Ok(()) +}