diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 4c9da19..fb17557 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -812,7 +812,7 @@ impl<'f> Dataset<'f> { let base_dir = self.file.base_dir.clone(); let resolver = move |name: &str| -> Option> { let dir = base_dir.as_ref()?; - std::fs::read(dir.join(name)).ok() + std::fs::read(dir.join(sibling_file_name(name)?)).ok() }; return Ok(data_read::read_raw_data_full_with_resolver( self.file.data.as_bytes(), @@ -888,6 +888,23 @@ fn datatype_byte_order(dt: &Datatype) -> DatatypeByteOrder { } } +/// A source-file name taken from inside an HDF5 file, accepted only if it +/// stays within the directory of the file that named it. +/// +/// The name is untrusted input. Joining it blindly lets a crafted file make +/// the reader open any path the process can reach — an absolute path replaces +/// the base directory entirely, and `..` components climb out of it. Only +/// plain relative paths made of normal components are allowed. +fn sibling_file_name(name: &str) -> Option<&std::path::Path> { + use std::path::Component; + let path = std::path::Path::new(name); + let mut components = path.components().peekable(); + components.peek()?; + components + .all(|c| matches!(c, Component::Normal(_) | Component::CurDir)) + .then_some(path) +} + fn find_message( header: &ObjectHeader, msg_type: MessageType, @@ -941,3 +958,24 @@ fn resolve_group_entries( Ok(Vec::new()) } } + +#[cfg(test)] +mod sibling_file_name_tests { + use super::sibling_file_name; + + #[test] + fn only_paths_inside_the_base_directory_are_accepted() { + for ok in ["source.h5", "./source.h5", "sub/dir/source.h5"] { + assert!(sibling_file_name(ok).is_some(), "{ok}"); + } + for bad in [ + "", + "/etc/passwd", + "../secret.h5", + "sub/../../secret.h5", + "sub/../ok.h5", + ] { + assert!(sibling_file_name(bad).is_none(), "{bad}"); + } + } +}