collective
Browse / github.com/storytold/effectcraft / patch
engine: a template's footage entry can't name a file outside the folder (#2123)
[Audit F-03] Windows path traversal in template footage extraction **Severity:** High · **Audit id:** F-03 · **Commit:** `8f02f52a` **Evidence.** `crates/engine/src/templates.rs:739-745`: ```rust let file = base_name(k); // splits on '/' and '\\' let item = k.trim_start_matches("media/").split('/').next().unwrap_or("0"); // splits on '/' only let name = if used.insert(file.clone()) { file } else { format!("{item}-{file}") }; let out = format!("{dir}/{name}"); s.services.store_file(&out, v) ``` **Trigger.** Two entries with the same base name. The second is `media/..\..\..\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Startup\x/evil.
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- Anubhav-Rai (imported from pull request #2523, open)
- Base
bc4b48f67deef2458e9dd3e3b0e592d5f75768a1- Size
- +81 −3 in 1 file
- Review
- pending
- Shared
- 2026-10-28
- Id
bcbe4cdc970fa488b0fe12a7
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/home/templates.rs
@@ -645,6 +645,21 @@ fn base_name(path: &str) -> String { path.rsplit(['/', '\\']).next().filter(|n| !n.is_empty()).unwrap_or("footage").to_string() } +/// A template entry's name, made safe to write inside a folder: a template is a file like any+/// other and can name its entries anything. Nothing a platform reads as a separator, a drive or+/// the folder above survives, and a name that leaves nothing usable becomes `footage` (#2123).+fn safe_file_name(name: &str) -> String {+ let name: String = name.chars().map(|c| if matches!(c, '/' | '\\' | ':' | '\0') { '_' } else { c }).collect();+ // `.` and `..`, and names of dots and spaces alone (Windows drops the trailing ones).+ if name.trim_matches(['.', ' ']).is_empty() { "footage".to_string() } else { name }+}++/// What two entry names must differ by to be sure of two files: a filesystem may ignore case, and+/// Win32 ignores trailing dots and spaces.+fn same_file_key(name: &str) -> String {+ name.trim_end_matches(['.', ' ']).to_lowercase()+}+ /// What [`embed_footage`] did. #[derive(Default)] struct Embedded {@@ -738,9 +753,21 @@ fn extract_footage(s: &Session, proj: &mut Project, entries: &[(String, Vec<u8>) let mut used = std::collections::HashSet::new(); for (k, v) in entries.iter().filter(|(k, _)| k.starts_with("media/")) { // `media/<item>/<file>` → one folder; a name another item took gets the item's id.- let file = base_name(k);- let item = k.trim_start_matches("media/").split('/').next().unwrap_or("0");- let name = if used.insert(file.clone()) { file } else { format!("{item}-{file}") };+ let file = safe_file_name(&base_name(k));+ // The item's id, and only that: it is written as one ([`embed_footage`]), but a template+ // from elsewhere can hold anything there, and `..\` in it walks out of `dir` on Windows.+ let item = k.trim_start_matches("media/").split('/').next().filter(|i| i.bytes().all(|b| b.is_ascii_digit())).unwrap_or("0");+ // A name another entry has taken gets the item's id, and a number if that is taken too:+ // the names an entry can ask for are not distinct, and two files must not become one.+ // They are compared the way a filesystem might: `CLIP.png` is `clip.png` on a+ // case-insensitive volume (Windows and macOS as they come), and Win32 drops a trailing+ // dot or space, so `a.png.` is `a.png` there. The name written stays as it is.+ let mut name = file.clone();+ let mut n = 0usize;+ while !used.insert(same_file_key(&name)) {+ n += 1;+ name = if n == 1 { format!("{item}-{file}") } else { format!("{item}-{n}-{file}") };+ } let out = format!("{dir}/{name}"); s.services.store_file(&out, v).map_err(|e| EngineError::Other(format!("cannot write {out}: {e}")))?; if let Some(imp) = &s.importer {@@ -970,6 +997,57 @@ mod tests { assert_eq!(l.iter().find(|t| t.id == "lower-third").unwrap().controls, ["Name", "Title", "Bar Color"]); } + /// #2123: a template holds its footage as `media/<item id>/<file>` entries, but a template+ /// from anywhere else can name them anything — and `..\\`, which only this platform reads as+ /// a plain character, is a separator on Windows, so a second entry with a base name already+ /// taken wrote `<dir>/..\\..\\..\\Startup\\x-evil.bat`. Whatever an entry is called, the file+ /// lands in the footage folder.+ #[test]+ fn extracted_footage_stays_in_its_folder() {+ let s = Session::default();+ let mut proj = effectcraft_project::Project::default();+ let dir = std::env::temp_dir().join(format!("ec-templates-media-{}", std::process::id()));+ let _ = std::fs::remove_dir_all(&dir);+ let ds = dir.to_string_lossy().into_owned();+ let entry = |k: &str| (k.to_string(), b"x".to_vec());+ let entries = [+ entry("media/0/clip.mp4"),+ // The base name is taken, so this one's "item id" is used — and it is not one.+ entry("media/..\\..\\..\\Startup\\x/clip.mp4"),+ entry("media/1/C:evil.bat"),+ // Three that collide once the first is made safe: each still gets its own file.+ entry("media/1/a:b.png"),+ entry("media/2/a_b.png"),+ entry("media/3/2-a_b.png"),+ // The same file on a case-insensitive volume, and on Windows, which drops the dot.+ entry("media/4/clip.png"),+ entry("media/5/CLIP.png"),+ entry("media/6/a.png"),+ entry("media/7/a.png."),+ ];+ let written = extract_footage(&s, &mut proj, &entries, &ds).unwrap();+ assert_eq!(written.len(), entries.len());+ for w in &written {+ let name = w.strip_prefix(&format!("{ds}/")).unwrap_or_else(|| panic!("{w} is outside {ds}"));+ assert!(!name.contains(['/', '\\', ':']), "{w}");+ assert!(!name.trim_matches(['.', ' ']).is_empty(), "{w}");+ }+ // One file each, all of them in the folder, none overwritten by the next.+ assert_eq!(written.iter().collect::<std::collections::BTreeSet<_>>().len(), entries.len(), "{written:?}");+ assert_eq!(std::fs::read_dir(&dir).map(|d| d.count()).unwrap_or(0), entries.len());+ let _ = std::fs::remove_dir_all(&dir);+ // And the names that would leave nothing to write.+ for n in ["..", ".", " .. ", "...", ""] {+ assert_eq!(safe_file_name(n), "footage", "{n:?}");+ }+ assert_eq!(safe_file_name("clip.mp4"), "clip.mp4");+ assert_eq!(safe_file_name(".hidden.mp4"), ".hidden.mp4");+ assert_eq!(safe_file_name("a/b\\c:d"), "a_b_c_d");+ // Names a filesystem may not tell apart (only the case half of this shows on macOS).+ assert_eq!(same_file_key("A.PNG. "), same_file_key("a.png"));+ assert_ne!(same_file_key("a.png"), same_file_key("b.png"));+ }+ #[test] fn create_opens_an_untitled_copy() { let mut s = Session::default();