diff --git a/crates/fonts-build/src/embed.rs b/crates/fonts-build/src/embed.rs index c6937ba5..31a93559 100644 --- a/crates/fonts-build/src/embed.rs +++ b/crates/fonts-build/src/embed.rs @@ -6,7 +6,7 @@ use std::collections::HashSet; use std::ffi::OsString; use std::fmt::{self, Write as _}; use std::io; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use crate::cmap::Cmap; use crate::manifest::{ManifestEntry, ParseError, parse_manifest, script_matches}; @@ -240,6 +240,8 @@ impl Embed { // Cargo treats a missing watched file as changed, which would rerun the build every time. cargo.push(format!("cargo::rerun-if-changed={}", manifest.display())); let entries = parse_manifest(&text).map_err(|error| Error::Parse { manifest: manifest.clone(), error })?; + // The manifest read, so the checkout exists. Faces and licences must resolve under it. + let root = dir.canonicalize().map_err(|source| Error::Read { path: dir.to_path_buf(), source })?; if let Some(families) = &self.families { let unknown: Vec = families.iter().filter(|f| !entries.iter().any(|e| &e.family == *f)).cloned().collect(); if !unknown.is_empty() { @@ -278,9 +280,19 @@ impl Embed { uncut.push(format!("{} {}", entry.family, entry.style)); continue; } + // A manifest path names a file in the checkout, and is served under the same name. + for file in [&entry.file, &entry.licence_file] { + if !file.is_empty() && !is_checkout_relative(file) { + return Err(Error::OutsideCheckout { manifest, file: file.clone() }); + } + } let file = dir.join(&entry.file); // `include_bytes!` resolves relative paths from the including source file. let path = file.canonicalize().map_err(|source| Error::Read { path: file, source })?; + // A symlink in the checkout must not pull a file from elsewhere on the build host. + if !path.starts_with(&root) { + return Err(Error::OutsideCheckout { manifest, file: entry.file }); + } cargo.push(format!("cargo::rerun-if-changed={}", path.display())); let (index, sha256) = (entry.index, entry.sha256.clone()); faces.push(Face { entry, path, index, cut, sha256 }); @@ -370,7 +382,7 @@ impl Embed { for family in self.elsewhere.iter().filter(|f| !entries.iter().any(|e| &e.family == *f)) { cargo.push(format!("cargo::warning=craft-fonts: embedded_elsewhere names {family}, which {} lacks", manifest.display())); } - entries.into_iter().filter(|e| self.elsewhere.contains(&e.family)).collect() + entries.into_iter().filter(|e| self.elsewhere.contains(&e.family) && is_checkout_relative(&e.file)).collect() } /// Warn about each catalog's and [`Embed::check_text`] file's characters that neither an @@ -476,6 +488,8 @@ fn table(faces: &[Face]) -> Result { impl Serve { /// Copy the faces' files and licences into the served directory; returns the table. fn copy(&self, checkout: &Path, faces: &[Face], cargo: &mut Vec) -> Result { + let root = checkout.canonicalize().map_err(|source| Error::Read { path: checkout.to_path_buf(), source })?; + let manifest = checkout.join("fonts/manifest.txt"); let mut copied: Vec = Vec::new(); let mut src = String::from("// Generated by craft-fonts-build. Do not edit.\n&[\n"); for face in faces { @@ -485,12 +499,23 @@ impl Serve { let size = font.len(); if !copied.contains(&served) { copied.push(served.clone()); + // `select` checked the manifest paths; the served names must stay under `dir` too. + for name in [served.as_str(), served_path(&entry.licence_file)] { + if !name.is_empty() && !is_checkout_relative(name) { + return Err(Error::OutsideCheckout { manifest, file: name.to_owned() }); + } + } copy_file(path, &self.dir.join(&served), cargo)?; - let licence = checkout.join(&entry.licence_file); - if entry.licence_file.is_empty() || !licence.is_file() { - cargo.push(format!("cargo::warning=craft-fonts: no licence file beside {}", entry.file)); - } else { - copy_file(&licence, &self.dir.join(served_path(&entry.licence_file)), cargo)?; + let licence = (!entry.licence_file.is_empty()) + .then(|| checkout.join(&entry.licence_file).canonicalize().ok()) + .flatten() + .filter(|licence| licence.is_file()); + match licence { + None => cargo.push(format!("cargo::warning=craft-fonts: no licence file beside {}", entry.file)), + Some(licence) if !licence.starts_with(&root) => { + return Err(Error::OutsideCheckout { manifest, file: entry.licence_file.clone() }); + } + Some(licence) => copy_file(&licence, &self.dir.join(served_path(&entry.licence_file)), cargo)?, } } let coverage = crate::coverage::of_face(&font, face.index).unwrap_or_else(|| { @@ -550,6 +575,12 @@ fn served_file(face: &Face) -> String { } } +/// Whether a manifest path stays under the directory it is joined to: relative, without `..`, a +/// root or a drive prefix. +fn is_checkout_relative(file: &str) -> bool { + !file.is_empty() && Path::new(file).components().all(|c| matches!(c, Component::Normal(_) | Component::CurDir)) +} + /// A checkout path relative to the served directory: `fonts/a/A.ttf` → `a/A.ttf`. fn served_path(file: &str) -> &str { file.strip_prefix("fonts/").unwrap_or(file) @@ -622,6 +653,9 @@ pub enum Error { UnknownFamilies { manifest: PathBuf, families: Vec, known: Vec }, /// A face to embed has a path that isn't UTF-8, which `include_bytes!` can't name. NonUtf8Path { path: PathBuf }, + /// A manifest font or licence path is absolute, climbs out with `..`, or resolves (through a + /// symlink) to a file outside the checkout. + OutsideCheckout { manifest: PathBuf, file: String }, } impl fmt::Display for Error { @@ -654,6 +688,9 @@ impl fmt::Display for Error { known.join(", ") ), Self::NonUtf8Path { path } => write!(f, "cannot embed {}: include_bytes! needs a UTF-8 path", path.display()), + Self::OutsideCheckout { manifest, file } => { + write!(f, "{}: {file:?} is not a file inside the checkout", manifest.display()) + } } } } diff --git a/crates/fonts-build/src/tests.rs b/crates/fonts-build/src/tests.rs index f614dc6e..8676646f 100644 --- a/crates/fonts-build/src/tests.rs +++ b/crates/fonts-build/src/tests.rs @@ -692,3 +692,52 @@ fn served_faces_carry_the_characters_their_cmap_maps() { assert!(table.contains(r#"family: "Broken""#) && table.ends_with("coverage: b\"\" },\n]\n"), "{table}"); assert!(warnings(&r).iter().any(|w| w.contains("no character map in fonts/1.ttf")), "{:?}", warnings(&r)); } + +#[test] +fn manifest_paths_stay_inside_the_checkout() { + let scratch = Scratch::new("outside"); + let dir = scratch.0.join("craft-fonts"); + std::fs::create_dir_all(dir.join("fonts")).unwrap(); + std::fs::write(dir.join("fonts/a.ttf"), b"a").unwrap(); + std::fs::write(dir.join("fonts/OFL.txt"), b"licence").unwrap(); + let secret = scratch.0.join("secret.txt"); + std::fs::write(&secret, b"host file").unwrap(); + let served = scratch.0.join("served"); + let line = |file: &str, licence: &str| format!("Sans | Regular | {file} | Latn | OFL-1.1 | {licence} | 00 | src\n"); + let outside = |r: &Run| matches!(r.result, Err(Error::OutsideCheckout { .. })); + + // Parent components and absolute paths, for the face and for its licence, embedded or served. + for (file, licence) in [ + ("../secret.txt", "fonts/OFL.txt"), + (secret.to_str().unwrap(), "fonts/OFL.txt"), + ("fonts/../../secret.txt", "fonts/OFL.txt"), + ("fonts/a.ttf", "../secret.txt"), + ("fonts/a.ttf", secret.to_str().unwrap()), + ] { + std::fs::write(dir.join("fonts/manifest.txt"), line(file, licence)).unwrap(); + let r = run(Embed::new(), Some(&dir), None); + assert!(r.result.is_ok() && !r.table.contains("include_bytes!"), "{file} {licence}: {:?} {}", r.result, r.table); + assert!(warnings(&r).iter().any(|w| w.contains("not a file inside the checkout")), "{file} {licence}: {:?}", r.cargo); + assert!(outside(&run(Embed::new(), Some(&dir), Some("1"))), "{file} {licence}"); + assert!(outside(&run(Embed::new().serve(&served, "fonts/"), Some(&dir), Some("1"))), "{file} {licence}"); + } + assert!(!served.exists()); + + // A symlink in the checkout that leads out of it. + #[cfg(unix)] + { + std::os::unix::fs::symlink(&secret, dir.join("fonts/link.ttf")).unwrap(); + std::os::unix::fs::symlink(&secret, dir.join("fonts/LINK.txt")).unwrap(); + std::fs::write(dir.join("fonts/manifest.txt"), line("fonts/link.ttf", "fonts/OFL.txt")).unwrap(); + assert!(outside(&run(Embed::new(), Some(&dir), Some("1")))); + std::fs::write(dir.join("fonts/manifest.txt"), line("fonts/a.ttf", "fonts/LINK.txt")).unwrap(); + assert!(outside(&run(Embed::new().serve(&served, "fonts/"), Some(&dir), Some("1")))); + assert!(!served.join("LINK.txt").exists()); + } + + // A plain checkout path still embeds and serves. + std::fs::write(dir.join("fonts/manifest.txt"), line("fonts/a.ttf", "fonts/OFL.txt")).unwrap(); + assert_eq!(run(Embed::new(), Some(&dir), Some("1")).result.unwrap(), ["Sans Regular"]); + run(Embed::new().serve(&served, "fonts/"), Some(&dir), Some("1")).result.unwrap(); + assert_eq!(std::fs::read(served.join("OFL.txt")).unwrap(), b"licence"); +}