Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/sentinel.md
Original file line number Diff line number Diff line change
@@ -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.
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ members = [
"lib",
"pna",
"fuzz",
"xtask",
"xtask", "test_time",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test_time member appears to be a temporary test or scratchpad crate that was accidentally included in this pull request. It is not mentioned in the PR description and seems unrelated to the hardening logic. It should be removed from the workspace members.

Suggested change
"xtask", "test_time",
"xtask",

]

# The profile that 'dist' will build with
Expand Down
16 changes: 11 additions & 5 deletions cli/src/command/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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()?;
Expand Down
192 changes: 152 additions & 40 deletions lib/src/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,13 @@ fn chunks_write_in<W: Write>(
chunks: impl Iterator<Item = impl Chunk>,
writer: &mut W,
) -> io::Result<usize> {
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)
}
Expand Down Expand Up @@ -318,18 +322,38 @@ where
{
#[inline]
fn chunks_write_in<W: Write>(&self, writer: &mut W) -> io::Result<usize> {
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)
}
}
Expand Down Expand Up @@ -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![];
Expand All @@ -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())),
Expand Down Expand Up @@ -714,7 +746,7 @@ where
{
#[inline]
fn chunks_write_in<W: Write>(&self, writer: &mut W) -> io::Result<usize> {
let mut total = 0;
let mut total: usize = 0;

let Metadata {
raw_file_size,
Expand All @@ -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)
}
}
Expand Down Expand Up @@ -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
Comment on lines +1268 to +1269
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of let_chains (the && operator in an if let expression) is an unstable Rust feature (see RFC 2497). Unless this project explicitly targets a nightly toolchain and has the feature enabled via #![feature(let_chains)], this will cause a compilation error on stable Rust. It is recommended to use a stable alternative like .filter() or nested if statements.

Suggested change
if let Some(next_total_size) = total_size.checked_add(chunk.bytes_len())
&& next_total_size <= max_bytes_len
if let Some(next_total_size) = total_size.checked_add(chunk.bytes_len()).filter(|&s| s <= 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 {
Expand All @@ -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)));
Expand Down
15 changes: 13 additions & 2 deletions lib/src/entry/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -408,7 +414,12 @@ impl Write for EntryBuilder {
#[inline]
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
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())
}
Expand Down
4 changes: 4 additions & 0 deletions test_time.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
fn main() {
let _ = time::Duration::seconds(i64::MAX);
println!("Safe!");
}
Comment on lines +1 to +4
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This file, along with the test_time directory, appears to be a leftover from local experimentation and is not part of the project's core logic or test suite. Please remove these files before merging.

7 changes: 7 additions & 0 deletions test_time/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "test_time"
version = "0.1.0"
edition = "2024"

[dependencies]
time = "0.3.47"
Loading
Loading