Skip to content

security improvements#951

Open
curious-rabbit wants to merge 5 commits intoouch-org:mainfrom
curious-rabbit:security
Open

security improvements#951
curious-rabbit wants to merge 5 commits intoouch-org:mainfrom
curious-rabbit:security

Conversation

@curious-rabbit
Copy link
Copy Markdown
Contributor

Multiple problems affecting security

  • Tar symlink/hardlink entries with absolute or ../ paths escape the output directory, because PathBuf::join replaces the base when the second arg is absolute.
  • Zip symlink targets aren't checked. Combined with a second entry that writes through the symlink, files land outside the output folder.
  • Filenames, zip comments, symlink targets and similar attacker-controlled strings are printed to the terminal with path.display(), so an archive can inject ESC sequences, BiDi overrides (CVE-2021-42574), zero-width chars, etc.
  • Setuid/setgid bits from zip entries are preserved on extraction.
  • No cap on decompression output, so a small gzip/xz/zstd bomb can fill disk or RAM. Applies to both decompress and list.
  • LZMA memory limit is set to u32::MAX (4 GiB) in both decompress.rs and list.rs.
  • --password on the command line leaks via ps and shell history; no env var alternative exists.
  • Zip extraction creates files with default permissions first, then chmods to the archive mode, leaving a short window where a sensitive file is world-readable.

Copy link
Copy Markdown
Collaborator

@tommady tommady left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the contribution!

I left a comment.

One small thing I noticed: it looks like LimitedReader went on vacation and didn’t make it into:

  1. tar
  2. rar

Might be worth inviting it back for consistency (and so things don’t get… too unlimited). 😄

Comment thread src/utils/fs.rs
Comment on lines +150 to +152
if target.is_absolute() {
return Ok(());
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Our zip.rs extraction is a bit too naive.

In zip.rs, we use output_folder.join(&relpath) + fs::File::create(), which will blindly follow symlinks on disk. That means a malicious archive like:

  1. link -> /etc (allowed by the absolute check)
  2. link/passwd -> (malicious content, allowed by the absolute check)

would end up overwriting /etc/passwd on the host.

The tar crate avoids this by checking path metadata during extraction to ensure writes stay inside the output dir.
https://docs.rs/tar/0.4.42/src/tar/entry.rs.html#937-947

I’d suggest we follow the same approach in zip.rs.
WDYT?

Copy link
Copy Markdown
Collaborator

@tommady tommady left a comment

Choose a reason for hiding this comment

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

I'd love your validate_dest_inside_root check!
it is a very clever and highly performant way than canonicalization.

@marcospb19 please help check this
thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants