diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 000000000..f87475fd3 --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,4 @@ +## 2025-05-15 - Integer overflows in archive entry size and length calculations +**Vulnerability:** Potential integer overflows in entry parsing (`NormalEntry::try_from`), writing (`chunks_write_in`), and CLI-side archive splitting logic. +**Learning:** Untrusted data from archive headers (like chunk lengths) was being added using wrapping or saturating arithmetic (`+=`, `.sum()`), which could lead to inconsistent state or logic errors if values exceeded `usize::MAX`. +**Prevention:** Always use `checked_add` or `checked_mul` when accumulating lengths or sizes derived from untrusted archive input, and propagate an `io::Error` on overflow to fail securely. diff --git a/Cargo.lock b/Cargo.lock index f67cdb9a5..5a693af3e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2574,6 +2574,13 @@ version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8f50febec83f5ee1df3015341d8bd429f2d1cc62bcba7ea2076759d315084683" +[[package]] +name = "test_time" +version = "0.1.0" +dependencies = [ + "time", +] + [[package]] name = "testing_table" version = "0.3.0" diff --git a/Cargo.toml b/Cargo.toml index 79d0c4e8c..101ca36b3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,7 +5,7 @@ members = [ "lib", "pna", "fuzz", - "xtask", + "xtask", "test_time", ] # The profile that 'dist' will build with diff --git a/cli/src/command/core.rs b/cli/src/command/core.rs index 28e0add9b..e6783f0c1 100644 --- a/cli/src/command/core.rs +++ b/cli/src/command/core.rs @@ -1812,12 +1812,12 @@ where ) .into()); } - let mut part_num = 1; + let mut part_num: usize = 1; let mut writer = Archive::write_header(initial_writer)?; // NOTE: max_file_size - (PNA_HEADER + AHED + ANXT + AEND) let max_file_size = max_file_size - SPLIT_ARCHIVE_OVERHEAD_BYTES; - let mut written_entry_size = 0; + let mut written_entry_size: usize = 0; for entry in entries { let p = EntryPart::from(entry?); let parts = split_to_parts( @@ -1826,13 +1826,19 @@ where max_file_size, )?; for part in parts { - if written_entry_size + part.bytes_len() > max_file_size { - part_num += 1; + if written_entry_size.saturating_add(part.bytes_len()) > max_file_size { + part_num = part_num.checked_add(1).ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "part number overflow") + })?; let file = get_next_writer(part_num)?; writer = writer.split_to_next_archive(file)?; written_entry_size = 0; } - written_entry_size += writer.add_entry_part(part)?; + written_entry_size = written_entry_size + .checked_add(writer.add_entry_part(part)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "written entry size overflow") + })?; } } writer.finalize()?; diff --git a/lib/src/entry.rs b/lib/src/entry.rs index e36f24aa3..f74170eca 100644 --- a/lib/src/entry.rs +++ b/lib/src/entry.rs @@ -54,9 +54,13 @@ fn chunks_write_in( chunks: impl Iterator, writer: &mut W, ) -> io::Result { - let mut total = 0; + let mut total: usize = 0; for chunk in chunks { - total += chunk.write_chunk_in(writer)?; + total = total + .checked_add(chunk.write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } Ok(total) } @@ -318,18 +322,38 @@ where { #[inline] fn chunks_write_in(&self, writer: &mut W) -> io::Result { - let mut total = 0; - total += (ChunkType::SHED, self.header.to_bytes()).write_chunk_in(writer)?; + let mut total: usize = 0; + total = total + .checked_add((ChunkType::SHED, self.header.to_bytes()).write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; for extra_chunk in &self.extra { - total += extra_chunk.write_chunk_in(writer)?; + total = total + .checked_add(extra_chunk.write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } if let Some(phsf) = &self.phsf { - total += (ChunkType::PHSF, phsf.as_bytes()).write_chunk_in(writer)?; + total = total + .checked_add((ChunkType::PHSF, phsf.as_bytes()).write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } for data in &self.data { - total += (ChunkType::SDAT, data).write_chunk_in(writer)?; + total = total + .checked_add((ChunkType::SDAT, data).write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } - total += (ChunkType::SEND, []).write_chunk_in(writer)?; + total = total + .checked_add((ChunkType::SEND, []).write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; Ok(total) } } @@ -633,7 +657,7 @@ where ), )); } - let mut compressed_size = 0; + let mut compressed_size: usize = 0; let mut extra = vec![]; let mut data = vec![]; let mut xattrs = vec![]; @@ -657,7 +681,15 @@ where ); } ChunkType::FDAT => { - compressed_size += chunk.data().len(); + compressed_size = + compressed_size + .checked_add(chunk.data().len()) + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidData, + "compressed size overflow", + ) + })?; data.push(chunk.data); } ChunkType::fSIZ => size = Some(u128_from_be_bytes_last(chunk.data())), @@ -714,7 +746,7 @@ where { #[inline] fn chunks_write_in(&self, writer: &mut W) -> io::Result { - let mut total = 0; + let mut total: usize = 0; let Metadata { raw_file_size, @@ -726,55 +758,129 @@ where link_target_type, } = &self.metadata; - total += (ChunkType::FHED, self.header.to_bytes()).write_chunk_in(writer)?; + total = total + .checked_add((ChunkType::FHED, self.header.to_bytes()).write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; for ex in &self.extra { - total += ex.write_chunk_in(writer)?; + total = total + .checked_add(ex.write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } if let Some(raw_file_size) = raw_file_size { - total += ( - ChunkType::fSIZ, - skip_while(&raw_file_size.to_be_bytes(), |i| *i == 0), - ) - .write_chunk_in(writer)?; + total = total + .checked_add( + ( + ChunkType::fSIZ, + skip_while(&raw_file_size.to_be_bytes(), |i| *i == 0), + ) + .write_chunk_in(writer)?, + ) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } if let Some(p) = &self.phsf { - total += (ChunkType::PHSF, p.as_bytes()).write_chunk_in(writer)?; + total = total + .checked_add((ChunkType::PHSF, p.as_bytes()).write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } for data_chunk in &self.data { - total += (ChunkType::FDAT, data_chunk).write_chunk_in(writer)?; + total = total + .checked_add((ChunkType::FDAT, data_chunk).write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } if let Some(c) = created { - total += (ChunkType::cTIM, c.whole_seconds().to_be_bytes()).write_chunk_in(writer)?; + total = total + .checked_add( + (ChunkType::cTIM, c.whole_seconds().to_be_bytes()).write_chunk_in(writer)?, + ) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; if c.subsec_nanoseconds() != 0 { - total += (ChunkType::cTNS, c.subsec_nanoseconds().to_be_bytes()) - .write_chunk_in(writer)?; + total = total + .checked_add( + (ChunkType::cTNS, c.subsec_nanoseconds().to_be_bytes()) + .write_chunk_in(writer)?, + ) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } } if let Some(d) = modified { - total += (ChunkType::mTIM, d.whole_seconds().to_be_bytes()).write_chunk_in(writer)?; + total = total + .checked_add( + (ChunkType::mTIM, d.whole_seconds().to_be_bytes()).write_chunk_in(writer)?, + ) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; if d.subsec_nanoseconds() != 0 { - total += (ChunkType::mTNS, d.subsec_nanoseconds().to_be_bytes()) - .write_chunk_in(writer)?; + total = total + .checked_add( + (ChunkType::mTNS, d.subsec_nanoseconds().to_be_bytes()) + .write_chunk_in(writer)?, + ) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } } if let Some(a) = accessed { - total += (ChunkType::aTIM, a.whole_seconds().to_be_bytes()).write_chunk_in(writer)?; + total = total + .checked_add( + (ChunkType::aTIM, a.whole_seconds().to_be_bytes()).write_chunk_in(writer)?, + ) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; if a.subsec_nanoseconds() != 0 { - total += (ChunkType::aTNS, a.subsec_nanoseconds().to_be_bytes()) - .write_chunk_in(writer)?; + total = total + .checked_add( + (ChunkType::aTNS, a.subsec_nanoseconds().to_be_bytes()) + .write_chunk_in(writer)?, + ) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } } if let Some(p) = permission { - total += (ChunkType::fPRM, p.to_bytes()).write_chunk_in(writer)?; + total = total + .checked_add((ChunkType::fPRM, p.to_bytes()).write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } if let Some(ltp) = link_target_type { - total += (ChunkType::fLTP, ltp.to_bytes()).write_chunk_in(writer)?; + total = total + .checked_add((ChunkType::fLTP, ltp.to_bytes()).write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } for xattr in &self.xattrs { - total += (ChunkType::xATR, xattr.to_bytes()).write_chunk_in(writer)?; + total = total + .checked_add((ChunkType::xATR, xattr.to_bytes()).write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; } - total += (ChunkType::FEND, []).write_chunk_in(writer)?; + total = total + .checked_add((ChunkType::FEND, []).write_chunk_in(writer)?) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "total write size overflow") + })?; Ok(total) } } @@ -1156,13 +1262,21 @@ impl EntryPart<&[u8]> { } let mut remaining = VecDeque::from(self.0); let mut first = Vec::new(); - let mut total_size = 0; + let mut total_size: usize = 0; while let Some(chunk) = remaining.pop_front() { // NOTE: If over max size, restore to the remaining chunk - if max_bytes_len < total_size + chunk.bytes_len() { - if chunk.is_stream_chunk() && total_size + MIN_CHUNK_BYTES_SIZE < max_bytes_len { - let available_bytes_len = max_bytes_len - total_size; - let chunk_split_index = available_bytes_len - MIN_CHUNK_BYTES_SIZE; + if let Some(next_total_size) = total_size.checked_add(chunk.bytes_len()) + && next_total_size <= max_bytes_len + { + total_size = next_total_size; + first.push(chunk); + } else { + if chunk.is_stream_chunk() + && total_size.saturating_add(MIN_CHUNK_BYTES_SIZE) < max_bytes_len + { + let available_bytes_len = max_bytes_len.saturating_sub(total_size); + let chunk_split_index = + available_bytes_len.saturating_sub(MIN_CHUNK_BYTES_SIZE); let (x, y) = chunk_data_split(chunk.ty, chunk.data, chunk_split_index); first.push(x); if let Some(y) = y { @@ -1173,8 +1287,6 @@ impl EntryPart<&[u8]> { } break; } - total_size += chunk.bytes_len(); - first.push(chunk); } if first.is_empty() { return Err(Self(Vec::from(remaining))); diff --git a/lib/src/entry/builder.rs b/lib/src/entry/builder.rs index 891928965..565b7af24 100644 --- a/lib/src/entry/builder.rs +++ b/lib/src/entry/builder.rs @@ -381,12 +381,18 @@ impl EntryBuilder { if let Some(iv) = self.iv { data.insert(0, iv); } + let mut compressed_size: usize = 0; + for d in &data { + compressed_size = compressed_size.checked_add(d.len()).ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "compressed size overflow") + })?; + } let metadata = Metadata { raw_file_size: match (self.store_file_size, self.header.data_kind) { (true, DataKind::File) => Some(self.file_size), _ => None, }, - compressed_size: data.iter().map(|d| d.len()).sum(), + compressed_size, created: self.created, modified: self.last_modified, accessed: self.accessed, @@ -408,7 +414,12 @@ impl Write for EntryBuilder { #[inline] fn write(&mut self, buf: &[u8]) -> io::Result { if let Some(w) = &mut self.data { - return w.write(buf).inspect(|len| self.file_size += *len as u128); + return w.write(buf).and_then(|len| { + self.file_size = self.file_size.checked_add(len as u128).ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "file size overflow") + })?; + Ok(len) + }); } Ok(buf.len()) } diff --git a/test_time.rs b/test_time.rs new file mode 100644 index 000000000..4b3f13a7a --- /dev/null +++ b/test_time.rs @@ -0,0 +1,4 @@ +fn main() { + let _ = time::Duration::seconds(i64::MAX); + println!("Safe!"); +} diff --git a/test_time/Cargo.toml b/test_time/Cargo.toml new file mode 100644 index 000000000..feaf722fd --- /dev/null +++ b/test_time/Cargo.toml @@ -0,0 +1,7 @@ +[package] +name = "test_time" +version = "0.1.0" +edition = "2024" + +[dependencies] +time = "0.3.47" diff --git a/test_time/src/main.rs b/test_time/src/main.rs new file mode 100644 index 000000000..4b3f13a7a --- /dev/null +++ b/test_time/src/main.rs @@ -0,0 +1,4 @@ +fn main() { + let _ = time::Duration::seconds(i64::MAX); + println!("Safe!"); +}