diff --git a/crates/engine/src/commands/essential.rs b/crates/engine/src/commands/essential.rs index 7ffa02d8f..cb6702660 100644 --- a/crates/engine/src/commands/essential.rs +++ b/crates/engine/src/commands/essential.rs @@ -891,6 +891,22 @@ pub fn read_template(bytes: &[u8]) -> std::result::Result<(Value, Project, Vec<( Ok((manifest, project, entries)) } +/// Where a template's `media//` entry is written: `//`. +/// `None` unless `` is all digits and `` is a plain file name (not empty, `.` or +/// `..`, no separators, and on Windows no drive, stream or device name), so every entry lands +/// in an item folder directly inside `media_dir`. +fn template_media_path(media_dir: &std::path::Path, entry: &str) -> Option { + let (item, file) = entry.strip_prefix("media/")?.split_once('/')?; + if item.is_empty() || !item.bytes().all(|b| b.is_ascii_digit()) { + return None; + } + if !super::scripts::is_bare_name(file) || (cfg!(windows) && super::scripts::windows_unsafe_name(file)) { + return None; + } + let out = media_dir.join(item).join(file); + (out.parent().and_then(std::path::Path::parent) == Some(media_dir) && out.file_name() == Some(std::ffi::OsStr::new(file))).then_some(out) +} + fn import_template(s: &mut Session, p: &Value) -> Result { const C: &str = "essential.importTemplate"; let path = str_p(p, "path").ok_or_else(|| bad(C, "missing `path` (.ectemplate)"))?.to_string(); @@ -905,8 +921,7 @@ fn import_template(s: &mut Session, p: &Value) -> Result { let media_dir = dir.join(format!("{stem} Media")); let mut written = std::collections::HashMap::new(); for (k, v) in entries.iter().filter(|(k, _)| k.starts_with("media/")) { - let rel = k.trim_start_matches("media/"); - let out = media_dir.join(rel); + let Some(out) = template_media_path(&media_dir, k) else { continue }; if let Some(parent) = out.parent() { let _ = std::fs::create_dir_all(parent); } diff --git a/crates/engine/src/commands/scripts.rs b/crates/engine/src/commands/scripts.rs index 3de5e6fe8..24049459c 100644 --- a/crates/engine/src/commands/scripts.rs +++ b/crates/engine/src/commands/scripts.rs @@ -132,7 +132,7 @@ fn is_script_name(name: &str) -> bool { } /// One path component on every platform: not empty, `.` or `..`, and without `/`, `\` or NUL. -fn is_bare_name(name: &str) -> bool { +pub(crate) fn is_bare_name(name: &str) -> bool { !name.is_empty() && name != "." && name != ".." && !name.contains(['/', '\\', '\0']) } @@ -140,7 +140,7 @@ fn is_bare_name(name: &str) -> bool { /// stream), ending in `.` or a space (Windows drops them), or a device name, which names a device /// in any folder and with any extension (`con.jsx`): `CON`, `PRN`, `AUX`, `NUL`, `CONIN$`, /// `CONOUT$`, `COM0`–`COM9` and `LPT0`–`LPT9` (also with `¹²³`). -fn windows_unsafe_name(name: &str) -> bool { +pub(crate) fn windows_unsafe_name(name: &str) -> bool { let stem = name.split('.').next().unwrap_or_default().trim_end_matches(' ').to_ascii_uppercase(); let port = |p: &str| { let mut n = stem.strip_prefix(p).unwrap_or_default().chars(); diff --git a/crates/engine/src/tests_essential.rs b/crates/engine/src/tests_essential.rs index 0732020ae..191416b9d 100644 --- a/crates/engine/src/tests_essential.rs +++ b/crates/engine/src/tests_essential.rs @@ -274,6 +274,55 @@ fn template_export_import_round_trip_with_overrides() { assert!(t.execute("essential.importTemplate", json!({"path": dir.join("bad.ectemplate").to_string_lossy()})).is_err()); } +#[test] +fn template_import_writes_media_only_into_its_media_folder() { + let root = out_dir("ectemplate-names"); + let dir = root.join("a").join("b"); + std::fs::create_dir_all(&dir).unwrap(); + let (mut s, card, _solid, _ids) = card(); + let src = dir.join("Src.ectemplate"); + s.execute("essential.exportTemplate", json!({"comp": card.0, "path": src.to_string_lossy()})).unwrap(); + let mut entries = effectcraft_lottie::zip::read_stored(&std::fs::read(&src).unwrap()); + std::fs::remove_file(&src).unwrap(); + let abs = root.join("abs").join("x").to_string_lossy().replace('\\', "/"); + for name in [ + "../x", + "media/../../x", + "media/1/../../../x", + &format!("media/{abs}"), + &format!("media/6{abs}"), + "media/C:\\x", + "media/7/C:\\x", + "media/x/clip.png", + "media/8/sub/clip.png", + "media/2/..", + "media/3/", + "media/4/clip.png", + "media/5/clip.png", + ] { + entries.push((name.to_string(), b"data".to_vec())); + } + let path = dir.join("T.ectemplate"); + std::fs::write(&path, effectcraft_lottie::zip::store(&entries)).unwrap(); + s.execute("essential.importTemplate", json!({"path": path.to_string_lossy()})).unwrap(); + + let media = dir.join("T Media"); + fn files(d: &std::path::Path, out: &mut Vec) { + for e in std::fs::read_dir(d).unwrap().map(|e| e.unwrap().path()) { + if e.is_dir() { files(&e, out) } else { out.push(e) } + } + } + let mut found = vec![]; + files(&root, &mut found); + let in_item = |f: &std::path::Path| f.parent().and_then(std::path::Path::parent) == Some(media.as_path()); + for f in &found { + assert!(f == &path || in_item(f), "written outside an item folder of the media folder: {f:?}"); + } + let mut names: Vec = found.iter().filter(|f| in_item(f)).map(|f| f.strip_prefix(&media).unwrap().to_string_lossy().replace('\\', "/")).collect(); + names.sort(); + assert_eq!(names, ["4/clip.png", "5/clip.png"]); +} + #[test] fn media_replacement_swaps_footage_per_instance() { use std::sync::Arc; diff --git a/crates/lottie/src/zip.rs b/crates/lottie/src/zip.rs index 984a6fcd7..2a11dfa25 100644 --- a/crates/lottie/src/zip.rs +++ b/crates/lottie/src/zip.rs @@ -70,8 +70,16 @@ pub fn store(entries: &[(String, Vec)]) -> Vec { out } +/// Whether an entry name is a relative path inside the archive: not absolute (`/…`, `\…`), no +/// drive (`C:…`) and no `..` component. +fn is_relative_name(name: &str) -> bool { + let drive = name.as_bytes().get(1) == Some(&b':'); + !name.starts_with(['/', '\\']) && !drive && !name.split(['/', '\\']).any(|c| c == "..") +} + /// Read the stored (uncompressed) entries of a ZIP archive written by [`store`] or any other -/// writer that did not compress. Compressed entries are skipped. +/// writer that did not compress. Compressed entries and entries whose names are not relative +/// paths inside the archive are skipped. pub fn read_stored(data: &[u8]) -> Vec<(String, Vec)> { let mut out = vec![]; let mut i = 0usize; @@ -82,8 +90,9 @@ pub fn read_stored(data: &[u8]) -> Vec<(String, Vec)> { let name_start = i + 30; let body = name_start + nlen + xlen; let (Some(name), Some(bytes)) = (data.get(name_start..name_start + nlen), data.get(body..body + size)) else { break }; - if method == 0 { - out.push((String::from_utf8_lossy(name).to_string(), bytes.to_vec())); + let name = String::from_utf8_lossy(name).to_string(); + if method == 0 && is_relative_name(&name) { + out.push((name, bytes.to_vec())); } i = body + size; } @@ -101,4 +110,12 @@ mod tests { assert_eq!(back[1].0, "b/c.txt"); assert_eq!(back[1].1, b"hello"); } + + #[test] + fn entries_outside_the_archive_root_are_skipped() { + let names = ["../up.txt", "a/../../up.txt", "/abs/x", "\\abs\\x", "C:\\x", "c:x", "a\\..\\x", "ok/x.txt", "ok/..x"]; + let z = super::store(&names.iter().map(|n| (n.to_string(), b"x".to_vec())).collect::>()); + let back: Vec = super::read_stored(&z).into_iter().map(|(n, _)| n).collect(); + assert_eq!(back, ["ok/x.txt", "ok/..x"]); + } }