gh-108948: tarfile should handle sticky bit in FreeBSD

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.

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.

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

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'

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.