diff --git a/Doc/library/tarfile.rst b/Doc/library/tarfile.rst index 62d67bc577c7d0..36f0643085c838 100644 --- a/Doc/library/tarfile.rst +++ b/Doc/library/tarfile.rst @@ -939,6 +939,13 @@ reused in custom filters: This implements the ``'fully_trusted'`` filter. + .. note:: + + Although the filter preserves all permission bits unchanged, the sticky + bit (:const:`~stat.S_ISVTX`) will not be set on an extracted file if the + file system does not allow it. For compatibility reasons, no exception is + raised in this case. + .. function:: tar_filter(member, path) Implements the ``'tar'`` filter. diff --git a/Lib/tarfile.py b/Lib/tarfile.py index 726f9f50ba2e72..b5fe9cb71a7b07 100755 --- a/Lib/tarfile.py +++ b/Lib/tarfile.py @@ -37,6 +37,7 @@ # Imports #--------- from builtins import open as bltn_open +import errno import sys import os import io @@ -2567,9 +2568,31 @@ def chmod(self, tarinfo, targetpath): if tarinfo.mode is None: return try: - os.chmod(targetpath, tarinfo.mode) - except OSError as e: - raise ExtractError("could not change mode") from e + try: + os.chmod(targetpath, tarinfo.mode) + except OSError as exc1: + # gh-108948: On FreeBSD, chmod() fails when trying to set the + # sticky bit on a file as non-root. On other platforms, the bit + # is silently ignored if it cannot be set. We make FreeBSD + # behave like other platforms by catching the error and trying + # again without the sticky bit. + if (hasattr(errno, "EFTYPE") and exc1.errno == errno.EFTYPE + and (tarinfo.mode & stat.S_ISVTX)): + try: + # Retry without the sticky bit + os.chmod(targetpath, tarinfo.mode & ~stat.S_ISVTX) + except OSError as exc2: + # The error from the second attempt is the direct cause + # of the ExtractError, but we keep the original error + # around for good information. + raise exc2 from exc1 + else: + # The second chmod() without sticky bit succeeded + pass + else: + raise + except OSError as exc3: + raise ExtractError("could not change mode") from exc3 def utime(self, tarinfo, targetpath): """Set modification time of targetpath according to tarinfo. diff --git a/Lib/test/support/os_helper.py b/Lib/test/support/os_helper.py index 821a4b1ffd5077..326dd0ebdadb0f 100644 --- a/Lib/test/support/os_helper.py +++ b/Lib/test/support/os_helper.py @@ -275,6 +275,39 @@ def skip_unless_working_chmod(test): return test if ok else unittest.skip(msg)(test) +_can_chmod_set_sticky_bit = None + + +def can_chmod_set_sticky_bit(): + """Return True if chmod allows to set the sticky bit on a regular file. + + Return False if chmod fails with an error, or it succeeds but does not set + the sticky bit. + """ + + def can_chmod_set_sticky_bit_inner(): + if not can_chmod(): + return False + filename = TESTFN + try: + with open(filename, "w") as f: + # assume it won't fail because can_chmod() returned True: + os.chmod(f.fileno(), 0o666) + try: + os.chmod(f.fileno(), 0o666 | stat.S_ISVTX) + except OSError: + return False + mode = os.stat(f.fileno()).st_mode + return bool(mode & stat.S_ISVTX) + finally: + unlink(filename) + + global _can_chmod_set_sticky_bit + if _can_chmod_set_sticky_bit is None: + _can_chmod_set_sticky_bit = can_chmod_set_sticky_bit_inner() + return _can_chmod_set_sticky_bit + + # Check whether the current effective user has the capability to override # DAC (discretionary access control). Typically user root is able to # bypass file read, write, and execute permission checks. The capability diff --git a/Lib/test/test_tarfile.py b/Lib/test/test_tarfile.py index 67009a3d2e9c24..382932cf7ff00d 100644 --- a/Lib/test/test_tarfile.py +++ b/Lib/test/test_tarfile.py @@ -9,6 +9,7 @@ import re import warnings import stat +import errno import unittest import unittest.mock @@ -3056,6 +3057,69 @@ def test_keyword_only(self, mock_geteuid): tarfl.extract, filename_1, TEMPDIR, False, True) +@os_helper.skip_unless_working_chmod +class ExtractStickyFileTest(unittest.TestCase): + + # to use in mock tests, on platforms that don't have it + fake_EFTYPE = getattr(errno, "EFTYPE", max(errno.errorcode) + 100) + + def setUp(self): + self.test_dir = os.path.join(TEMPDIR, "chmod") + os.mkdir(self.test_dir) + self.addCleanup(os_helper.rmtree, self.test_dir) + self.sticky_mode = "-rwxrwxrwt" + self.non_sticky_mode = "-rwxrwxrwx" + + def extract_sticky_file(self, file_name="sticky"): + with ArchiveMaker() as arc: + arc.add(file_name, mode=self.sticky_mode) + with arc.open(errorlevel=2) as tar: + tar.extract(file_name, self.test_dir, filter="fully_trusted") + self.extracted_path = os.path.join(self.test_dir, file_name) + + def test_extract_chmod_eftype_success(self): + # Extracting a file as non-root should skip the sticky bit (gh-108948) + # even on platforms where chmod fails with EFTYPE (i.e. FreeBSD) + if os_helper.can_chmod_set_sticky_bit(): + expected_mode = self.sticky_mode + else: + expected_mode = self.non_sticky_mode + self.extract_sticky_file() + got_mode = stat.filemode(os.stat(self.extracted_path).st_mode) + self.assertEqual(got_mode, expected_mode) + + @unittest.mock.patch.object(errno, "EFTYPE", fake_EFTYPE, create=True) + def test_extract_chmod_eftype_mock(self): + # Like test_extract_chmod_eftype_success but mock chmod so we can + # explicitly test the case we want, regardless of the OS. + with unittest.mock.patch("os.chmod") as mock: + eftype_error = OSError(errno.EFTYPE, "EFTYPE") + mock.side_effect = [eftype_error, None] + self.extract_sticky_file() + self.assertTrue(mock.call_count, 2) + _, chmod_mode1 = mock.call_args_list[0].args + _, chmod_mode2 = mock.call_args_list[1].args + self.assertTrue(chmod_mode1 & stat.S_ISVTX) + self.assertFalse(chmod_mode2 & stat.S_ISVTX) + self.assertEqual(chmod_mode1 | chmod_mode2, chmod_mode1) + + @unittest.mock.patch.object(errno, "EFTYPE", fake_EFTYPE, create=True) + def test_extract_chmod_eftype_error(self): + # Take care that any other error is preserved in the EFTYPE case. + # It's hard to create a situation where chmod() fails with EFTYPE and, + # retrying without the sticky bit, it fails with a different error. But + # we can mock it, so we can exercise all branches. + with unittest.mock.patch("os.chmod") as mock: + eftype_error = OSError(errno.EFTYPE, "EFTYPE") + other_error = OSError(errno.EPERM, "different error") + mock.side_effect = [eftype_error, other_error] + with self.assertRaises(tarfile.ExtractError, + msg="could not change mode") as excinfo: + self.extract_sticky_file() + self.assertEqual(excinfo.exception.__cause__, other_error) + self.assertEqual(excinfo.exception.__cause__.__cause__, eftype_error) + + class ReplaceTests(ReadTest, unittest.TestCase): def test_replace_name(self): member = self.tar.getmember('ustar/regtype') @@ -3816,8 +3880,9 @@ def test_modes(self): tmp_filename = os.path.join(TEMPDIR, "tmp.file") with open(tmp_filename, 'w'): pass - new_mode = (os.stat(tmp_filename).st_mode - | stat.S_ISVTX | stat.S_ISGID | stat.S_ISUID) + new_mode = os.stat(tmp_filename).st_mode | stat.S_ISGID | stat.S_ISUID + if os_helper.can_chmod_set_sticky_bit(): + new_mode |= stat.S_ISVTX os.chmod(tmp_filename, new_mode) got_mode = os.stat(tmp_filename).st_mode _t_file = 't' if (got_mode & stat.S_ISVTX) else 'x' diff --git a/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst b/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst new file mode 100644 index 00000000000000..1d1dcb12356524 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst @@ -0,0 +1 @@ +:mod:`tarfile`: On FreeBSD, ``tarfile`` will not attempt to set the sticky bit on extracted files when it's not possible, matching the behavior of other platforms.