fonts-build: keep manifest font and licence paths inside the checkout

A manifest file or licence_file that is absolute, climbs out with .., or
resolves through a symlink to a file outside the checkout is now an
Error::OutsideCheckout instead of being embedded with include_bytes! or
copied (and written) outside the directory given to Embed::serve.

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<String> = 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<String, Error> {
 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<String>) -> Result<String, Error> {
+        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<String> = 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<String>, known: Vec<String> },
     /// 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())
+            }
         }
     }
 }

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");
+}