Prevent extraction from escaping destination_dir via pre-existing symlinks#9493
Open
thesmartshadow wants to merge 3 commits into
Open
Prevent extraction from escaping destination_dir via pre-existing symlinks#9493thesmartshadow wants to merge 3 commits into
thesmartshadow wants to merge 3 commits into
Conversation
hsbt
requested changes
Apr 30, 2026
Member
hsbt
left a comment
hsbt
left a comment
There was a problem hiding this comment.
Please align code indentation at first.
thesmartshadow
force-pushed
the
fix/cwe59-symlink-extract
branch
from
2c5093b to
643e023
Compare
Author
Thanks I fixed the indentation and pushed an update. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Gem::Package#extract_tar_gz to prevent extraction from escaping destination_dir when pre-existing symlinks exist inside the destination tree, and adds a regression test for that scenario.
Changes:
- Add a realpath-based containment check on the target entry’s parent directory during extraction.
- Add a regression test ensuring extraction fails when a pre-existing symlink in the destination path would redirect writes outside the extraction root.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/rubygems/package.rb | Adds a link-aware containment check intended to block symlink-based escape during extraction. |
| test/rubygems/test_gem_package.rb | Adds a regression test that plants a symlink in the destination path and expects Gem::Package::PathError. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
451
to
+459
| unless directories.include?(mkdir) | ||
| FileUtils.mkdir_p mkdir, mode: dir_mode ? 0o755 : (entry.header.mode if entry.directory?) | ||
| directories << mkdir | ||
| end | ||
|
|
||
| real_mkdir = File.realpath(mkdir) | ||
| unless real_mkdir == destination_dir || normalize_path(real_mkdir).start_with?(normalize_path(destination_dir + "/")) | ||
| raise Gem::Package::PathError.new(real_mkdir, destination_dir) | ||
| end |
| raise Gem::Package::PathError.new(real_mkdir, destination_dir) | ||
| end | ||
|
|
||
| if entry.file? |
|
|
||
| refute File.exist?(escaped), "must not write outside extraction root via symlink" | ||
| end | ||
|
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was the end-user or developer problem that led to this PR?
When extracting a gem, RubyGems correctly rejects obvious path traversal patterns such as absolute paths or
.., but it can still follow a pre-existing symlink inside the destination directory.In practice, if part of the extraction path already exists as a symlink pointing outside the extraction root, a file that appears to be written under
destination_dircan end up being written outside of it instead. That breaks the expected safety boundary of extraction and makes extraction unsafe in reused or shared directories.What is your fix for the problem, implemented in this PR?
This PR adds a link-aware safety check before writing extracted files.
After ensuring the parent directory exists, it resolves the real path of that parent directory and verifies that it still stays under the intended extraction root. If the resolved parent path escapes
destination_dir, extraction now raisesGem::Package::PathErrorinstead of continuing with the write.This keeps extraction confined to the intended destination even when pre-existing symlinks are present in the path.
This PR also adds a regression test covering that case by planting a symlink in the destination path and asserting that:
Gem::Package::PathErrorValidation
I verified the change with:
Gem::Package::PathErrorChecklist