-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
[rust] Prevent path traversal in tar and pkg extraction #17668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Changes from all commits
7c2b6e3
dde0400
c5d6d7e
6c652bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. check_path_traversal missing doc comment 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
|
||
|
|
||
| pub fn uncompress( | ||
| compressed_file: &str, | ||
| target: &Path, | ||
|
|
@@ -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))?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. check_path_traversal lacks tests 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
|
||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3. Absolute path escape 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
Comment on lines
233
to
237
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Pkg empty name accepted 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
|
||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. Tar strips before validation 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
|
||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Tar symlink escape 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
|
||
|
|
@@ -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()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2. Tar.gz bypasses validation
🐞 Bug⛨ SecurityAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools