diff --git a/crates/engine/src/file_access.rs b/crates/engine/src/file_access.rs index 81a6761bd..69edeaca1 100644 --- a/crates/engine/src/file_access.rs +++ b/crates/engine/src/file_access.rs @@ -10,8 +10,9 @@ //! folder, the rest of the path being plain names (`..` below a missing folder is refused, and so //! is a link that leads nowhere). //! - The result must lie inside one of the roots of its kind, compared by whole components -//! (`/work2` isn't inside `/work`), without regard to case on Windows. Two roots don't make their -//! common parent one. +//! (`/work2` isn't inside `/work`), in the case the file system stores (so a case-variant name +//! reaches the same folder on an ordinary Windows volume, and a distinct sibling in a +//! case-sensitive one stays outside). Two roots don't make their common parent one. //! - Read and write access are separate: a root given alone grants only its own kind of access. //! //! Roots are in force on a thread inside [`confine`], and in the whole process after @@ -255,24 +256,13 @@ fn resolve(path: &Path) -> Result { } } -/// Is `path` (resolved) `root` or inside it, component by component (without regard to case on -/// Windows, whose file systems ignore it)? +/// Is `path` (resolved) `root` or inside it, component by component? Both are canonical, so each +/// component carries the case the file system stores: a path that differs only in case names the +/// same folder on an ordinary Windows volume (and resolves to the same components), and a distinct +/// sibling in a case-sensitive folder (which must not pass as the root). fn inside(path: &Path, root: &Path) -> bool { let mut path = path.components(); - root.components().all(|r| path.next().is_some_and(|p| same_component(p, r))) -} - -fn same_component(a: Component, b: Component) -> bool { - if a == b { - return true; - } - if !cfg!(windows) { - return false; - } - match (a.as_os_str().to_str(), b.as_os_str().to_str()) { - (Some(a), Some(b)) => a.to_lowercase() == b.to_lowercase(), - _ => false, - } + root.components().all(|r| path.next() == Some(r)) } /// A Windows device name (`CON`, `NUL`, `COM1`… also with an extension), which names no file. @@ -571,6 +561,14 @@ mod tests { let _ = std::fs::remove_dir_all(base); } + #[test] + fn inside_compares_components_exactly() { + assert!(inside(Path::new("/work/in/a"), Path::new("/work/in"))); + assert!(inside(Path::new("/work/in"), Path::new("/work/in"))); + assert!(!inside(Path::new("/work/In/a"), Path::new("/work/in")), "a case-variant sibling is another folder"); + assert!(!inside(Path::new("/work/in2/a"), Path::new("/work/in"))); + } + #[test] fn device_names() { for d in ["CON", "nul", "Com1", "LPT9.txt", "aux.designcraft", "NUL "] { diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index eb6d99886..25256cbc5 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -441,6 +441,49 @@ fn files(dir: &Path, uid: u64) -> (PathBuf, PathBuf) { (dir.join(format!("{uid}.designcraft")), dir.join(format!("{uid}.json"))) } +/// The bytes of the recovery entry file at `path`, which must be a regular file: the folder was +/// checked, and a link planted among its entries would lead the read anywhere. +fn read_entry_file(path: &Path) -> std::io::Result> { + use std::io::Read; + let refuse = || std::io::Error::other(format!("{}: a recovery entry must be a regular file", path.display())); + let before = std::fs::symlink_metadata(path)?; + #[cfg(not(target_arch = "wasm32"))] + if is_link_like(&before) { + return Err(refuse()); + } + if !before.file_type().is_file() { + return Err(refuse()); + } + #[cfg(windows)] + let mut file = { + use std::os::windows::fs::OpenOptionsExt; + // A reparse point swapped in after the check is opened as itself, not followed. + const FILE_FLAG_OPEN_REPARSE_POINT: u32 = 0x0020_0000; + std::fs::OpenOptions::new().read(true).custom_flags(FILE_FLAG_OPEN_REPARSE_POINT).open(path)? + }; + #[cfg(not(windows))] + let mut file = std::fs::File::open(path)?; + let opened = file.metadata()?; + #[cfg(windows)] + if is_link_like(&opened) { + return Err(refuse()); + } + if !opened.file_type().is_file() { + return Err(refuse()); + } + // The name was checked, then opened: make sure what was opened is what was checked. + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + if opened.dev() != before.dev() || opened.ino() != before.ino() { + return Err(refuse()); + } + } + let mut bytes = Vec::new(); + file.read_to_end(&mut bytes)?; + Ok(bytes) +} + /// Remove a document's recovery entry (it was saved or closed). In a folder that is not this /// run's own, only an entry this run wrote or recovered is removed. pub fn discard(dir: &Path, uid: u64) { @@ -761,9 +804,9 @@ pub fn list(dir: &Path) -> Vec<(u64, Value)> { } let Some(uid) = p.file_stem().and_then(std::ffi::OsStr::to_str).and_then(|s| s.parse::().ok()) else { continue }; // Metadata is a file on disk: anything but an object (corrupt, hand-edited) counts as none. - let meta = std::fs::read_to_string(p.with_extension("json")) + let meta = read_entry_file(&p.with_extension("json")) .ok() - .and_then(|t| serde_json::from_str::(&t).ok()) + .and_then(|t| serde_json::from_slice::(&t).ok()) .filter(Value::is_object) .unwrap_or(json!({})); out.push((uid, meta)); @@ -810,7 +853,7 @@ fn open_with_publisher(s: &mut Session, dir: &Path, mut publish: impl FnMut(&Pat #[cfg(not(target_arch = "wasm32"))] validate_recovery_dir(dir)?; let (doc, _) = files(dir, uid); - let Ok(bytes) = std::fs::read(&doc) else { continue }; + let Ok(bytes) = read_entry_file(&doc) else { continue }; let mut d = match crate::cmd::from_bytes(&bytes) { Ok(d) => d, Err(_) => continue, diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index 395e8385b..c23a2c831 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -183,6 +183,37 @@ fn symlink_recovery_directory_is_rejected() { assert!(std::fs::symlink_metadata(&link).unwrap().file_type().is_symlink()); } +/// A link planted among a checked folder's entries doesn't get its target read: not as metadata +/// (listed as is) and not as a document (recovered). +#[cfg(unix)] +#[test] +fn linked_recovery_entries_are_not_read() { + use std::os::unix::fs::symlink; + + let root = tempfile::tempdir().unwrap(); + let (dir, elsewhere) = (root.path().join("recovery"), root.path().join("elsewhere")); + std::fs::create_dir(&dir).unwrap(); + std::fs::create_dir(&elsewhere).unwrap(); + // A real document whose metadata is a link to a private file. + assert_eq!(save(&unsaved("real"), &dir).unwrap(), 1); + let uid = list(&dir)[0].0; + std::fs::write(elsewhere.join("secret.json"), br#"{"path": "/home/someone/secret.designcraft"}"#).unwrap(); + std::fs::remove_file(dir.join(format!("{uid}.json"))).unwrap(); + symlink(elsewhere.join("secret.json"), dir.join(format!("{uid}.json"))).unwrap(); + // A document entry that is a link to a document elsewhere. + assert_eq!(save(&unsaved("elsewhere"), &elsewhere).unwrap(), 1); + let linked = list(&elsewhere)[0].0; + symlink(elsewhere.join(format!("{linked}.designcraft")), dir.join("9000000.designcraft")).unwrap(); + + let entries = list(&dir); + assert_eq!(entries.iter().map(|e| e.0).collect::>(), [uid, 9_000_000]); + assert_eq!(entries[0].1, json!({}), "the linked metadata counts as none"); + let mut s = Session::new(); + assert_eq!(open(&mut s, &dir).unwrap().len(), 1); + assert_eq!(texts(&s), ["real"]); + assert!(std::fs::symlink_metadata(dir.join("9000000.designcraft")).unwrap().file_type().is_symlink(), "left as it was"); +} + #[test] fn a_run_has_a_locked_folder_of_its_own() { let root = tempfile::tempdir().unwrap();