diff --git a/crates/clawhdf5-format/src/data_read.rs b/crates/clawhdf5-format/src/data_read.rs index 8aec190..d1766a9 100644 --- a/crates/clawhdf5-format/src/data_read.rs +++ b/crates/clawhdf5-format/src/data_read.rs @@ -32,6 +32,22 @@ fn ensure_len(data: &[u8], offset: usize, needed: usize) -> Result<(), FormatErr Ok(()) } +/// How many bytes to read from a contiguous dataset's storage of +/// `storage_size` bytes (the layout message's size) holding `needed` bytes +/// of elements. libhdf5 reads the elements' bytes from the start of the +/// storage and ignores storage past them (`H5D__contig_check` checks only +/// that the elements fit in the file), so a larger storage reads; one too +/// small to hold the elements is an error. +pub fn contiguous_read_len(storage_size: u64, needed: usize) -> Result { + if storage_size < needed as u64 { + return Err(FormatError::DataSizeMismatch { + expected: needed, + actual: usize::try_from(storage_size).unwrap_or(usize::MAX), + }); + } + Ok(needed) +} + /// Zero-copy read of contiguous raw data, returning a borrowed slice. /// /// For contiguous layouts, returns a direct `&[u8]` slice into `file_data`. @@ -55,13 +71,7 @@ pub fn read_raw_data_zerocopy<'a>( DataLayout::Contiguous { address, size } => { let addr = address.ok_or(FormatError::NoDataAllocated)?; let addr = addr as usize; - let sz = *size as usize; - if sz != expected_size { - return Err(FormatError::DataSizeMismatch { - expected: expected_size, - actual: sz, - }); - } + let sz = contiguous_read_len(*size, expected_size)?; ensure_len(file_data, addr, sz)?; Ok(Some(&file_data[addr..addr + sz])) } @@ -172,13 +182,7 @@ fn read_raw_data_full_impl( DataLayout::Contiguous { address, size } => { let addr = address.ok_or(FormatError::NoDataAllocated)?; let addr = addr as usize; - let sz = *size as usize; - if sz != expected_size { - return Err(FormatError::DataSizeMismatch { - expected: expected_size, - actual: sz, - }); - } + let sz = contiguous_read_len(*size, expected_size)?; ensure_len(file_data, addr, sz)?; let mut out = crate::bulk_alloc::vec_for_bulk(sz); out.extend_from_slice(&file_data[addr..addr + sz]); @@ -2632,6 +2636,36 @@ mod tests { assert_eq!(result.unwrap(), &[1.5f32, 2.5, 3.5]); } + /// libhdf5 reads a contiguous dataset's elements from the start of its + /// storage and ignores storage past them (cve-2024-32623's scalar + /// `/Dset1` has 240 bytes of storage for one 4-byte element). Storage too + /// small for the elements is still an error. + #[test] + fn contiguous_storage_larger_than_the_elements_reads() { + let dt = make_f64_le_type(); + let ds = make_simple_dataspace(&[2]); + let mut file_data = vec![0u8; 64]; + file_data[..8].copy_from_slice(&1.5f64.to_le_bytes()); + file_data[8..16].copy_from_slice(&2.5f64.to_le_bytes()); + file_data[16..24].copy_from_slice(&9.0f64.to_le_bytes()); + let layout = DataLayout::Contiguous { + address: Some(0), + size: 40, + }; + let raw = read_raw_data(&file_data, &layout, &ds, &dt).unwrap(); + assert_eq!(raw, file_data[..16]); + let zc = read_raw_data_zerocopy(&file_data, &layout, &ds, &dt).unwrap(); + assert_eq!(zc, Some(&file_data[..16])); + let small = DataLayout::Contiguous { + address: Some(0), + size: 8, + }; + assert!(matches!( + read_raw_data(&file_data, &small, &ds, &dt), + Err(FormatError::DataSizeMismatch { .. }) + )); + } + #[test] fn zerocopy_size_mismatch() { let dt = make_f64_le_type(); diff --git a/crates/clawhdf5-format/src/dataspace.rs b/crates/clawhdf5-format/src/dataspace.rs index a08ca9d..8eb37e1 100644 --- a/crates/clawhdf5-format/src/dataspace.rs +++ b/crates/clawhdf5-format/src/dataspace.rs @@ -7,6 +7,9 @@ use alloc::vec::Vec; use crate::error::FormatError; +/// Most dimensions a dataspace can have (`H5S_MAX_RANK`). +pub const MAX_RANK: u8 = 32; + /// Type of dataspace. #[derive(Debug, Clone, PartialEq)] pub enum DataspaceType { @@ -67,6 +70,12 @@ impl Dataspace { let version = data[0]; let rank = data[1]; let flags = data[2]; + // H5O__sdspace_decode's checks. + if rank > MAX_RANK { + return Err(FormatError::InvalidDataspace( + "simple dataspace dimensionality is too large", + )); + } let (space_type, header_size) = match version { 1 => { @@ -88,6 +97,11 @@ impl Dataspace { 2 => DataspaceType::Null, _ => return Err(FormatError::InvalidDataspaceType(type_byte)), }; + if st != DataspaceType::Simple && rank > 0 { + return Err(FormatError::InvalidDataspace( + "invalid rank for scalar or NULL dataspace", + )); + } (st, 4usize) } _ => return Err(FormatError::InvalidDataspaceVersion(version)), @@ -107,8 +121,13 @@ impl Dataspace { // Read max dimensions if flags bit 0 is set let max_dimensions = if flags & 0x01 != 0 { let mut max_dims = Vec::with_capacity(rank as usize); - for _ in 0..rank { + for i in 0..rank as usize { let val = read_length(data, pos, length_size)?; + if dimensions[i] > val { + return Err(FormatError::InvalidDataspace( + "dataspace dimension size is greater than its maximum size", + )); + } max_dims.push(val); pos += ls; } @@ -176,7 +195,6 @@ impl Dataspace { match self.space_type { DataspaceType::Null => Ok(0), DataspaceType::Scalar => Ok(1), - DataspaceType::Simple if self.dimensions.is_empty() => Ok(0), DataspaceType::Simple => self .dimensions .iter() @@ -195,18 +213,14 @@ impl Dataspace { match self.space_type { DataspaceType::Null => 0, DataspaceType::Scalar => 1, - DataspaceType::Simple => { - if self.dimensions.is_empty() { - 0 - } else { - // Saturate rather than wrap: a wrapped product could - // under-size a buffer. Size-critical callers use - // `checked_num_elements`. - self.dimensions - .iter() - .fold(1u64, |acc, &d| acc.saturating_mul(d)) - } - } + // A simple dataspace of rank 0 holds one element, as in libhdf5 + // (the product of no dimensions). Saturate rather than wrap: a + // wrapped product could under-size a buffer. Size-critical + // callers use `checked_num_elements`. + DataspaceType::Simple => self + .dimensions + .iter() + .fold(1u64, |acc, &d| acc.saturating_mul(d)), } } } @@ -352,4 +366,44 @@ mod tests { let ds = Dataspace::parse(&data, 8).unwrap(); assert_eq!(ds.max_dimensions, Some(vec![10])); } + + /// A simple dataspace of rank 0 (cve-2020-18494's `/dset1`) holds one + /// element in libhdf5, which h5py reads as shape `()`. It was 0. + #[test] + fn simple_rank_zero_holds_one_element() { + let data = build_v2_dataspace(0, 0, 1, &[], None); + let ds = Dataspace::parse(&data, 8).unwrap(); + assert_eq!(ds.space_type, DataspaceType::Simple); + assert_eq!(ds.num_elements(), 1); + assert_eq!(ds.checked_num_elements().unwrap(), 1); + } + + /// `H5O__sdspace_decode`'s checks. + #[test] + fn refuses_what_libhdf5_refuses() { + let too_many = build_v2_dataspace(33, 0, 1, &[1; 33], None); + assert!(matches!( + Dataspace::parse(&too_many, 8), + Err(FormatError::InvalidDataspace(_)) + )); + let scalar_with_rank = build_v2_dataspace(1, 0, 0, &[4], None); + assert!(matches!( + Dataspace::parse(&scalar_with_rank, 8), + Err(FormatError::InvalidDataspace(_)) + )); + let null_with_rank = build_v2_dataspace(1, 0, 2, &[4], None); + assert!(matches!( + Dataspace::parse(&null_with_rank, 8), + Err(FormatError::InvalidDataspace(_)) + )); + let over_max = build_v1_dataspace(2, 0x01, &[5, 20], Some(&[10, 10])); + assert!(matches!( + Dataspace::parse(&over_max, 8), + Err(FormatError::InvalidDataspace(_)) + )); + // 32 dimensions, and a size equal to the maximum or unlimited, are fine. + assert!(Dataspace::parse(&build_v2_dataspace(32, 0, 1, &[1; 32], None), 8).is_ok()); + let at_max = build_v1_dataspace(2, 0x01, &[10, 20], Some(&[10, u64::MAX])); + assert!(Dataspace::parse(&at_max, 8).is_ok()); + } } diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index d285cbf..b365794 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -226,6 +226,10 @@ pub enum FormatError { /// A link libhdf5 refuses to list: a symbol-table entry with an empty /// name ("invalid link name"). Listing the group fails, as in libhdf5. InvalidLinkName, + /// A dataspace message libhdf5 refuses to decode (the reason is + /// libhdf5's own error text): more than 32 dimensions, a rank on a + /// scalar or null dataspace, a dimension larger than its maximum. + InvalidDataspace(&'static str), } impl fmt::Display for FormatError { @@ -500,6 +504,9 @@ impl fmt::Display for FormatError { FormatError::InvalidLinkName => { write!(f, "invalid link name: a group entry has an empty name") } + FormatError::InvalidDataspace(why) => { + write!(f, "invalid dataspace: {why}") + } } } } diff --git a/crates/clawhdf5/src/mmap_file.rs b/crates/clawhdf5/src/mmap_file.rs index 76d9ea1..59adee9 100644 --- a/crates/clawhdf5/src/mmap_file.rs +++ b/crates/clawhdf5/src/mmap_file.rs @@ -390,13 +390,7 @@ impl<'f> MmapDataset<'f> { match &dl { DataLayout::Contiguous { address, size } => { let addr = address.ok_or(Error::Format(FormatError::NoDataAllocated))?; - let sz = *size as usize; - if sz != expected { - return Err(Error::Format(FormatError::DataSizeMismatch { - expected, - actual: sz, - })); - } + let sz = clawhdf5_format::data_read::contiguous_read_len(*size, expected)?; let data = self.file.hdf5_bytes(); let a = addr as usize; if a + sz > data.len() {