Skip to content
Open
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
113 changes: 112 additions & 1 deletion rust/src/files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,22 @@ pub fn create_path_if_not_exists(path: &Path) -> Result<(), Error> {
Ok(())
}

pub fn check_path_traversal(entry_path: &Path) -> Result<(), Error> {
if entry_path.as_os_str().is_empty()
|| entry_path.components().any(|c| {
matches!(
c,
std::path::Component::ParentDir
| std::path::Component::RootDir
| std::path::Component::Prefix(_)
)
})
{
return Err(anyhow!("Unsafe entry (path traversal): {:?}", entry_path));
}
Ok(())
}
Comment on lines +95 to +109

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.

Action required

2. Tar.gz bypasses validation 🐞 Bug ⛨ Security

check_path_traversal() is never applied to the .gz extraction path: uncompress() dispatches
GZ to untargz(), which calls Archive::unpack() directly without validating archive entry
paths. As a result, tar.gz remains outside the PR’s “reject unsafe entries” behavior, undermining
the stated traversal prevention for tar extraction.
Agent Prompt
### Issue description
The PR introduces `check_path_traversal()` and uses it in `uncompress_pkg()` and `uncompress_tar()`, but `.tar.gz` files are extracted via `untargz()` which does not validate entry paths.

### Issue Context
`uncompress()` routes `GZ` to `untargz()`, and `untargz()` uses `tar::Archive::unpack()` without calling `check_path_traversal()` on each entry. This means the security behavior added in this PR is not applied to tar.gz.

### Fix Focus Areas
- rust/src/files.rs[149-167]
- rust/src/files.rs[360-375]

### Suggested fix approach
- Refactor `untargz()` to iterate `archive.entries()?` and, for each entry:
  - read `entry.path()?` (or equivalent),
  - apply `check_path_traversal()` to that path,
  - unpack the entry into the intended destination (e.g., by joining to `parent_path` after validation, or using an unpack-into-directory API if available).
- Optionally add a unit test similar to `uncompress_tar_rejects_path_traversal_entry`, but covering the `.gz` path to prevent regressions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +95 to +109

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.

Action required

1. check_path_traversal missing doc comment 📘 Rule violation ✧ Quality

The new public function check_path_traversal is introduced without any preceding /// doc
comment. This violates the requirement that public API items be documented with structured Rust doc
comments (including sections like # Arguments and # Errors where applicable).
Agent Prompt
## Issue description
A new public function `check_path_traversal` was added without required Rust doc comments (`///`). Because it takes an argument and returns `Result<_, _>`, its docs should include `# Arguments` and `# Errors` sections.

## Issue Context
This function is part of the public API surface (`pub fn`) and is security-relevant validation logic.

## Fix Focus Areas
- rust/src/files.rs[95-109]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


pub fn uncompress(
compressed_file: &str,
target: &Path,
Expand Down Expand Up @@ -215,6 +231,7 @@ pub fn uncompress_pkg(compressed_file: &str, target: &Path, log: &Logger) -> Res
while let Some(next) = cpio_reader.next() {
let entry = next?;
let name = entry.name();
check_path_traversal(Path::new(name))?;

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.

Action required

1. check_path_traversal lacks tests 📘 Rule violation ☼ Reliability

New validation rejects archive entries containing .. components in uncompress_pkg() and
uncompress_tar(), but this behavior change is not accompanied by targeted tests. This risks
regressions and gaps in coverage for a security-sensitive extraction path.
Agent Prompt
## Issue description
The PR adds path traversal validation during archive extraction, but does not add tests to confirm malicious paths are rejected and safe paths still extract.

## Issue Context
The behavior change occurs in `uncompress_pkg()` and `uncompress_tar()` via `check_path_traversal()`. Because this is security-sensitive and error-path driven, tests should cover both allowed and rejected cases.

## Fix Focus Areas
- rust/src/files.rs[95-103]
- rust/src/files.rs[219-236]
- rust/src/files.rs[371-386]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

let mut file = Vec::new();
cpio_reader.read_to_end(&mut file)?;
let target_path_buf = target_path.join(name);
Comment on lines 231 to 237

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.

Action required

3. Absolute path escape 🐞 Bug ⛨ Security

check_path_traversal() only rejects ParentDir (..) components, so absolute/prefixed entry paths
pass validation. In uncompress_pkg(), an absolute name can cause target_path.join(name) to
ignore target_path and write outside the extraction directory.
Agent Prompt
## Issue description
`check_path_traversal()` currently only rejects `..` path components. Absolute paths (Unix `/etc/passwd`) and Windows-style prefixed paths (e.g., `C:\Windows\...`, UNC) can still pass validation and escape the intended extraction root when joined with `target`.

## Issue Context
`uncompress_pkg()` validates `entry.name()` with `check_path_traversal(Path::new(name))` and then writes to `target_path.join(name)`. If `name` is absolute/prefixed, the joined path may resolve outside `target`.

## Fix Focus Areas
- rust/src/files.rs[95-103]
- rust/src/files.rs[219-237]

### Suggested fix
- Update `check_path_traversal()` to reject any non-relative components, not just `ParentDir`, e.g. fail on `Component::RootDir` and `Component::Prefix(_)` as well.
- Optionally add a defensive post-join check in `uncompress_pkg()` (and any similar extraction code): after building the output path, ensure it is not absolute and is lexically under `target`.
- Add unit/integration tests (if available in this repo) covering entry names like `/tmp/evil`, `\\server\share\evil` (Windows), and `C:\evil`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines 233 to 237

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.

Action required

1. Pkg empty name accepted 🐞 Bug ≡ Correctness

uncompress_pkg() validates ../absolute/prefixed paths but does not reject an empty
entry.name(), so target.join("") resolves to the extraction root and fs::write() may
create/overwrite the target path as a file instead of a directory. This can cause extraction
failures or clobber the intended extraction directory when the target path does not yet exist.
Agent Prompt
## Issue description
`uncompress_pkg()` calls `check_path_traversal(Path::new(name))`, but `check_path_traversal()` does not reject empty paths. If a CPIO entry name is `""`, then `target_path.join(name)` resolves to `target` itself, and `fs::write(&target_path_buf, file)` may create/overwrite the extraction root path as a file (especially when `target` doesn’t exist yet).

## Issue Context
The tar extraction path already explicitly rejects an empty entry path; pkg/cpio should match that safety behavior.

## Fix Focus Areas
- rust/src/files.rs[223-253]
- rust/src/files.rs[95-107]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Expand Down Expand Up @@ -367,7 +384,13 @@ pub fn uncompress_tar(decoder: &mut dyn Read, target: &Path, log: &Logger) -> Re
let mut archive = Archive::new(Cursor::new(buffer));
for entry in archive.entries()? {
let mut entry_decoder = entry?;
let entry_path: PathBuf = entry_decoder.path()?.iter().skip(1).collect();
let path = entry_decoder.path()?;
let entry_path: PathBuf = if path.iter().count() > 1 {
path.iter().skip(1).collect()
} else {
path.to_path_buf()
};
check_path_traversal(&entry_path)?;
Comment on lines +387 to +393

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.

Action required

2. Tar strips before validation 🐞 Bug ⛨ Security

uncompress_tar() drops the first path component before calling check_path_traversal(), so a tar
entry like ../evil (or any other unsafe first component) can be rewritten into a seemingly safe
path and not rejected. This contradicts the intended "reject ParentDir(..) components" behavior and
makes tar validation weaker/inconsistent compared to pkg validation.
Agent Prompt
### Issue description
`uncompress_tar()` validates a *modified* path (`entry_path`) after stripping the first component, which can remove `..`/prefix/root indicators present in the original tar entry path.

### Issue Context
The PR introduces `check_path_traversal()` to reject unsafe path components. However, `uncompress_tar()` constructs `entry_path` via `path.iter().skip(1)` before calling `check_path_traversal()`, meaning the original entry path is never validated.

### Fix Focus Areas
- Validate the original path returned by `entry_decoder.path()?` with `check_path_traversal()` **before** stripping the first component.
- After stripping the leading component (when desired), validate the resulting `entry_path` as well.
- Consider only stripping the first component when the first component is a normal directory name (not `RootDir`, `Prefix`, `ParentDir`, etc.).

- rust/src/files.rs[377-397]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

let entry_target = target.join(entry_path);
fs::create_dir_all(entry_target.parent().unwrap())?;
entry_decoder.unpack(entry_target)?;
Comment on lines 385 to 396

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.

Action required

1. Tar symlink escape 🐞 Bug ⛨ Security

uncompress_tar() only rejects .. components, then unconditionally calls
entry_decoder.unpack(entry_target), which can create symlinks/hardlinks from the archive. A
malicious tar can first create a symlink inside the target (e.g., link -> /etc) and then write
link/passwd, escaping the extraction directory despite passing the .. check.
Agent Prompt
## Issue description
`uncompress_tar()` currently validates paths only by rejecting `..` components, but then unpacks every tar entry as-is. Tar archives can contain symlinks and hardlinks; extracting them allows later entries to write outside the intended extraction directory by writing through those links.

## Issue Context
- `check_path_traversal()` only checks for `Component::ParentDir` and does not account for link-based escapes.
- `uncompress_tar()` uses `entry_decoder.unpack(entry_target)` without checking the tar entry type or preventing symlink/hardlink creation.

## Fix Focus Areas
- rust/src/files.rs[95-103]
- rust/src/files.rs[371-386]

## What to change
1. In `uncompress_tar()`, inspect each tar entry’s type (via the tar header) before unpacking.
2. Reject (return an error) for symlink and hardlink entry types (and any other non-regular types you don’t explicitly support).
   - If directory entries are expected, allow directories.
   - If only regular files are expected for these tarballs, allow only regular files.
3. (Optional but stronger) Add a defense-in-depth check that the extraction path does not traverse through symlinks already present on disk (walk parent components, using `symlink_metadata`, and fail if any component is a symlink).

## Suggested tests
Add a unit test that builds a tar containing:
- an entry that creates a symlink inside the extraction directory
- a subsequent entry that writes through that symlink
and assert extraction fails (or the escaped path is not written).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Expand Down Expand Up @@ -696,3 +719,91 @@ pub fn get_win_file_version(file_path: &str) -> Option<String> {
Some(product_version.trim_end_matches('\0').to_string())
}
}

#[cfg(test)]
mod tests {
use super::*;
use std::io::Cursor;

fn build_tar(entries: &[(&str, &[u8])]) -> Vec<u8> {
let mut buffer = Vec::new();
for (name, contents) in entries {
let mut header = tar::Header::new_gnu();
header.set_size(contents.len() as u64);
header.set_mode(0o644);
header.set_path("browser/file.txt").unwrap();
header.set_cksum();

let mut header_bytes = header.as_bytes().to_vec();
let name_bytes = name.as_bytes();
assert!(name_bytes.len() <= 100, "test tar name too long");
header_bytes[0..100].fill(0);
header_bytes[0..name_bytes.len()].copy_from_slice(name_bytes);
header_bytes[148..156].fill(b' ');
let checksum: u32 = header_bytes.iter().map(|byte| *byte as u32).sum();
let checksum_bytes = format!("{:06o}\0 ", checksum);
header_bytes[148..156].copy_from_slice(checksum_bytes.as_bytes());

buffer.extend_from_slice(&header_bytes);
buffer.extend_from_slice(contents);

let remainder = contents.len() % 512;
if remainder != 0 {
buffer.extend_from_slice(&vec![0u8; 512 - remainder]);
}
}
buffer.extend_from_slice(&[0u8; 1024]);
buffer
}

#[test]
fn check_path_traversal_allows_safe_paths() {
assert!(check_path_traversal(Path::new("browser/file.txt")).is_ok());
}

#[test]
fn check_path_traversal_rejects_empty_path() {
let err = check_path_traversal(Path::new("")).unwrap_err();
assert!(err.to_string().contains("Unsafe entry (path traversal)"));
}

#[test]
fn uncompress_tar_extracts_safe_entry() {
let temp_dir = tempfile::tempdir().unwrap();
let target = temp_dir.path().join("extract");
let tar_data = build_tar(&[("browser/file.txt", b"hello")]);
let mut decoder = Cursor::new(tar_data);
let log = Logger::new();

uncompress_tar(&mut decoder, &target, &log).unwrap();

assert_eq!(fs::read(target.join("file.txt")).unwrap(), b"hello");
}

#[test]
fn uncompress_tar_keeps_single_component_entry() {
let temp_dir = tempfile::tempdir().unwrap();
let target = temp_dir.path().join("extract");
let tar_data = build_tar(&[("file.txt", b"hello")]);
let mut decoder = Cursor::new(tar_data);
let log = Logger::new();

uncompress_tar(&mut decoder, &target, &log).unwrap();

assert_eq!(fs::read(target.join("file.txt")).unwrap(), b"hello");
}

#[test]
fn uncompress_tar_rejects_path_traversal_entry() {
let temp_dir = tempfile::tempdir().unwrap();
let target = temp_dir.path().join("extract");
let escape_path = temp_dir.path().join("escape.txt");
let tar_data = build_tar(&[("browser/../../escape.txt", b"owned")]);
let mut decoder = Cursor::new(tar_data);
let log = Logger::new();

let err = uncompress_tar(&mut decoder, &target, &log).unwrap_err();
assert!(err.to_string().contains("Unsafe entry (path traversal)"));
assert!(!escape_path.exists());
}
}
Loading