From 7e11610bc0b392bc88397410b5481d2709a1abcd Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 29 Oct 2026 03:47:09 +0000 Subject: [PATCH] gh-160863: tarfile: Judge link members by where the link is made gh-160097 gave tar_filter the "link at the destination" check that data_filter had, but made it lexical (the member's normalised name is '.'), so link members that reach the destination through '..' or symlinks passed even filter='data': * '../dest' now raised RecursionError instead of OutsideDestinationError; * 's' -> '.' followed by a hard link 's/.' changed the destination directory's own mode; * '../current', where 'current' is an existing symlink outside the destination pointing at it, deleted that symlink and wrote later members outside the destination. A link is made at its own path without following whatever is already there, so check that path with only its parent directory resolved, and require it to be strictly inside the destination. This covers all of the above for both filters, also rejects a link whose parent is reached through an earlier symlink member leading outside, and avoids the false positive of the old resolved-path check when re-extracting an archive containing a symlink to '.'. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019EeY1aVDfL7Y3eMxYWdYgc --- Doc/library/tarfile.rst | 7 +- Lib/tarfile.py | 19 +++-- Lib/test/test_tarfile.py | 76 +++++++++++++++++++ ...-10-29-12-00-00.gh-issue-160863.zfGUag.rst | 5 ++ 4 files changed, 100 insertions(+), 7 deletions(-) create mode 100644 Misc/NEWS.d/next/Security/2026-10-29-12-00-00.gh-issue-160863.zfGUag.rst diff --git a/Doc/library/tarfile.rst b/Doc/library/tarfile.rst index 06eba01d21e5261..a28d8b906979661 100644 --- a/Doc/library/tarfile.rst +++ b/Doc/library/tarfile.rst @@ -1132,7 +1132,9 @@ reused in custom filters: path (after following symlinks) would end up outside the destination. This raises :class:`~tarfile.OutsideDestinationError`. - :ref:`Refuse ` to extract links (hard or soft) - whose name refers to the destination directory itself. + that would be made at the destination directory itself or outside it. + Only the directories leading to the link are resolved: a link replaces + whatever is at its own path rather than following it. This raises :class:`~tarfile.OutsideDestinationError`. Note that such files are refused even on platforms that do not support symbolic links. @@ -1152,6 +1154,9 @@ reused in custom filters: Filenames containing ``..`` components are now normalized. + Links that would be made at the destination directory itself, or + at an existing symbolic link outside it, are now refused. + .. function:: data_filter(member, path) Implements the ``'data'`` filter. diff --git a/Lib/tarfile.py b/Lib/tarfile.py index 57bba1bba54db35..ea205e0c1a56561 100644 --- a/Lib/tarfile.py +++ b/Lib/tarfile.py @@ -854,12 +854,19 @@ def _get_filtered_attrs(member, dest_path, for_data=True): strict=os.path.ALLOW_MISSING) if os.path.commonpath([target_path, dest_path]) != dest_path: raise OutsideDestinationError(member, target_path) - # A link member named after the destination directory itself - # would replace it with a (sym)link, redirecting the destination - # for all subsequent members. - if ((member.islnk() or member.issym()) - and os.path.normpath(name) == os.curdir): - raise OutsideDestinationError(member, target_path) + if member.islnk() or member.issym(): + # A link is made at its own path without following a link already + # there (that is unlinked first), so judge it by where it is made: + # resolve only the parent directory. The link must land strictly + # inside the destination; one that replaced the destination itself + # would redirect all subsequent members. + parent, leaf = os.path.split(name.rstrip('/' + os.sep)) + parent = os.path.realpath(os.path.join(dest_path, parent), + strict=os.path.ALLOW_MISSING) + link_path = os.path.normpath(os.path.join(parent, leaf)) + if (os.path.commonpath([link_path, dest_path]) != dest_path + or os.path.normcase(link_path) == os.path.normcase(dest_path)): + raise OutsideDestinationError(member, link_path) # Limit permissions (no high bits, and go-w) mode = member.mode if mode is not None: diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index 639fd0449a989ab..1ca58a95d76ef80 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -4676,6 +4676,82 @@ def test_link_at_destination(self): with self.check_context(arc.open(), filter): self.expect_exception(tarfile.OutsideDestinationError) + @symlink_test + @os_helper.skip_unless_symlink + def test_link_reaching_destination(self): + # gh-160863: link members whose names are not lexically '.' but + # that would still be made at the destination directory itself. + def via_parent_name(arc): + arc.add('../dest', symlink_to='dest') + + def via_symlink_to_dot(arc): + arc.add('d/') + arc.add('s', symlink_to='.') + arc.add('s/.', hardlink_to='d') + + def via_symlink_to_dot_symlink(arc): + arc.add('s', symlink_to='.') + arc.add('s/.', symlink_to='.') + + for make in (via_parent_name, via_symlink_to_dot, + via_symlink_to_dot_symlink): + with ArchiveMaker() as arc: + make(arc) + for filter in 'tar', 'data': + with self.subTest(make.__name__, filter=filter): + with self.check_context(arc.open(), filter): + self.expect_exception(tarfile.OutsideDestinationError) + + @symlink_test + @os_helper.skip_unless_symlink + def test_link_replacing_outer_symlink(self): + # gh-160863: a link member must not replace an existing symlink + # outside the destination, even one that points into it. + with ArchiveMaker() as arc: + arc.add('first.txt', content='1') + arc.add('../current', symlink_to='real') + arc.add('later.txt', content='2') + for filter in 'tar', 'data': + with self.subTest(filter=filter), \ + os_helper.temp_dir(self.outerdir): + real = self.outerdir / 'real' + current = self.outerdir / 'current' + real.mkdir() + current.symlink_to('real') + with arc.open() as tar: + with self.assertRaises(tarfile.OutsideDestinationError): + tar.extractall(current, filter=filter) + self.assertTrue(current.is_symlink()) + self.assertEqual(os.listdir(real), ['first.txt']) + + # The link's parent directory may also be reached through a + # symlink made earlier in the same archive. + with ArchiveMaker() as arc: + arc.add('s', symlink_to='..') + arc.add('s/outer', symlink_to='dest') + with os_helper.temp_dir(self.outerdir): + outer = self.outerdir / 'outer' + outer.symlink_to('dest') + with arc.open() as tar: + with self.assertRaises(tarfile.OutsideDestinationError): + tar.extractall(self.destdir, filter='tar') + self.assertTrue(outer.is_symlink()) + + @symlink_test + @os_helper.skip_unless_symlink + def test_link_to_dot_reextracted(self): + # A symlink to '.' resolves to the destination once it exists; + # extracting the same archive again must still replace it. + with ArchiveMaker() as arc: + arc.add('self', symlink_to='.') + for filter in 'tar', 'data': + with self.subTest(filter=filter), \ + os_helper.temp_dir(self.outerdir): + for _ in range(2): + with arc.open() as tar: + tar.extractall(self.destdir, filter=filter) + self.assertEqual(os.readlink(self.destdir / 'self'), '.') + @symlink_test @os_helper.skip_unless_symlink def test_empty_name_symlink_chain(self): diff --git a/Misc/NEWS.d/next/Security/2026-10-29-12-00-00.gh-issue-160863.zfGUag.rst b/Misc/NEWS.d/next/Security/2026-10-29-12-00-00.gh-issue-160863.zfGUag.rst new file mode 100644 index 000000000000000..e68de2463eb9f49 --- /dev/null +++ b/Misc/NEWS.d/next/Security/2026-10-29-12-00-00.gh-issue-160863.zfGUag.rst @@ -0,0 +1,5 @@ +:func:`tarfile.tar_filter` and :func:`tarfile.data_filter` now judge a link +member by where the link would be made, resolving only its parent directory. +This rejects links that reach the destination directory, or an existing +symbolic link outside it, through ``..`` components or earlier symlink +members, which :gh:`160097` had allowed past the ``'data'`` filter.