From 83c2e7bf8c6e409581b099a098eb00ac423d8b6c Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Sun, 1 Nov 2026 19:22:52 +0000 Subject: [PATCH] =?UTF-8?q?CP4.1:=20review=20=E2=80=94=20record=20rebuilt?= =?UTF-8?q?=20atomically=20in=20place=20(and=20when=20missing),=20staging?= =?UTF-8?q?=20never=20deletes=20another=20process's=20folder,=20zstd=20fra?= =?UTF-8?q?mes=20capped,=20neutral=20index=20errors,=20plain=20index=20tex?= =?UTF-8?q?t,=20optional=20source?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - read_installed copies an unreadable record to .bad and writes the rebuilt one atomically over it; a missing record with pack folders is rebuilt too; install reads the record before moving the pack in. - Stage::new uses create_dir and takes the next name on AlreadyExists (PID namespaces in Flatpak/containers); stale folders are left to sweep_staging. - At most 4096 zstd frames per archive, empty ones included. - IndexError and the sha256 mismatch say "the index", not "the font index". - licence, release, version and class lose control and Cf characters like name. - PackEntry.source: Option, an https URL or the index is refused. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FypgCX8LsYYE8fsj4byBm2 --- crates/font-store/README.md | 2 +- crates/font-store/src/index.rs | 12 ++-- crates/font-store/src/language_packs.rs | 66 ++++++++++++++----- crates/font-store/src/language_packs/tests.rs | 45 ++++++++++++- crates/font-store/src/store.rs | 20 ++++-- crates/font-store/src/verify.rs | 2 +- 6 files changed, 114 insertions(+), 33 deletions(-) diff --git a/crates/font-store/README.md b/crates/font-store/README.md index 64e61b562..fcbade7fd 100644 --- a/crates/font-store/README.md +++ b/crates/font-store/README.md @@ -138,7 +138,7 @@ let installed = store.installed_language_packs(); | `installed_language_packs()` | The recorded packs whose folder is there. | | `language_packs_dir()` | `/language-packs`, for `craft_proof::packs::set_packs_dir`. | -**Unpacking is hostile-input safe:** pure-Rust zstd (`ruzstd`, MIT; every frame is read, and nothing but tar padding may follow the archive) and `tar` (MIT OR Apache-2.0); regular files and the `LICENCES` folder only (no links or devices). Only the contract's names are allowed: `pack.toml`, `spelling.aff`, `spelling.dic`, `hyphenation.pat` at the root and `LICENCES/` (`[A-Za-z0-9._-]`, no leading or trailing dot, no Windows device names such as `CON` or `nul.txt`). Each file at most 64 MiB, 256 MiB and 256 entries per pack. `pack.toml` is required and must name the entry's locale; the files the index promises (`spelling`, `hyphenation`) must be there. Index entries need a lowercase BCP 47 locale (listed once, not a Windows device name), a sha256 and an `https://` URL; pack names lose control and bidi/format characters before they are stored or listed. An unreadable `language-packs.json` is kept as `language-packs.json.bad` and rebuilt from the pack folders (each `pack.toml`), so no installed pack drops out of the list. +**Unpacking is hostile-input safe:** pure-Rust zstd (`ruzstd`, MIT; every frame is read, and nothing but tar padding may follow the archive) and `tar` (MIT OR Apache-2.0); regular files and the `LICENCES` folder only (no links or devices). Only the contract's names are allowed: `pack.toml`, `spelling.aff`, `spelling.dic`, `hyphenation.pat` at the root and `LICENCES/` (`[A-Za-z0-9._-]`, no leading or trailing dot, no Windows device names such as `CON` or `nul.txt`). Each file at most 64 MiB, 256 MiB and 256 entries per pack. `pack.toml` is required and must name the entry's locale; the files the index promises (`spelling`, `hyphenation`) must be there. Index entries need a lowercase BCP 47 locale (listed once, not a Windows device name), a sha256 and an `https://` URL; names, licences, versions, classes and the release lose control and bidi/format characters before they are stored or listed; an optional `source` must be an `https://` URL. At most 4096 zstd frames per archive. An unreadable `language-packs.json` is copied to `language-packs.json.bad` and rebuilt from the pack folders (each `pack.toml`) atomically in place, as is a missing one when pack folders are there, so no installed pack drops out of the list. **Developer override (testing only — bypasses the pin):** `CRAFT_LANGUAGE_PACK_INDEX=` replaces the pinned index URL with an `http://` URL on a loopback host (`127.0.0.1`, `localhost`, `[::1]`), an `https://` URL or a local file path (`file://` URLs are not taken), and **skips the index's sha256 pin**. Archives are still checked against the sha256 and size in that index, and may then also be loopback `http://` URLs or local paths. Off unless the variable is set; it works in release builds too (for end-to-end tests of a real build), a warning is logged whenever it is used, and `language_pack_index_overridden() -> Option` returns the value so apps can show that the pin is bypassed. Never set it for users. diff --git a/crates/font-store/src/index.rs b/crates/font-store/src/index.rs index 96585aa1b..aad4bc45d 100644 --- a/crates/font-store/src/index.rs +++ b/crates/font-store/src/index.rs @@ -24,25 +24,25 @@ pub const LICENSES: [&str; 3] = ["OFL", "APACHE2", "UFL"]; #[derive(Debug, thiserror::Error)] pub enum IndexError { /// More than [`MAX_INDEX_BYTES`]. - #[error("the font index is larger than {max} bytes")] + #[error("the index is larger than {max} bytes")] TooLarge { /// The limit. max: usize, }, /// Not JSON, or a field has the wrong type. - #[error("the font index can't be read: {0}")] + #[error("the index can't be read: {0}")] Json(String), /// A major version this crate doesn't know. - #[error("the font index has version {0}; this build reads version {MAJOR_VERSION}.x")] + #[error("the index has version {0}; this build reads version {MAJOR_VERSION}.x")] Version(String), /// An entry breaks the format's rules. - #[error("the font index has an invalid entry: {0}")] + #[error("the index has an invalid entry: {0}")] Invalid(String), /// A gzipped index that doesn't decompress. - #[error("the gzipped font index can't be read: {0}")] + #[error("the gzipped index can't be read: {0}")] Gzip(String), /// The bytes aren't the pinned ones. - #[error("the font index doesn't have the sha256 this build expects")] + #[error("the index doesn't have the sha256 this build expects")] Checksum, } diff --git a/crates/font-store/src/language_packs.rs b/crates/font-store/src/language_packs.rs index 948302bf4..c1b4e8dfa 100644 --- a/crates/font-store/src/language_packs.rs +++ b/crates/font-store/src/language_packs.rs @@ -97,6 +97,9 @@ pub struct PackEntry { /// Whether it has hyphenation patterns. #[serde(default)] pub hyphenation: bool, + /// Where the data comes from (an `https://` page), for display; optional. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub source: Option, } /// An installed pack, as recorded when it was installed. @@ -151,9 +154,18 @@ impl PackIndex { if !(p.url.starts_with("https://") || local && local_source(&p.url).is_some()) { return Err(bad(format!("pack {}: {} is not an https URL", p.locale, p.url.chars().take(200).collect::()))); } + if let Some(src) = &p.source + && !(src.starts_with("https://") && src.len() <= 2048 && !src.chars().any(|c| c.is_control() || is_format(c) || c == ' ')) + { + return Err(bad(format!("pack {}: source {:?} is not an https URL", p.locale, src.chars().take(200).collect::()))); + } } + // Text shown to people loses control and bidi/format characters. + index.release = plain_text(&index.release); for p in &mut index.packs { - p.name = plain_text(&p.name); + for t in [&mut p.name, &mut p.version, &mut p.licence, &mut p.class] { + *t = plain_text(t); + } } Ok(index) } @@ -366,13 +378,15 @@ impl FontStore { let record = InstalledLanguagePack { locale: pack.locale.clone(), name: plain_text(&pack.name), - release: index.release.chars().take(200).collect(), - version: pack.version.chars().take(200).collect(), - licence: pack.licence.chars().take(200).collect(), + release: plain_text(&index.release), + version: plain_text(&pack.version), + licence: plain_text(&pack.licence), sha256: pack.sha256.clone(), size: pack.size, }; let _lock = self.lock()?; + // The record first: rebuilding a damaged one must not count the pack being installed. + let mut installed = self.read_installed()?; let packs = self.language_packs_dir(); std::fs::create_dir_all(&packs).map_err(io("creating the Language packs folder"))?; let dest = packs.join(&pack.locale); @@ -387,7 +401,6 @@ impl FontStore { } return Err(io(format!("installing the {} pack", pack.locale))(e)); } - let mut installed = self.read_installed()?; installed.packs.retain(|p| p.locale != record.locale); installed.packs.push(record.clone()); installed.packs.sort_by(|a, b| a.locale.cmp(&b.locale)); @@ -410,7 +423,13 @@ impl FontStore { .packs .into_iter() .filter(|p| is_locale(&p.locale) && dir.join(&p.locale).is_dir()) - .map(|p| InstalledLanguagePack { name: plain_text(&p.name), ..p }) + .map(|p| InstalledLanguagePack { + name: plain_text(&p.name), + release: plain_text(&p.release), + version: plain_text(&p.version), + licence: plain_text(&p.licence), + ..p + }) .collect() } @@ -436,20 +455,24 @@ impl FontStore { // `stage` deletes the old folder when dropped. } - /// The record of installed packs (none yet: empty). One that can't be read is kept aside as - /// `language-packs.json.bad` and rebuilt from the pack folders on disk, so no installed pack - /// is ever dropped from it. + /// The record of installed packs. One that can't be read is copied aside as + /// `language-packs.json.bad` and rebuilt from the pack folders on disk (written atomically + /// over it), so no installed pack is ever dropped from it; a missing one is rebuilt the same + /// way when pack folders are there (none: empty). fn read_installed(&self) -> Result { let path = self.dir().join(INSTALLED); - if std::fs::symlink_metadata(&path).is_err() { - return Ok(Installed::default()); - } - if let Some(i) = read_limited(&path, MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice::(&b).ok()) { + let there = std::fs::symlink_metadata(&path).is_ok(); + if there && let Some(i) = read_limited(&path, MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice::(&b).ok()) { return Ok(i); } - log::warn!("{}: can't be read; rebuilt from the Language pack folders", path.display()); let rebuilt = self.installed_from_disk(); - let _ = std::fs::rename(&path, self.dir().join(format!("{INSTALLED}.bad"))); + if !there && rebuilt.packs.is_empty() { + return Ok(Installed::default()); + } + log::warn!("{}: {}; rebuilt from the Language pack folders", path.display(), if there { "can't be read" } else { "missing" }); + if there { + let _ = std::fs::copy(&path, self.dir().join(format!("{INSTALLED}.bad"))); + } self.write_installed(&rebuilt)?; Ok(rebuilt) } @@ -495,11 +518,16 @@ impl FontStore { /// a concatenation of frames is a valid `.zst` too, and must not stop after the first). struct Frames<'a> { decoder: Option>, + /// Frames started so far, empty ones included (at most [`MAX_PACK_FRAMES`]). + frames: usize, } +/// Most zstd frames in a pack archive (`zstd` writes one; empty frames count too). +const MAX_PACK_FRAMES: usize = 4096; + impl<'a> Frames<'a> { fn new(archive: &'a [u8]) -> Result { - Ok(Self { decoder: Some(ruzstd::decoding::StreamingDecoder::new(archive).map_err(|e| format!("not zstd: {e}"))?) }) + Ok(Self { decoder: Some(ruzstd::decoding::StreamingDecoder::new(archive).map_err(|e| format!("not zstd: {e}"))?), frames: 1 }) } } @@ -512,6 +540,12 @@ impl Read for Frames<'_> { return Ok(n); } let rest: &[u8] = d.get_ref(); + if !rest.is_empty() { + self.frames += 1; + if self.frames > MAX_PACK_FRAMES { + return Err(std::io::Error::other(format!("more than {MAX_PACK_FRAMES} zstd frames"))); + } + } self.decoder = if rest.is_empty() { None } else { Some(ruzstd::decoding::StreamingDecoder::new(rest).map_err(std::io::Error::other)?) }; } } diff --git a/crates/font-store/src/language_packs/tests.rs b/crates/font-store/src/language_packs/tests.rs index 7cd107a8b..5b7d13edb 100644 --- a/crates/font-store/src/language_packs/tests.rs +++ b/crates/font-store/src/language_packs/tests.rs @@ -348,6 +348,13 @@ fn hostile_archives_are_refused() { let dest = temp("frames").join("nl"); unpack(&[zst(a), zst(b)].concat(), &dest, &nl).unwrap(); assert!(dest.join("LICENCES/COPYING").is_file()); + // Empty frames are read past, but only so many. + let one = zst(&tar_of(&[("pack.toml", FILE, toml)])); + let empty = zst(b""); + unpack(&[one.clone(), empty.clone(), empty.clone()].concat(), &temp("empties").join("nl"), &nl).unwrap(); + let many: Vec = std::iter::once(one).chain(std::iter::repeat_n(empty, MAX_PACK_FRAMES)).flatten().collect(); + let err = unpack(&many, &temp("frames-cap").join("nl"), &nl).unwrap_err(); + assert!(err.contains("frames"), "{err}"); // The index's promises are checked against the files. let bare = zst(&tar_of(&[("pack.toml", FILE, toml)])); for e in [PackEntry { spelling: true, ..nl.clone() }, PackEntry { hyphenation: true, ..nl.clone() }] { @@ -389,13 +396,42 @@ fn a_corrupt_record_is_rebuilt_from_the_folders() { let mut locales: Vec = store.installed_language_packs().into_iter().map(|p| p.locale).collect(); locales.sort(); assert_eq!(locales, ["de", "nl"]); - assert!(store.dir().join("language-packs.json.bad").is_file(), "the unreadable record is kept aside"); + assert_eq!(std::fs::read(store.dir().join("language-packs.json.bad")).unwrap(), b"{ not json", "the unreadable record is kept aside"); + assert!(serde_json::from_slice::(&std::fs::read(&record).unwrap()).is_ok(), "rebuilt in place"); + // A missing record with pack folders is rebuilt too. + std::fs::remove_file(&record).unwrap(); + assert_eq!(store.installed_language_packs().len(), 2); + let nl_again = PackEntry { url: format!("{RELEASE}nl.tar.zst"), ..nl.clone() }; + std::fs::remove_file(&record).unwrap(); + store.install_pack_from(false, &index, &nl_again, &Cancel::new(), &mut |_| {}).unwrap(); + assert_eq!(store.installed_language_packs().len(), 2, "de kept when the record was missing"); // Removing works on a rebuilt record too. std::fs::write(&record, b"\xff").unwrap(); store.remove_language_pack("nl").unwrap(); assert_eq!(store.installed_language_packs().into_iter().map(|p| p.locale).collect::>(), ["de"]); } +#[test] +fn index_text_is_plain_and_sources_are_checked() { + let a = pack_archive("nl", "x"); + let mut e = entry("nl", &format!("{RELEASE}nl.tar.zst"), &a); + e["licence"] = serde_json::json!("MIT\u{202e}"); + e["version"] = serde_json::json!("ab\u{0}c"); + e["class"] = serde_json::json!("\u{200f}Embeddable"); + e["source"] = serde_json::json!("https://example.org/nl"); + let json = serde_json::to_vec(&serde_json::json!({"format": 1, "release": "r1\u{2066}", "packs": [e.clone()]})).unwrap(); + let index = PackIndex::from_json(&json).unwrap(); + let p = &index.packs[0]; + assert_eq!((index.release.as_str(), p.licence.as_str(), p.version.as_str(), p.class.as_str()), ("r1", "MIT", "abc", "Embeddable")); + assert_eq!(p.source.as_deref(), Some("https://example.org/nl")); + assert_eq!(PackIndex::from_json(&index_json(&[entry("nl", &format!("{RELEASE}nl.tar.zst"), &a)])).unwrap().packs[0].source, None); + for bad in ["javascript:alert(1)", "http://example.org", "https://a b", "https://a\u{202e}b"] { + e["source"] = serde_json::json!(bad); + let err = PackIndex::from_json(&index_json(std::slice::from_ref(&e))).unwrap_err(); + assert!(!err.to_string().contains("font"), "{err}"); + } +} + #[test] fn bad_hand_made_entries_are_index_errors() { let (store, _server, pin, _) = setup("variant"); @@ -413,8 +449,8 @@ fn the_override_is_visible() { assert_eq!(language_pack_index_overridden(), set); } -/// A staging folder an earlier process with the same id left behind is cleared, so an unpack -/// (`create_new`) can't fail on, or mix in, its files. +/// A staging folder of the same name (an earlier process, or another one in a PID namespace) is +/// never used or deleted: the next name is taken, so an unpack can't mix in its files. #[test] fn a_stale_stage_folder_is_cleared() { let store = temp("stale-stage"); @@ -426,4 +462,7 @@ fn a_stale_stage_folder_is_cleared() { } let stage = Stage::new(&store).unwrap(); assert_eq!(std::fs::read_dir(&stage.dir).unwrap().count(), 0, "{}", stage.dir.display()); + // The others are left alone: they may be another process's (PID namespaces). + let left = std::fs::read_dir(&staging).unwrap().flatten().filter(|e| e.path().join("nl/pack.toml").is_file()).count(); + assert_eq!(left, 512); } diff --git a/crates/font-store/src/store.rs b/crates/font-store/src/store.rs index 7c3ed0f3b..4dabaad85 100644 --- a/crates/font-store/src/store.rs +++ b/crates/font-store/src/store.rs @@ -839,13 +839,21 @@ pub(crate) struct Stage { impl Stage { pub(crate) fn new(store: &Path) -> Result { static N: AtomicU64 = AtomicU64::new(0); - let dir = store.join(STAGING).join(format!("{}-{}", std::process::id(), N.fetch_add(1, Ordering::Relaxed))); - // A folder left by an earlier process with the same id is cleared, not mixed in. - if std::fs::symlink_metadata(&dir).is_ok() { - std::fs::remove_dir_all(&dir).map_err(io("clearing an old download folder"))?; + let staging = store.join(STAGING); + std::fs::create_dir_all(&staging).map_err(io("creating a download folder"))?; + // A folder of that name may be another process's (PID namespaces in Flatpak or + // containers repeat ids) or a stale one: never touched here, the next name is taken + // (stale ones go in `sweep_staging`). + let mut last = None; + for _ in 0..1000 { + let dir = staging.join(format!("{}-{}", std::process::id(), N.fetch_add(1, Ordering::Relaxed))); + match std::fs::create_dir(&dir) { + Ok(()) => return Ok(Self { dir }), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => last = Some(e), + Err(e) => return Err(io("creating a download folder")(e)), + } } - std::fs::create_dir_all(&dir).map_err(io("creating a download folder"))?; - Ok(Self { dir }) + Err(io("creating a download folder")(last.unwrap_or_else(|| std::io::Error::other("no free name")))) } /// Writes a verified download as `.part`, which no font scan picks up. diff --git a/crates/font-store/src/verify.rs b/crates/font-store/src/verify.rs index ba976df16..5ae297d0b 100644 --- a/crates/font-store/src/verify.rs +++ b/crates/font-store/src/verify.rs @@ -20,7 +20,7 @@ pub enum VerifyError { actual: u64, }, /// The sha256 differs. - #[error("the file's sha256 doesn't match the font index")] + #[error("the file's sha256 doesn't match the index")] Sha256, /// It doesn't parse as a font with outlines. #[error("the file is not a font, or a damaged one")]