security(clawhdf5): confine virtual-dataset source files to the base directory
The VDS resolver joined the source file name stored in the HDF5 file straight onto the opened file's directory. That name is untrusted: an absolute path replaces the base directory outright and `..` components climb out of it, so a crafted file could make the reader open any path the process can reach. Only plain relative paths of normal components are accepted now; anything else resolves to "source not found". Co-Authored-By: Claude Fable 5.1 <[email protected]>
This commit is contained in:
co-authored by
Claude Fable 5.1
parent
e38c8133bc
commit
8f62cb44e0
@@ -812,7 +812,7 @@ impl<'f> Dataset<'f> {
|
||||
let base_dir = self.file.base_dir.clone();
|
||||
let resolver = move |name: &str| -> Option<Vec<u8>> {
|
||||
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}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user