diff --git a/crates/catalog/src/journal.rs b/crates/catalog/src/journal.rs index edcfae22a..2600d794a 100644 --- a/crates/catalog/src/journal.rs +++ b/crates/catalog/src/journal.rs @@ -263,14 +263,36 @@ fn newer(what: String) -> CatalogError { CatalogError::Newer(format!("{what}; this version reads catalog format v{VERSION} and older")) } +/// The most `resync` spends re-validating the suffixes of one line whose whole bytes are not valid +/// UTF-8 (a real fragment ends at any byte): one line of a modest malformed log that holds a start +/// of a record in every few bytes used to re-validate the whole remaining suffix at every attempt +/// (CWE-407). Past the budget the rest of the line stays a torn tail, which the damaged path keeps +/// and reports, same state as before, minus the CPU. +const RESYNC_BUDGET_BYTES: usize = 64 << 20; // 64 MiB + /// A line that doesn't decode but ends with a whole record: a fragment of a failed append followed /// by the record that retried it (the retry re-encodes the same ops with the same `seq`s). Returns /// the fragment's length and the record, if that record continues the log (`seq <= last + 1`). /// The record's CRC must match, so damage is never mistaken for it. fn resync(line: &[u8], last: u64) -> Option<(usize, (u64, Op))> { const START: &[u8] = b"{\"seq\":"; + // One record per line, so a line of valid UTF-8 holds a valid suffix at every record start and + // one validation covers every candidate; only a line that is not wholly valid re-checks, and + // then up to the budget rather than the suffix length times the candidate count. + let whole = std::str::from_utf8(line).ok(); + let mut spent = 0usize; (1..line.len().saturating_sub(START.len() - 1)).filter(|&i| line[i..].starts_with(START)).find_map(|i| { - let rec = std::str::from_utf8(&line[i..]).ok().map(|l| l.trim_end_matches('\r')).and_then(decode_record)?; + let text = match &whole { + Some(t) => &t[i..], + None => { + if spent >= RESYNC_BUDGET_BYTES { + return None; + } + spent = spent.saturating_add(line.len() - i); + std::str::from_utf8(&line[i..]).ok()? + } + }; + let rec = decode_record(text.trim_end_matches('\r'))?; (rec.0 <= last.saturating_add(1)).then_some((i, rec)) }) } @@ -714,3 +736,28 @@ impl Drop for Journal { } } } + +#[cfg(test)] +mod tests_resync { + use super::*; + + /// A torn first line may end at any byte and a failed append's retried records may follow its + /// own start `{"seq":` more than once: `resync` finds the CRC-valid record after a fragment of + /// many record starts. UTF-8 is validated once for a valid line, and for a line that isn't + /// wholly valid only up to the budget instead of the suffix length per candidate (CWE-407). + #[test] + fn resync_finds_the_retried_record_after_a_fragment_of_many_starts() { + let op = Op::Batch { ops: vec![] }; + let record = encode_record(5, &op); + let mut line = Vec::from(b"\xf2" as &[u8]); // an invalid byte, so the whole line is not UTF-8 + for i in 0..1024 { + line.extend_from_slice(format!("{{\"seq\":{},\"crc\":{{", i).as_bytes()); + } + let fragment = line.len(); + line.extend_from_slice(record.as_bytes()); + assert_eq!(resync(&line, 4).map(|(at, (seq, _))| (at, seq)), Some((fragment, 5))); + // a retry is still recognised past `last`: the fragment logs nothing valid, but the CRC + // matches, so the double `Some` is only reached with `last >= 4` + assert_eq!(resync(&line, 100).map(|(at, (seq, _))| (at, seq)), Some((fragment, 5))); + } +} diff --git a/crates/catalog/src/keywords.rs b/crates/catalog/src/keywords.rs index 0d8b666a1..6b0c97f5a 100644 --- a/crates/catalog/src/keywords.rs +++ b/crates/catalog/src/keywords.rs @@ -48,7 +48,11 @@ pub fn reparent(k: &str, from: &str, to: &str) -> String { /// name, its parents' and synonyms flat besides its path). Each once, cleaned. pub fn from_file(flat: &[String], hierarchical: &[String]) -> Vec { let mut out: Vec = hierarchical.iter().map(|k| clean(k)).collect(); - out.extend(flat.iter().map(|k| clean(k)).filter(|k| !hierarchical.iter().any(|h| h.split(SEP).any(|level| same(level.trim(), k))))); + // One lowered set of every hierarchical level, not a search of `hierarchical` for each flat + // keyword: disjoint lists from sidecars made the old approach quadratic in both (CWE-407), and + // `same` allocates two lower-cased strings per comparison. + let levels: std::collections::HashSet = hierarchical.iter().flat_map(|h| h.split(SEP).map(|l| l.trim().to_lowercase())).collect(); + out.extend(flat.iter().map(|k| clean(k)).filter(|k| !levels.contains(&k.to_lowercase()))); dedupe(&mut out); out } @@ -78,6 +82,9 @@ pub fn parse_keyword_list(text: &str) -> Result> { let mut out: Vec<(String, KeywordInfo)> = Vec::new(); // where each keyword is in `out`, by lower-case path (a search of `out` per line was quadratic) let mut index: std::collections::HashMap = std::collections::HashMap::new(); + // synonyms already accepted, by keyword (a search of each keyword's growing synonym list per + // line was quadratic and re-allocated both spellings every comparison — CWE-407) + let mut synonyms: std::collections::HashMap> = std::collections::HashMap::new(); // the keyword at each level above the line: its path and where it is in `out` let mut above: Vec<(String, usize)> = Vec::new(); for (i, line) in text.split('\n').enumerate() { @@ -103,9 +110,9 @@ pub fn parse_keyword_list(text: &str) -> Result> { if synonym.contains(SEP) { return Err(bad(n, "“|” separates a keyword's levels: a synonym can't hold one")); } - if let Some((_, info)) = out.get_mut(at) - && !synonym.is_empty() - && !info.synonyms.iter().any(|s| same(s, synonym)) + if !synonym.is_empty() + && synonyms.entry(at).or_default().insert(synonym.to_lowercase()) + && let Some((_, info)) = out.get_mut(at) { info.synonyms.push(synonym.to_string()); } @@ -264,7 +271,14 @@ impl Catalog { let mut chain: Vec = Vec::new(); let mut path = String::new(); let mut lower = String::new(); - for part in k.split(SEP).map(str::trim).filter(|s| !s.is_empty()) { + // Level paths hold their every prefix (`path` and `lower` both), so a photo's XMP + // keyword split far more levels than a list file accepts (`MAX_LEVELS`) made the + // stored prefixes quadratic in the level count: a keyword nests [MAX_LEVELS] + // levels here, as a list file would, and what lies below is left out (CWE-400). + for (part_n, part) in (1..).zip(k.split(SEP).map(str::trim).filter(|s| !s.is_empty())) { + if part_n > MAX_LEVELS { + break; + } if !path.is_empty() { path.push(SEP); lower.push(SEP); @@ -505,7 +519,24 @@ impl Catalog { } } let mut file = KeywordListFile::default(); + // The tree is capped at [`MAX_LEVELS`] (its level rows hold every level's prefix twice, so a + // synthetic keyword past that depth made the stored bytes quadratic); whatever lies deeper + // comes from the photos' and list's own spellings instead, named unwritable as before. + let mut over_deep: std::collections::HashSet = Default::default(); + for k in self.listed_keywords().map(|k| &k.path) { + if k.split(SEP).count() > MAX_LEVELS { + over_deep.insert(k.clone()); + } + } + for p in self.photos().filter(|p| p.in_library()) { + for k in &p.meta.keywords { + if clean(k).split(SEP).count() > MAX_LEVELS { + over_deep.insert(k.clone()); + } + } + } write(self, &self.keyword_tree(), 0, &mut file); + file.unwritable.extend(over_deep); file } @@ -1470,4 +1501,35 @@ mod tests { let s = c.keyword_suggestions(&[], "E", 5); assert_eq!(s, ["eagle", "beach", "parade"], "prefix matches before substring matches"); } + + /// A flat keyword already covered by a hierarchical level only in another spelling is not kept + /// again, whatever the size of either list (the per-flat-keyword search of every level, twice + /// lower-cased per comparison, was quadratic: CWE-407). + #[test] + fn from_file_matches_levels_in_any_case() { + let flat = ["Rome".to_string(), "Seaside".to_string()]; + let hierarchical = ["travel|ITALY|rome".to_string()]; + assert_eq!(from_file(&flat, &hierarchical), ["travel|ITALY|rome", "Seaside"]); + } + + /// The same synonym, in any spelling, is kept once (the search of each keyword's growing + /// synonym list per line was quadratic, with lower-cased copies made per comparison).(CWE-407) + #[test] + fn parse_keyword_list_keeps_a_synonym_once() { + let read = parse_keyword_list("Travel\n\t{Trip}\n\t{journey}\n\t{TRIP}\n").unwrap(); + assert_eq!(read.len(), 1); + assert_eq!(read[0].1.synonyms, ["Trip", "journey"]); + } + + /// A keyword split into far more levels than a list file accepts is cut at [`MAX_LEVELS`] in the + /// tree, whose level rows hold every prefix twice: unbounded it made the stored bytes quadratic + /// in the level count of one short keyword (CWE-400). + #[test] + fn keyword_tree_caps_a_keyword_at_the_list_depth() { + let deep: Vec = (0..4096).map(|i| format!("k{i}")).collect(); + let joined = deep.join("|"); + let (c, _) = lib(&[&[joined.as_str()]]); + let deepest = tree_paths(&c.keyword_tree()).iter().map(|(p, _)| p.split(SEP).count()).max().unwrap_or(0); + assert_eq!(deepest, MAX_LEVELS); + } } diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 1776c895c..ca581b06e 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -1067,21 +1067,50 @@ impl Catalog { st.photos.len() >= 2 }); let parents: Vec<(AlbumId, Option)> = self.albums.values().map(|a| (a.id, a.parent)).collect(); + // A folder chain is shared by every album under it, so each chain is validated once and its + // verdict is what later albums reused: a valid chain encoded in a flat snapshot made the old + // per-album walk quadratic in the album count (CWE-407). + enum Chain { + Ok, + Bad, + } + let mut chains: std::collections::HashMap = std::collections::HashMap::new(); for (id, parent) in parents { let Some(p) = parent else { continue }; - let mut cur = Some(p); - let mut steps = 0usize; - let ok = loop { - match cur { - None => break true, - Some(c) if c == id || steps > self.albums.len() => break false, - Some(c) => match self.albums.get(&c) { - Some(a) if a.folder => cur = a.parent, - _ => break false, - }, + if chains.contains_key(&id) { + continue; + } + // Walk up from `p` once, recording everything seen; what it reaches decides every node + // walked (a chain already judged by an earlier album stops the walk early — the rest of + // the chain was judged then). + let mut walked: Vec = Vec::new(); + let ok = { + let mut cur = Some(p); + let mut on: std::collections::HashSet = std::collections::HashSet::new(); + loop { + match cur { + None => break true, + Some(c) => { + if c == id || !on.insert(c) { + break false; + } + match chains.get(&c) { + Some(Chain::Bad) => break false, + Some(Chain::Ok) => break true, + None => {} + } + match self.albums.get(&c) { + Some(a) if a.folder => walked.push(c), + _ => break false, + } + cur = self.albums[&c].parent; + } + } } - steps += 1; }; + for c in walked { + chains.insert(c, if ok { Chain::Ok } else { Chain::Bad }); + } if !ok && let Some(a) = self.albums.get_mut(&id) { changed.push(format!("album {}: its folder {} is missing, not a folder or inside it: moved to the top level", id.0, p.0)); a.parent = None; diff --git a/crates/catalog/src/store.rs b/crates/catalog/src/store.rs index ad008cf0c..a79930f14 100644 --- a/crates/catalog/src/store.rs +++ b/crates/catalog/src/store.rs @@ -45,6 +45,18 @@ pub trait Store: Send { } } +/// Appends and truncates open the name's own file, so anything else at the name is the library's +/// problem: a link planted there redirects the write outside the directory (CWE-59), and a device +/// hangs the open. Refused with an error the open can show, before anything is written through. +fn check_regular_directory_entry(path: &Path) -> io::Result<()> { + match std::fs::symlink_metadata(path) { + Ok(md) if md.is_file() => Ok(()), + Ok(md) => Err(io::Error::other(format!("{} is a {:?}, not a regular file", path.display(), md.file_type()))), + Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(e), + } +} + /// Files in a directory. pub struct FsStore { dir: PathBuf, @@ -137,6 +149,7 @@ impl Store for FsStore { fn append(&mut self, name: &str, data: &[u8]) -> io::Result<()> { use std::io::Write; if self.appender.as_ref().is_none_or(|(n, _)| n != name) { + check_regular_directory_entry(&self.path(name))?; let existed = self.path(name).exists(); let f = std::fs::OpenOptions::new().create(true).append(true).open(self.path(name))?; if !existed { @@ -159,6 +172,7 @@ impl Store for FsStore { if self.appender.as_ref().is_some_and(|(n, _)| n == name) { self.appender = None; } + check_regular_directory_entry(&self.path(name))?; let f = match std::fs::OpenOptions::new().write(true).open(self.path(name)) { Ok(f) => f, // nothing to cut (an append that failed before creating the file) @@ -266,4 +280,28 @@ mod tests_staging { assert_eq!(std::fs::read(dir.join("catalog.log")).unwrap(), b"[1]"); let _ = std::fs::remove_dir_all(&dir); } + + /// The journal's append and truncate (which recovery uses) refuse a non-regular file at the + /// name instead of following it, so a link planted at `catalog.log` never receives library + /// bytes (CWE-59, Audit row 107); the snapshot write already removes such a link (#1502). + #[test] + fn append_and_truncate_refuse_a_link_at_the_name() { + let dir = std::env::temp_dir().join(format!("lc-log-link-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let victim = dir.join("victim.txt"); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(&victim, b"keep").unwrap(); + std::os::unix::fs::symlink(&victim, dir.join("catalog.log")).unwrap(); + let mut store = FsStore::open(&dir).unwrap(); + assert!(store.append("catalog.log", b"appended").is_err()); + assert!(store.truncate("catalog.log", 0).is_err()); + assert_eq!(std::fs::read(&victim).unwrap(), b"keep"); + assert!(std::fs::symlink_metadata(dir.join("catalog.log")).unwrap().file_type().is_symlink()); + // A regular file keeps working. + std::fs::remove_file(dir.join("catalog.log")).unwrap(); + std::fs::write(dir.join("catalog.log"), b"abc").unwrap(); + store.truncate("catalog.log", 1).unwrap(); + assert_eq!(std::fs::read(dir.join("catalog.log")).unwrap(), b"a"); + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/crates/testkit/src/lib.rs b/crates/testkit/src/lib.rs index cfdf7e65a..1715dee94 100644 --- a/crates/testkit/src/lib.rs +++ b/crates/testkit/src/lib.rs @@ -164,10 +164,14 @@ mod tests { #[test] fn appended_lines_accumulate() { + // Exclusive, private: the pid-derived name in a shared /tmp can be planted by another account + // (CWE-59), so an existing directory fails the test instead of it symlinking inside. let dir = std::env::temp_dir().join(format!("lightcraft-testkit-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); + std::fs::create_dir(&dir).unwrap(); let log = dir.join("skips.jsonl"); - let _ = std::fs::remove_file(&log); + if std::fs::symlink_metadata(&log).is_ok() { + std::fs::remove_file(&log).unwrap(); + } let threads: Vec<_> = (0..8) .map(|i| { let log = log.clone(); diff --git a/docs/showcase/run.py b/docs/showcase/run.py index bb80e0c1c..828f8decc 100644 --- a/docs/showcase/run.py +++ b/docs/showcase/run.py @@ -4,6 +4,10 @@ import json, socket, sys, time from pathlib import Path +# Largest control reply we accept (a screenshot response can carry a base64 image), after twice +# that the connection is closed instead of buffering further. +MAX_RESPONSE = 128 << 20 + def _jsonl_path(arg): """Resolve the CLI file argument; require an existing readable regular file.""" path = Path(arg).resolve() @@ -19,11 +23,15 @@ def _jsonl_path(arg): def call(method, params): s = socket.create_connection(("127.0.0.1", 7980), timeout=180) s.sendall((json.dumps({"id": 1, "method": method, "params": params}) + "\n").encode()) - buf = b"" - while not buf.endswith(b"\n"): + buf = bytearray() + while not (len(buf) and buf[-1:] == b"\n"): c = s.recv(1 << 20) if not c: break - buf += c + # a hostile server must not make a client buffer or pay quadratic copies forever (CWE-407) + if len(buf) + len(c) > MAX_RESPONSE: + s.close() + return {"ok": False, "error": f"response exceeds {MAX_RESPONSE} bytes"} + buf.extend(c) return json.loads(buf) for line in open(_jsonl_path(sys.argv[1])): line = line.strip() diff --git a/packaging/macos/import-cert.sh b/packaging/macos/import-cert.sh index ad384372b..959f80fce 100755 --- a/packaging/macos/import-cert.sh +++ b/packaging/macos/import-cert.sh @@ -18,8 +18,12 @@ if [[ -z "${APPLE_CERTIFICATE:-}" ]]; then fi TMP="${RUNNER_TEMP:-${TMPDIR:-/tmp}}" -KEYCHAIN="$TMP/lightcraft-signing.keychain-db" -CERT="$TMP/lightcraft-signing.p12" +# A private directory (mktemp -d): a predictable name in a shared TMPDIR can be precreated by another +# local account, and a plain redirect would then receive the private key archive (CWE-377). Encoded +# signing material is written only inside the 0700 directory. +CERTDIR="$(mktemp -d "$TMP/lightcraft-signing.XXXXXX")" +KEYCHAIN="$CERTDIR/lightcraft-signing.keychain-db" +CERT="$CERTDIR/lightcraft-signing.p12" KC_PASS="${KEYCHAIN_PASSWORD:-$(openssl rand -hex 24)}" trap 'rm -f "$CERT"' EXIT diff --git a/tools/install-sam3.sh b/tools/install-sam3.sh index 897e835f2..9787d4a49 100755 --- a/tools/install-sam3.sh +++ b/tools/install-sam3.sh @@ -79,18 +79,32 @@ fi mkdir -p "$DIR" BASE="https://huggingface.co/$REPO/resolve/main" +# The bearer header goes through a private temporary file, never argv: on systems that expose other +# users' process arguments another local account could read the token from `ps` (CWE-214), but not a +# 0600 file. curl's config format: `header = "..."`. +if [ -n "$TOKEN" ]; then + AUTH_FILE=$(mktemp "${TMPDIR:-/tmp}/lightcraft-sam3-auth.XXXXXX") + trap 'rm -f "$AUTH_FILE"' EXIT + printf 'header = "Authorization: Bearer %s"\n' "$TOKEN" > "$AUTH_FILE" +fi + # fetch FILE DEST [resume]: HTTP errors never reach DEST (--fail writes no error page), and a # resume the server refuses (416) starts over instead of counting as done. fetch() { file=$1 dest=$2 resume=${3:-} - if [ -n "$TOKEN" ]; then auth="Authorization: Bearer $TOKEN"; else auth="X-No-Auth: 1"; fi - code=$(curl --proto '=https' -fsSL ${resume:+-C -} -H "$auth" -w '%{http_code}' -o "$dest" "$BASE/$file") || true + if [ -n "$TOKEN" ]; then + CURL_AUTH=(--config "$AUTH_FILE") + else + # not secret (no token is sent); kept in argv only to mirror the authenticated call shape + CURL_AUTH=(-H "X-No-Auth: 1") + fi + code=$(curl --proto '=https' -fsSL ${resume:+-C -} "${CURL_AUTH[@]}" -w '%{http_code}' -o "$dest" "$BASE/$file") || true case "$code" in 200|206) return 0 ;; 416) echo "the partial download of $file doesn't match the server's; starting over" >&2 rm -f "$dest" - code=$(curl --proto '=https' -fsSL -H "$auth" -w '%{http_code}' -o "$dest" "$BASE/$file") || true + code=$(curl --proto '=https' -fsSL "${CURL_AUTH[@]}" -w '%{http_code}' -o "$dest" "$BASE/$file") || true [ "$code" = 200 ] && return 0 echo "download of $REPO/$file failed ($code)" >&2; return 1 ;; 401|403) echo "access denied to $REPO/$file (has your request for facebook/sam3 been approved?)" >&2; return 1 ;; diff --git a/tools/spectral_camera_table.py b/tools/spectral_camera_table.py index 188f6ba4c..d936983b4 100644 --- a/tools/spectral_camera_table.py +++ b/tools/spectral_camera_table.py @@ -91,6 +91,23 @@ def dng_tags(cmf, refl, ssf): return out +def rust_lit(s): + """The body of a Rust string literal: text from a checkout becomes data, never code.""" + out = [] + for ch in s: + if ch == "\\": out.append("\\\\") + elif ch == '"': out.append('\\"') + elif ch == "\t": out.append("\\t") + elif ord(ch) < 0x20 or ord(ch) == 0x7F: out.append(f"\\u{{{ord(ch):x}}}") + else: out.append(ch) + return "".join(out) + + +def rust_comment(s): + """One line of printable text: a raw newline in an external comment would introduce Rust code.""" + return rust_lit(" ".join(str(s).split())) + + def mat(m): return "Mat3([" + ", ".join("[" + ", ".join(f"{v:.6}" for v in row) + "]" for row in m) + "])" @@ -111,12 +128,12 @@ def main(root, out): (cm1, fm1, e1), (cm2, fm2, e2) = dng_tags(cmf, refl, spectral(path, 3)) print(f"{f}: training dE76 A {e1:.3f}, D65 {e2:.3f}", file=sys.stderr) rows.append( - f' Camera {{\n make: "{header["manufacturer"]}",\n model: "{header["model"]}",\n' + f' Camera {{\n make: "{rust_lit(header["manufacturer"])}",\n model: "{rust_lit(header["model"])}",\n' f" color_matrix: [{mat(cm1)}, {mat(cm2)}],\n forward_matrix: [{mat(fm1)}, {mat(fm2)}],\n }}," ) aliases = json.load(open(os.path.join(root, "data/aliases.json")))["data"]["camera"]["model"] alias_rows = [ - f' ("{make}", "{alias}", "{target["model"]}"), // {target.get("comments", "")}' + f' ("{rust_lit(make)}", "{rust_lit(alias)}", "{rust_lit(target["model"])}"), // {rust_comment(target.get("comments", ""))}' for make, models in aliases.items() for alias, target in models.items() ]