Adds verification of the file name by andsel · Pull Request #18848 · elastic/logstash

andsel · 2026-03-11T09:07:08Z

Release notes

[rn:skip]

What does this PR do?

Adds a file name verification

github-actions · 2026-03-11T09:07:17Z

🤖 GitHub comments

Just comment with:

run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)
run exhaustive tests : Run the exhaustive tests Buildkite pipeline.

mergify · 2026-03-11T09:07:50Z

This pull request does not have a backport label. Could you fix it @andsel? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
If no backport is necessary, please add the backport-skip label

andsel · 2026-03-11T13:58:49Z

run exhaustive tests

andrewvc

If we're improving the code here we should explicitly handle the symlink case for clarity.

Is there a reason tests weren't added? For a security fix I'd expect that.

andrewvc · 2026-03-11T15:50:38Z

lib/bootstrap/util/compress.rb

        raise CompressError.new("Directory #{target} exist") if ::File.exist?(target)
        ::Zip::File.open(source) do |zip_file|
          zip_file.each do |file|
+            raise CompressError.new("Extracting file outside target directory: #{file.name}") unless LogStash::Util.name_safe?(file.name)


I think the copy here could be improved.

Suggested change

raise CompressError.new("Extracting file outside target directory: #{file.name}") unless LogStash::Util.name_safe?(file.name)

raise CompressError.new("Refusing to extract file to unsafe path: #{file.name}. Files may not traverse with `..`") unless LogStash::Util.name_safe?(file.name)

I'd rather have the validation code tell us why the provided path is unsafe, instead of having the validator return boolean and hard-coding a reason here.

For example, the name_safe? method will also return false when the provided path is absolute.

If we do provide blanket guidance, I would prefer to include all possible failure reasons:

Suggested change

raise CompressError.new("Extracting file outside target directory: #{file.name}") unless LogStash::Util.name_safe?(file.name)

raise CompressError.new("Refusing to extract file outside target directory: `#{file.name}`. Paths must be relative and may not traverse with `..`") unless LogStash::Util.name_safe?(file.name)

andrewvc · 2026-03-11T16:13:11Z

lib/bootstrap/util/compress.rb

            tar_file.each do |entry|
+              raise CompressError.new("Extracting file outside target directory: #{entry.full_name}") unless LogStash::Util.name_safe?(entry.full_name)
              target_path = ::File.join(target, entry.full_name)



Since we're already in here, it'd be nice to handle the symlink issue for safety. When I started reviewing this I saw the note about them and was forced to think through whether this was exploitable or not.

While we are not presently vulnerable--since we only write regular files or create directories--it would be nice to at least handle the case here explicitly.

If someone (or some agent) mistakenly added symlink support this would be defensive. Also, we can remove the note later in the file about symlinks not being covered. This is one less bit of security surface area future contributors would have to worry about.

Suggested change

# POSIX tar typeflag values: '1' = hard link, '2' = symbolic link

# https://pubs.opengroup.org/onlinepubs/9699919799/utilities/pax.html#tag_20_92_13_06

# The pathname of a link being created to another file, of any type, previously archived. This record shall override the linkname field in the following ustar header block(s). The following ustar header block shall determine the type of link created. If typeflag of the following header block is 1, it shall be a hard link. If typeflag is 2, it shall be a symbolic link and the linkpath value shall be the contents of the symbolic link. The pax utility shall translate the name of the link (contents of the symbolic link) from the encoding in the header to the character set appropriate for the local file system. When used in write or copy mode, pax shall include a linkpath extended header record for each link whose pathname cannot be represented entirely with the members of the portable character set other than NUL.

case entry.header.typeflag

when '2' then raise CompressError.new("Refusing to extract symlink entry: #{entry.full_name}")

when '1' then raise CompressError.new("Refusing to extract hardlink entry: #{entry.full_name}")

end

I think we could use the symlink? method present in Gem::Package::TarReader::Entry instead of using magic numbers (https://github.com/ruby/rubygems/blob/bcc44695d6bb0915912e10cbb09283c1e242f1ff/lib/rubygems/package/tar_reader/entry.rb#L127).

However, it doesn't cover the hardlink case.

It seems that also zip has symlink? method we can leverage.

From better investigation I found that rubyzip (at the version we use and above) disabled to decompress symlinks for security reasons.

Version 3.2.2 apparently re-introduced symlink: rubyzip/rubyzip#531

Hmmm, I'm not nuts about magic numbers either, weird there's no hardlink option. What would you like to do here?

Ah, I see you added the symlink check below. Looks like the lib only covers symlinks anyway, so I think this is good!

andrewvc · 2026-03-11T16:15:20Z

lib/bootstrap/util/compress.rb

    end
+
+    # Is the name a relative path, free of `..` patterns that could lead to
+    # path traversal attacks? This does NOT handle symlinks; if the path


Per my comment above, we can remove the mention of symlinks if we add the check for them.

andsel · 2026-03-13T16:10:26Z

run exhaustive tests

…ent exception for each case

andsel · 2026-03-16T16:19:22Z

run exhaustive tests

Copilot

Pull request overview

This PR adds security validation for file names during archive extraction (both zip and tar.gz) to prevent path traversal attacks. It also adds symlink support for tar.gz extraction with safety checks to ensure symlink targets remain within the extraction directory. Additionally, the pack installer is updated to support .tar.gz format alongside .zip.

Changes:

Adds verify_name_safe! method to reject nil/empty, absolute, or ..-traversing paths during extraction
Adds symlink_target_safe? method and symlink extraction support for tar.gz archives
Extends PackInstaller::Local to accept and extract .tar.gz packs alongside .zip

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
lib/bootstrap/util/compress.rb	Adds path validation (`verify_name_safe!`), symlink safety check (`symlink_target_safe?`), and symlink extraction in `Tar.extract`
lib/pluginmanager/pack_installer/local.rb	Extends pack installer to support `.tar.gz` format with appropriate extraction routing and error handling
spec/unit/util/compress_spec.rb	Tests for path validation, traversal rejection, and symlink safety in both zip and tar extraction
spec/unit/plugin_manager/pack_installer/local_spec.rb	Test for tar.gz pack extraction with symlinks
spec/support/pack/pack_with_symlink.tar.gz	Test fixture tar.gz containing a symlink
spec/support/pack/README.md	Documents how to recreate the test fixture

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

lib/bootstrap/util/compress.rb

lib/pluginmanager/pack_installer/local.rb

spec/support/pack/README.md

lib/bootstrap/util/compress.rb

…that uses tar.gz files

andsel · 2026-03-19T12:49:41Z

run exhaustive tests

Copilot

Pull request overview

This PR hardens archive extraction by adding validation for archive entry names (to prevent unsafe extraction paths) and rejecting tar symlink entries, with accompanying spec updates and fixture documentation.

Changes:

Add archive-entry path validation for Zip/Tar extraction and reject tar symlink entries.
Add/extend unit and integration-ish specs around unsafe paths and symlink behavior.
Improve plugin pack installation error handling by mapping LogStash::CompressError to InvalidPackError.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`lib/bootstrap/util/compress.rb`	Adds entry-name validation, rejects tar symlinks, and wires checks into Zip/Tar extraction.
`lib/pluginmanager/pack_installer/local.rb`	Rescues `LogStash::CompressError` during uncompress and raises `InvalidPackError` with details.
`spec/unit/util/compress_spec.rb`	Adds tests for new safety checks and adjusts tar mocks for new `symlink?`/`directory?` usage.
`x-pack/spec/geoip_database_management/downloader_spec.rb`	Adds a spec asserting tarballs with symlinks raise `LogStash::CompressError`.
`x-pack/spec/geoip_database_management/fixtures/README.md`	Documents how to recreate the symlink-containing tar fixture.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

x-pack/spec/geoip_database_management/downloader_spec.rb

spec/unit/util/compress_spec.rb

lib/bootstrap/util/compress.rb

…ymlink just raising an error when a tgz contains a symlink

Copilot

Pull request overview

This PR hardens archive extraction by validating entry filenames/paths and rejecting tar symlink entries, with accompanying specs and fixtures to prevent path traversal and symlink-based extraction.

Changes:

Add LogStash::Util.verify_name_safe! and apply it to Zip/Tar extraction paths.
Reject tar symlink entries via verify_tar_link_entry! and add fixture-based specs asserting the behavior.
Wrap LogStash::CompressError as InvalidPackError in the local plugin pack installer.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`lib/bootstrap/util/compress.rb`	Adds entry-name safety verification for zip/tar extraction and rejects tar symlink entries.
`lib/pluginmanager/pack_installer/local.rb`	Converts `CompressError` failures during uncompress into `InvalidPackError`.
`spec/unit/util/compress_spec.rb`	Adds unit coverage for safe-name verification, traversal rejection, and tar symlink rejection.
`x-pack/spec/geoip_database_management/downloader_spec.rb`	Adds a downloader spec asserting symlink tarball rejection.
`x-pack/spec/geoip_database_management/fixtures/sample_with_symlink.tgz`	Adds a tarball fixture containing a symlink entry.
`x-pack/spec/geoip_database_management/fixtures/README.md`	Documents how to regenerate the symlink fixture.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

lib/bootstrap/util/compress.rb

+    # Tar.gz-only: rejects symlink entries
+    # @param entry [Gem::Package::TarReader::Entry]
+    # @raise [CompressError] if entry is a symlink or path is unsafe
+    def self.verify_tar_link_entry!(entry)


x-pack/spec/geoip_database_management/downloader_spec.rb

+
+    it "raises when tarball contains a symlink entry" do
+      zip_path = ::File.expand_path("./fixtures/sample_with_symlink.tgz", ::File.dirname(__FILE__))
+      skip("sample_with_symlink.tgz not found") unless ::File.exist?(zip_path)


x-pack/spec/geoip_database_management/downloader_spec.rb

+    it "raises when tarball contains a symlink entry" do
+      zip_path = ::File.expand_path("./fixtures/sample_with_symlink.tgz", ::File.dirname(__FILE__))
+      skip("sample_with_symlink.tgz not found") unless ::File.exist?(zip_path)
+
+      expect { downloader.send(:unzip, database_type, dirname, zip_path) }.to raise_error(LogStash::CompressError, /Refusing to extract symlink entry/)


spec/unit/util/compress_spec.rb

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR adds archive entry validation to prevent unsafe extraction paths and expands test coverage around tar/zip extraction safety, including rejecting tar symlink entries.

Changes:

Validate zip and tar entry names during extraction via LogStash::Util.verify_name_safe!.
Reject symlink entries during tar.gz extraction via verify_tar_link_entry!, and surface failures as InvalidPackError in the plugin pack installer.
Add fixtures and specs to assert symlink rejection and unsafe-path handling.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`lib/bootstrap/util/compress.rb`	Adds path validation for zip/tar extraction and rejects tar symlinks.
`lib/pluginmanager/pack_installer/local.rb`	Converts `CompressError` from extraction into `InvalidPackError` for user-facing pack install errors.
`spec/unit/util/compress_spec.rb`	Adds unit coverage for path validation, traversal rejection, and symlink tarball fixture behavior.
`x-pack/spec/geoip_database_management/downloader_spec.rb`	Adds an integration-style spec to ensure GeoIP downloader rejects symlink tar entries.
`x-pack/spec/geoip_database_management/fixtures/sample_with_symlink.tgz`	New fixture tarball containing a symlink entry for extraction rejection tests.
`x-pack/spec/geoip_database_management/fixtures/README.md`	Documents how to recreate the new symlink-containing tarball fixture.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

spec/unit/util/compress_spec.rb

lib/bootstrap/util/compress.rb

              if entry.directory?
                FileUtils.mkdir_p(target_path)
-              else # is a file to be extracted
+              else # is a file to be extracted (symlinks rejected in verify_tar_link_entry!)


lib/bootstrap/util/compress.rb

+    # Tar.gz-only: rejects symlink entries
+    # @param entry [Gem::Package::TarReader::Entry]
+    # @raise [CompressError] if entry is a symlink or path is unsafe
+    def self.verify_tar_link_entry!(entry)


spec/unit/util/compress_spec.rb

+    it "does not raise for safe relative paths" do
+      expect { LogStash::Util.verify_name_safe!("foo/bar") }.not_to raise_error
+      expect { LogStash::Util.verify_name_safe!("a/b/c") }.not_to raise_error
+      expect { LogStash::Util.verify_name_safe!("single") }.not_to raise_error
+    end


x-pack/spec/geoip_database_management/downloader_spec.rb

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

elasticmachine · 2026-03-19T16:56:59Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 8095570

Failed CI Steps

History

💚 Build #4490 succeeded cc001c4
💚 Build #4481 succeeded 65c206d
💔 Build #4471 failed c90f19f
💔 Build #4470 failed fc75e3f
💛 Build #4469 was flaky 833979b
💚 Build #4467 succeeded 0e69e4b

cc @andsel

Adds verification of the file name

cdf7b80

andsel self-assigned this Mar 11, 2026

andsel marked this pull request as ready for review March 11, 2026 13:54

andrewvc requested changes Mar 11, 2026

View reviewed changes

Reimplemented name_safe? to work both on Unix and Windows filesystems

5aab49f

andsel added 2 commits March 13, 2026 18:03

Switched from boolean method to verification method that thorw differ…

0e69e4b

…ent exception for each case

Implemented handling of symlinks into tgz

fc75e3f

andsel force-pushed the fix/verify_name branch from 833979b to fc75e3f Compare March 16, 2026 15:25

andsel requested a review from Copilot March 16, 2026 16:19

Copilot started reviewing on behalf of andsel March 16, 2026 16:20 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

lib/bootstrap/util/compress.rb Show resolved Hide resolved

lib/pluginmanager/pack_installer/local.rb Outdated Show resolved Hide resolved

Keep the pack installer support only for zip files

c90f19f

andrewvc reviewed Mar 17, 2026

View reviewed changes

spec/support/pack/README.md Outdated Show resolved Hide resolved

andrewvc reviewed Mar 17, 2026

View reviewed changes

lib/bootstrap/util/compress.rb Outdated Show resolved Hide resolved

Moved test to verify symlink into GeoIP downloader, which is the one …

18be6d9

…that uses tar.gz files

andsel marked this pull request as draft March 17, 2026 13:51

andsel requested a review from Copilot March 19, 2026 14:42

Copilot AI reviewed Mar 19, 2026

View reviewed changes

Moved tar.gz test to the geoip downloader part and avoid to support s…

cbbf6d9

…ymlink just raising an error when a tgz contains a symlink

andsel force-pushed the fix/verify_name branch from 65c206d to cbbf6d9 Compare March 19, 2026 14:56

Fix, explicit import of pathname and bad method refernce name in comment

cc001c4

andsel requested a review from Copilot March 19, 2026 15:34

Copilot AI reviewed Mar 19, 2026

View reviewed changes

andsel marked this pull request as ready for review March 19, 2026 16:18

andsel requested a review from Copilot March 19, 2026 16:18

Potential fix for pull request finding

5a56f8a

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI reviewed Mar 19, 2026

View reviewed changes

Potential fix for pull request finding

8095570

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

andsel requested a review from andrewvc March 19, 2026 17:07

	raise CompressError.new("Extracting file outside target directory: #{file.name}") unless LogStash::Util.name_safe?(file.name)
	raise CompressError.new("Refusing to extract file to unsafe path: #{file.name}. Files may not traverse with `..`") unless LogStash::Util.name_safe?(file.name)

+# POSIX tar typeflag values: '1' = hard link, '2' = symbolic link
+              # https://pubs.opengroup.org/onlinepubs/9699919799/utilities/pax.html#tag_20_92_13_06
+              # The pathname of a link being created to another file, of any type, previously archived. This record shall override the linkname field in the following ustar header block(s). The following ustar header block shall determine the type of link created. If typeflag of the following header block is 1, it shall be a hard link. If typeflag is 2, it shall be a symbolic link and the linkpath value shall be the contents of the symbolic link. The pax utility shall translate the name of the link (contents of the symbolic link) from the encoding in the header to the character set appropriate for the local file system. When used in write or copy mode, pax shall include a linkpath extended header record for each link whose pathname cannot be represented entirely with the members of the portable character set other than NUL.
+              case entry.header.typeflag
+              when '2' then raise CompressError.new("Refusing to extract symlink entry: #{entry.full_name}")
+              when '1' then raise CompressError.new("Refusing to extract hardlink entry: #{entry.full_name}")
+              end

Conversation

andsel commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes

What does this PR do?

Uh oh!

github-actions bot commented Mar 11, 2026

🤖 GitHub comments

Uh oh!

mergify bot commented Mar 11, 2026

Uh oh!

andsel commented Mar 11, 2026

Uh oh!

andrewvc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewvc Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

yaauie Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

andrewvc Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andsel Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

andsel Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewvc Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

andrewvc Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

andrewvc Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

andsel commented Mar 13, 2026

Uh oh!

andsel commented Mar 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andsel commented Mar 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

andsel commented Mar 11, 2026 •

edited

Loading

andrewvc left a comment •

edited

Loading

andrewvc Mar 11, 2026 •

edited

Loading

andsel Mar 16, 2026 •

edited

Loading