🛡️ Sentinel: Harden link target extraction against memory exhaustion#3069
🛡️ Sentinel: Harden link target extraction against memory exhaustion#3069ChanTsune wants to merge 2 commits into
Conversation
Define `MAX_LINK_TARGET_SIZE` (64 KiB) and enforce this limit when reading symbolic and hard link targets from archives. This prevents a potential denial-of-service (DoS) attack where a malicious archive contains excessively large link targets that exhaust available memory during extraction, listing, or comparison. Changes: - Added `MAX_LINK_TARGET_SIZE` constant in `cli/src/command/core.rs`. - Enforced size limit in `cli/src/command/extract.rs`, `cli/src/command/list.rs`, and `cli/src/command/diff.rs`. - Added integration tests in `cli/tests/cli/extract/link_limit.rs` to verify enforcement. - Updated `.jules/sentinel.md` with security learnings. Co-authored-by: ChanTsune <41658782+ChanTsune@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR hardens the ChangesLink target size limit enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/tests/cli/extract/link_limit.rs (1)
16-21: ⚡ Quick winAdd an exact-limit acceptance test to lock in the boundary.
You already test
MAX + 1; please also test exactly64 * 1024bytes succeeds (for at least one link type) to prevent off-by-one regressions. Using a localconst MAX_LINK_TARGET_SIZEin this test file would also remove duplicated magic numbers.Proposed test tweak
+const MAX_LINK_TARGET_SIZE: usize = 64 * 1024; + #[test] fn extract_fails_on_huge_symlink_target() { @@ - let huge_target = "a".repeat(65536 + 1); + let huge_target = "a".repeat(MAX_LINK_TARGET_SIZE + 1); @@ } + +#[test] +fn extract_succeeds_on_max_symlink_target() { + setup(); + let base_dir = "extract_succeeds_on_max_symlink_target"; + fs::create_dir_all(format!("{base_dir}/out")).unwrap(); + + let archive_path = format!("{base_dir}/archive.pna"); + let archive_file = fs::File::create(&archive_path).unwrap(); + let mut archive = pna::Archive::write_header(archive_file).unwrap(); + + let max_target = "a".repeat(MAX_LINK_TARGET_SIZE); + let entry = pna::EntryBuilder::new_symlink( + pna::EntryName::from("max_link"), + pna::EntryReference::from_utf8_preserve_root(&max_target), + ) + .unwrap() + .build() + .unwrap(); + archive.add_entry(entry).unwrap(); + archive.finalize().unwrap(); + + let output = cargo_bin_cmd!("pna") + .arg("x") + .arg(&archive_path) + .arg("--out-dir") + .arg(format!("{base_dir}/out")) + .arg("--overwrite") + .output() + .unwrap(); + + assert!(output.status.success()); +}Also applies to: 55-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/tests/cli/extract/link_limit.rs` around lines 16 - 21, Add an exact-limit acceptance test by defining a local const MAX_LINK_TARGET_SIZE = 64 * 1024 in cli/tests/cli/extract/link_limit.rs and use it to create a target with "a".repeat(MAX_LINK_TARGET_SIZE) (in addition to the existing MAX+1 case). Construct the entry with pna::EntryName::from("huge_link_exact") and pna::EntryReference::from_utf8_preserve_root(&huge_target) and assert that creating/accepting this link succeeds for at least one link type used in the file; this removes the duplicated magic number and locks the 64 KiB boundary to avoid off-by-one regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/tests/cli/extract/link_limit.rs`:
- Around line 16-21: Add an exact-limit acceptance test by defining a local
const MAX_LINK_TARGET_SIZE = 64 * 1024 in cli/tests/cli/extract/link_limit.rs
and use it to create a target with "a".repeat(MAX_LINK_TARGET_SIZE) (in addition
to the existing MAX+1 case). Construct the entry with
pna::EntryName::from("huge_link_exact") and
pna::EntryReference::from_utf8_preserve_root(&huge_target) and assert that
creating/accepting this link succeeds for at least one link type used in the
file; this removes the duplicated magic number and locks the 64 KiB boundary to
avoid off-by-one regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72c50548-b05a-4037-94b2-14ee3c988f99
📒 Files selected for processing (7)
.jules/sentinel.mdcli/src/command/core.rscli/src/command/diff.rscli/src/command/extract.rscli/src/command/list.rscli/tests/cli/extract.rscli/tests/cli/extract/link_limit.rs
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a 64 KiB limit on link target sizes across the diff, extract, and list commands to mitigate potential Denial of Service (DoS) attacks via memory exhaustion. The changes include documentation in a sentinel file and new integration tests to verify the enforcement of this limit. The reviewer recommends refactoring the duplicated size-checking and reading logic into a centralized helper function in core.rs to improve code maintainability and ensure consistent error handling across the CLI.
| /// Maximum size of a link target in bytes. | ||
| /// Prevents memory exhaustion when reading link targets from untrusted archives. | ||
| pub(crate) const MAX_LINK_TARGET_SIZE: usize = 64 * 1024; |
There was a problem hiding this comment.
The logic for reading a link target with a size limit is duplicated across list, diff, and extract commands. It would be better to refactor this into a shared helper function in core.rs to improve maintainability and ensure consistent behavior. Note that using io::Error::new with a specific io::ErrorKind is preferred over io::Error::other for better semantic clarity.
/// Maximum size of a link target in bytes.
/// Prevents memory exhaustion when reading link targets from untrusted archives.
pub(crate) const MAX_LINK_TARGET_SIZE: usize = 64 * 1024;
pub(crate) fn read_link_target<T: AsRef<[u8]>>(
entry: &pna::NormalEntry<T>,
password: Option<&[u8]>,
) -> std::io::Result<String> {
let mut target = String::new();
entry
.reader(pna::ReadOptions::with_password(password))?
.take(MAX_LINK_TARGET_SIZE as u64 + 1)
.read_to_string(&mut target)?;
if target.len() > MAX_LINK_TARGET_SIZE {
return Err(std::io::Error::new(
std::io::ErrorKind::InvalidData,
"link target too long",
));
}
Ok(target)
}References
- When reporting errors in Rust, using io::Error::new with a specific io::ErrorKind (e.g., io::ErrorKind::PermissionDenied) is preferred over io::Error::other with a generic message, as it provides better semantic clarity and aids in debugging and error handling.
| MAX_LINK_TARGET_SIZE, PathFilter, ProcessAction, SplitArchiveReader, | ||
| TimeFilterResolver, TimeFilters, collect_split_archives, read_paths, run_read_entries, | ||
| run_read_entries_stoppable, | ||
| }, |
There was a problem hiding this comment.
Add the read_link_target helper to the imports from core to allow refactoring the duplicated link target extraction logic.
MAX_LINK_TARGET_SIZE, PathFilter, ProcessAction, SplitArchiveReader,
TimeFilterResolver, TimeFilters, collect_split_archives, read_link_target,
read_paths, run_read_entries, run_read_entries_stoppable,| (|| { | ||
| let mut reader = entry.reader(ReadOptions::with_password(password))?; | ||
| let mut s = String::new(); | ||
| reader | ||
| .by_ref() | ||
| .take(MAX_LINK_TARGET_SIZE as u64 + 1) | ||
| .read_to_string(&mut s)?; | ||
| if s.len() > MAX_LINK_TARGET_SIZE { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "link target too long", | ||
| )); | ||
| } | ||
| Ok(s) | ||
| })() |
There was a problem hiding this comment.
Use the read_link_target helper function to simplify the code and avoid duplication.
read_link_target(entry, password)| (|| { | ||
| let mut reader = entry.reader(ReadOptions::with_password(password))?; | ||
| let mut s = String::new(); | ||
| reader | ||
| .by_ref() | ||
| .take(MAX_LINK_TARGET_SIZE as u64 + 1) | ||
| .read_to_string(&mut s)?; | ||
| if s.len() > MAX_LINK_TARGET_SIZE { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "link target too long", | ||
| )); | ||
| } | ||
| Ok(s) | ||
| })() |
There was a problem hiding this comment.
Use the read_link_target helper function to simplify the code and avoid duplication.
read_link_target(entry, password)| command::{ | ||
| Command, ask_password, | ||
| core::{SplitArchiveReader, collect_split_archives}, | ||
| core::{MAX_LINK_TARGET_SIZE, SplitArchiveReader, collect_split_archives}, |
There was a problem hiding this comment.
Add the read_link_target helper to the imports from core.
| core::{MAX_LINK_TARGET_SIZE, SplitArchiveReader, collect_split_archives}, | |
| core::{MAX_LINK_TARGET_SIZE, SplitArchiveReader, collect_split_archives, read_link_target}, |
| let mut reader = entry.reader(ReadOptions::with_password(password))?; | ||
| let mut link_str = String::new(); | ||
| reader.read_to_string(&mut link_str)?; | ||
| reader | ||
| .by_ref() | ||
| .take(MAX_LINK_TARGET_SIZE as u64 + 1) | ||
| .read_to_string(&mut link_str)?; | ||
| if link_str.len() > MAX_LINK_TARGET_SIZE { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "link target too long", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Use the read_link_target helper function to simplify the code and avoid duplication.
| let mut reader = entry.reader(ReadOptions::with_password(password))?; | |
| let mut link_str = String::new(); | |
| reader.read_to_string(&mut link_str)?; | |
| reader | |
| .by_ref() | |
| .take(MAX_LINK_TARGET_SIZE as u64 + 1) | |
| .read_to_string(&mut link_str)?; | |
| if link_str.len() > MAX_LINK_TARGET_SIZE { | |
| return Err(io::Error::new( | |
| io::ErrorKind::InvalidData, | |
| "link target too long", | |
| )); | |
| } | |
| let link_str = read_link_target(&entry, password)?; |
| let mut reader = entry.reader(ReadOptions::with_password(password))?; | ||
| let mut target = String::new(); | ||
| reader.read_to_string(&mut target)?; | ||
| reader | ||
| .by_ref() | ||
| .take(MAX_LINK_TARGET_SIZE as u64 + 1) | ||
| .read_to_string(&mut target)?; | ||
| if target.len() > MAX_LINK_TARGET_SIZE { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "link target too long", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Use the read_link_target helper function to simplify the code and avoid duplication.
| let mut reader = entry.reader(ReadOptions::with_password(password))?; | |
| let mut target = String::new(); | |
| reader.read_to_string(&mut target)?; | |
| reader | |
| .by_ref() | |
| .take(MAX_LINK_TARGET_SIZE as u64 + 1) | |
| .read_to_string(&mut target)?; | |
| if target.len() > MAX_LINK_TARGET_SIZE { | |
| return Err(io::Error::new( | |
| io::ErrorKind::InvalidData, | |
| "link target too long", | |
| )); | |
| } | |
| let target = read_link_target(&entry, password)?; |
| PermissionStrategyResolver, ProcessAction, SafeWriter, TimeFilterResolver, TimeFilters, | ||
| TimestampStrategy, TimestampStrategyResolver, Umask, XattrStrategy, apply_chroot, | ||
| collect_split_archives, | ||
| AclStrategy, FflagsStrategy, KeepOptions, MAX_LINK_TARGET_SIZE, MacMetadataStrategy, |
There was a problem hiding this comment.
Add the read_link_target helper to the imports from core.
| AclStrategy, FflagsStrategy, KeepOptions, MAX_LINK_TARGET_SIZE, MacMetadataStrategy, | |
| AclStrategy, FflagsStrategy, KeepOptions, MAX_LINK_TARGET_SIZE, read_link_target, MacMetadataStrategy, |
| let mut reader = item.reader(ReadOptions::with_password(password))?; | ||
| let mut original = String::new(); | ||
| reader | ||
| .by_ref() | ||
| .take(MAX_LINK_TARGET_SIZE as u64 + 1) | ||
| .read_to_string(&mut original)?; | ||
| if original.len() > MAX_LINK_TARGET_SIZE { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "link target too long", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Use the read_link_target helper function to simplify the code and avoid duplication.
| let mut reader = item.reader(ReadOptions::with_password(password))?; | |
| let mut original = String::new(); | |
| reader | |
| .by_ref() | |
| .take(MAX_LINK_TARGET_SIZE as u64 + 1) | |
| .read_to_string(&mut original)?; | |
| if original.len() > MAX_LINK_TARGET_SIZE { | |
| return Err(io::Error::new( | |
| io::ErrorKind::InvalidData, | |
| "link target too long", | |
| )); | |
| } | |
| let original = read_link_target(&item, password)?; |
| let mut reader = item.reader(ReadOptions::with_password(password))?; | ||
| let mut original = String::new(); | ||
| reader | ||
| .by_ref() | ||
| .take(MAX_LINK_TARGET_SIZE as u64 + 1) | ||
| .read_to_string(&mut original)?; | ||
| if original.len() > MAX_LINK_TARGET_SIZE { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "link target too long", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Use the read_link_target helper function to simplify the code and avoid duplication.
| let mut reader = item.reader(ReadOptions::with_password(password))?; | |
| let mut original = String::new(); | |
| reader | |
| .by_ref() | |
| .take(MAX_LINK_TARGET_SIZE as u64 + 1) | |
| .read_to_string(&mut original)?; | |
| if original.len() > MAX_LINK_TARGET_SIZE { | |
| return Err(io::Error::new( | |
| io::ErrorKind::InvalidData, | |
| "link target too long", | |
| )); | |
| } | |
| let original = read_link_target(&item, password)?; |
Define `MAX_LINK_TARGET_SIZE` (64 KiB) and enforce this limit when reading symbolic and hard link targets from archives. This prevents a potential denial-of-service (DoS) attack where a malicious archive contains excessively large link targets that exhaust available memory during extraction, listing, or comparison. Changes: - Added `MAX_LINK_TARGET_SIZE` constant in `cli/src/command/core.rs`. - Enforced size limit in `cli/src/command/extract.rs`, `cli/src/command/list.rs`, and `cli/src/command/diff.rs`. - Added integration tests in `cli/tests/cli/extract/link_limit.rs` to verify enforcement (excluded for WASM target). - Suppressed unused variable warning in `cli/src/command/extract.rs` for non-macOS targets. - Updated `.jules/sentinel.md` with security learnings. Co-authored-by: ChanTsune <41658782+ChanTsune@users.noreply.github.com>
1 participant
🛡️ Sentinel: [HIGH] Harden link target extraction against memory exhaustion
MAX_LINK_TARGET_SIZEconstant set to 64 KiB (a generous limit considering standard OS path limits like 4096 bytes). Modified theextract,list, anddiffcommands to usereader.take(MAX_LINK_TARGET_SIZE + 1)when reading link targets and verify that the actual length does not exceed the limit.cli/tests/cli/extract/link_limit.rsthat verify the archiver correctly rejects archives with link targets exceeding the 64 KiB limit. All tests passed.cargo test -p portable-network-archive --test cli link_limitPR created automatically by Jules for task 6585411856353083135 started by @ChanTsune
Summary by CodeRabbit
Bug Fixes
Tests