[cleaner] refuse out-of-tree tar members on extract by ByteHackr · Pull Request #4461 · sosreport/sos

ByteHackr · 2026-08-21T15:17:14Z

sos clean used fully_trusted_filter plus a member-name-only path guard. That does not stop a symlink whose target is an absolute host path followed by a regular file with the same relative name, so extraction can write outside the dest directory.

Use PEP-706 data_filter when available and skip FilterError members. Keep in-tree relative symlinks.

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

Is the commit message split over multiple lines and hard-wrapped at 72 characters?
Is the subject and message clear and concise?
Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
Does the commit contain a Signed-off-by: First Lastname email@example.com?
Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
Are all passwords or private data gathered by this PR obfuscated?

packit-as-a-service · 2026-08-21T15:23:18Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo dnf install -y 'dnf*-command(copr)'
dnf copr enable packit/sosreport-sos-4461
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

pmoravec · 2026-08-21T20:27:10Z

+    data_filter = getattr(tarfile, 'data_filter', None)
+    if data_filter is not None:
+        try:
+            return data_filter(member, dest_path)


This sadly causes a regression on Python 3.12+ / whenever data_filter is present in tarfile class.

data_filter silently drops block devices and FIFOs, where esp. the block devices are legitimate members of a sosreport. While current sos clean keeps them in the archive.

Older Python versions would keep block devices and FIFOs, hence we would have an inconsistent behaviour of sos clean, depending on Python version used.

There is a legitimate question "oh, sos clean does not touch block device files, is that safe? Can't they contain some sensitive data to clean? Or.. isn't it safer to remove them..?" I am a noob on block devices, so I have no opinion how sos clean should handle them.

I think one potential fix of this comment is modifying the except tarfile.FilterError to something like:

except tarfile.FilterError as e: # data_filter rejects special files, but sosreports legitimately # contain them (collected from /dev, /proc, etc.). Re-validate # just the path/symlink safety and allow special files through. if member.isdev() or member.isfifo(): # Still validate the path is within destination dest_path_abs = os.path.abspath(dest_path) member_path = os.path.abspath(os.path.join(dest_path_abs, member.name)) if _path_is_within(dest_path_abs, member_path): return member # Safe special file, allow it # All other FilterError cases: reject return None

but that means some code duplication I dont like much..

So a "block/char device" in this context is purely a device special file inode: it just carries inode information (mode/ownership/maj/min/xattrs). Similarly for named FIFOs. They have historically been included (along with much of /dev) in the tarball because they carry potentially useful diagnostic information.

so I guess worth collecting them if they sit inside the archive directory, and ignore them if they are a target of a symlink outside archive.

pmoravec · 2026-08-21T20:45:00Z

+    data_filter = getattr(tarfile, 'data_filter', None)
+    if data_filter is not None:


This code (as well as dest_path = os.path.abspath(dest_path)) is redundantly re-evaluated for each and every member of the archive (typical sosreport contains a few tens of thousands of files). Please make these calls just once.

Potential implementation here: can't you nest the rest of the method inside a nested function, and have:

def _make_safer_extract_filter(dest_path): abs_dest = os.path.abspath(dest_path) data_filter = getattr(tarfile, 'data_filter', None) def filter_func(member, dest_path): # here is vast majority of the original method return filter_func

? (I dont insist on this specific idea, though)

pmoravec · 2026-08-21T20:46:39Z

+    directory = os.path.abspath(directory)
+    target = os.path.abspath(target)


These os.path.abspath calls are redundant - the method is called for arguments already "abspath-ed".

TurboTurtle · 2026-08-25T02:29:11Z

I have nothing more to add to @pmoravec's initial review

bmr-cymru

Aside from @pmoravec 's points, I wonder if for the cleaner use case we couldn't get away with using the predefined 'tar' filter?

Strip leading slashes (/ and os.sep) from filenames.
Refuse to extract files with absolute paths (in case the name is absolute even after stripping slashes, e.g. C:/foo on Windows). This raises AbsolutePathError.
Normalize filenames (TarInfo.name) that contain .. components using os.path.normpath(). Note that this removes internal .. components, which may change the meaning of the name if it traverses symbolic links.
Refuse to extract files whose absolute path (after following symlinks) would end up outside the destination. This raises OutsideDestinationError.
Clear high mode bits (setuid, setgid, sticky) and group/other write bits (S_IWGRP | S_IWOTH).

The problematic parts here seem to be the name normalisation (which would alter the symlink structure), and the high mode bits. Could this be acceptable for the clean use cases?

bmr-cymru · 2026-08-25T13:02:40Z

@@ -25,28 +25,73 @@
 # python older than 3.8 will hit a pickling error when we go to spawn a new
 # process for extraction if this method is a part of the SoSObfuscationArchive
 # class. So, the simplest solution is to remove it from the class.


This comment has become detached from the code it originally described

bmr-cymru · 2026-08-25T13:17:14Z

-            prefix = os.path.commonprefix([abs_directory, abs_target])
-            if prefix != abs_directory:
-                raise Exception(f"Attempted path traversal in tarfle"
-                                f"{prefix} != {abs_directory}")


This is the source of the current weakness; PEP706 rightly says: "When verifying members in advance, it may be necessary to track how each member would have changed the filesystem, e.g. how symlinks are being set up. This is hard. We can’t expect users to do it." (emph added).

pmoravec · 2026-08-27T10:42:17Z

Preliminary review reveals no issue. We might consider adding some logging when untar-ing rejects a file, not sure how worth it is.

I will review the PR further more today.

pmoravec

ACK from me. Unsure if it is worth to shout out (even as info/debug log) we are dropping extraction of some files outside the directory.

Hard to compare the PR to Bryn's proposal. It sounds to me similarly good as this one, per the description. Can it work for all supported Python versions? Will we get shorter or cleaner code?

Putting that discussion aside, for the matter of fixing the vulnerability, the PR sounds good to me.

bmr-cymru · 2026-08-27T17:22:13Z

@pmoravec agreed: my only outstanding request for this PR is to fix the comment position above the extract_archive() function. @ByteHackr could you push a revised commit to keep the comment with the code it describes?

With a fix & a regression test in place we can revisit this in future and possibly simplify things further.

pmoravec · 2026-08-27T19:31:25Z

+        self.assertTrue(found_link and os.path.islink(found_link))
+        self.assertEqual(os.readlink(found_link), 'motd')
+        os.remove(tar_path)
+


pylint complains:

tests/unittests/cleaner_tests.py:788:0: C0305: Trailing newlines (trailing-newlines)

pmoravec · 2026-08-27T19:31:48Z

+        handle = tempfile.NamedTemporaryFile(suffix='.tar', delete=False)
+        handle.close()


pylint complains:

tests/unittests/cleaner_tests.py:735:17: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)

sos clean used fully_trusted_filter plus a member-name-only path guard. That does not stop a symlink whose target is an absolute host path followed by a regular file with the same relative name, so extraction can write outside the dest directory. Use PEP-706 data_filter when available and skip FilterError members. Keep in-tree relative symlinks. Assisted-by: Cursor <https://cursor.com> Signed-off-by: Sandipan Roy <saroy@redhat.com>

pmoravec

LGTM.

Please @TurboTurtle or @arif-ali or @bmr-cymru for 2nd review, to let it merged.

sandrobonazzola · 2026-09-02T06:17:59Z

Looks like test is failing on Debian 12:

======================================================================
FAIL: test_absolute_symlink_does_not_write_outside_dest (tests.unittests.cleaner_tests.ExtractArchiveSafetyTests.test_absolute_symlink_does_not_write_outside_dest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/__w/sos/sos/sos/tests/unittests/cleaner_tests.py", line 763, in test_absolute_symlink_does_not_write_outside_dest
    self.assertFalse(
AssertionError: True is not false : extraction followed an out-of-tree symlink

[':0', ':2']
----------------------------------------------------------------------
Ran 172 tests in 4.357s

FAILED (failures=1)

Python 3.11.2 (Debian 12) ignores TarFile.extraction_filter, so the absolute-symlink TarSlip still wrote outside dest. Drop rejected members before extractall; keep the PEP-706 filter on 3.12+. Assisted-by: Cursor <https://cursor.com> Signed-off-by: Sandipan Roy <saroy@redhat.com>

ByteHackr force-pushed the cleaner-safe-extract branch from 919dc98 to 99d2698 Compare August 21, 2026 15:18

ByteHackr mentioned this pull request Aug 21, 2026

[cleaner] extract follows out-of-tree tar symlinks #4460

Open

pmoravec reviewed Aug 21, 2026

View reviewed changes

TurboTurtle added security Kind/cleaner cleaner component of sos Reviewed/Needs Iteration Review has been performed, change needs to be iterated on based on feedback before merge. labels Aug 25, 2026

ByteHackr force-pushed the cleaner-safe-extract branch from 99d2698 to 8824f4a Compare August 25, 2026 08:36

bmr-cymru reviewed Aug 25, 2026

View reviewed changes

pmoravec approved these changes Aug 27, 2026

View reviewed changes

pmoravec reviewed Aug 27, 2026

View reviewed changes

ByteHackr force-pushed the cleaner-safe-extract branch from 8824f4a to 850dcbd Compare September 1, 2026 03:48

pmoravec approved these changes Sep 1, 2026

View reviewed changes

		data_filter = getattr(tarfile, 'data_filter', None)
		if data_filter is not None:

		directory = os.path.abspath(directory)
		target = os.path.abspath(target)

		handle = tempfile.NamedTemporaryFile(suffix='.tar', delete=False)
		handle.close()

Uh oh!

Conversation

ByteHackr commented Aug 21, 2026

Uh oh!

packit-as-a-service Bot commented Aug 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TurboTurtle commented Aug 25, 2026

Uh oh!

bmr-cymru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmoravec commented Aug 27, 2026

Uh oh!

pmoravec left a comment

Choose a reason for hiding this comment

Uh oh!

bmr-cymru commented Aug 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmoravec left a comment

Choose a reason for hiding this comment

Uh oh!

sandrobonazzola commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants