gh-108948: tarfile should handle sticky bit in FreeBSD #108950
Conversation
|
I think this should be backported to 3.12 and 3.11 but it would be useful if #107902 is backported first. |
There was a problem hiding this comment.
I would prefer to keep the first exception and ignore the second chmod() exception:
def chmod(self, tarinfo, targetpath):
"""Set file permissions of targetpath according to tarinfo.
"""
if tarinfo.mode is None:
return
try:
try:
os.chmod(targetpath, tarinfo.mode)
except OSError as exc:
if not(hasattr(errno, "EFTYPE") and e.errno == errno.EFTYPE):
raise
# gh-108948: On FreeBSD, chmod() fails when trying to set the
# sticky bit on a file as a normal user. It's a noop in most
# other platforms.
try:
os.chmod(targetpath, tarinfo.mode & ~stat.S_ISVTX)
except OSError:
# re-raise the first chmod() exception,
# ignore the second chmod() exception.
raise exc from None
else:
# second chmod() succeeded
pass
except OSError:
raise ExtractError("could not change mode")Enjoy the 3 levels of nested try/exception :-)
|
Will the first Also, the patch as-is doesn't check for root -- if the chmod is failing for the root user, there should be no need to try (and possibly succeed) without the sticky-bit. |
Concerns about failures as root user.
Well, if you care about keeping all information, the 3 exceptions can easily be chained ;-) Maybe that the most accurate information ;-) |
The first exception within the check is inevitably Note: I checked what BSD tar does. When running as non-root, it defaults to ignore "SGID, SUID, sticky bit, file attributes or file flags, extended file attributes and ACLs", with an option to reverse the behavior. When enabling this option (default on root) it indeed fails with an error similar to
The retry only happens on EFTYPE. Among the supported platforms, I think only FreeBSD sets this error. It's documented to only happen on one very specific case:
If we are to trust this, EFTYPE already implies non-root. The only issue I see is that this is platform-specific and some other system might use EFTYPE to mean something different. I don't mind adding a safeguard, be it against euid or sys.platform, if you think it's useful. |
|
I was trying to add a test case to cover the branch where the two exceptions are chained. But I cannot think of a way to make chmod fail that wouldn't trigger an error before getting to chmod. Opening the file for write is enough to fall into any of the error cases, except for EFTYPE itself, and the EPERM one related to groups which we can't test in general because it needs a specific setup. Good news is that this is such a corner case that it wouldn't matter if the traceback is not perfectly clear, and I feel we are already doing more than enough. |
|
@sorcio, thank you for doing the extra research! LGTM. |
You can mock the os.chmod() function for the duration of your test, so you can control exactly how it works. |
|
Thanks for the review feedback @vstinner! I applied all your suggestions and replied to some of your comments. I could merge / rebase main to add the FreeBSD CI runner, since it's one of those PRs where it's directly useful. But I haven't yet because Cirrus is blocking new runs for the month. |
There was a problem hiding this comment.
@encukou: Oh good, it seems like filter='data' already clears the sticky bit in _get_filtered_attrs():
# Strip high bits & group/other write bits
mode = mode & 0o755Is it a new functional test needed to ensure that the sticky bit is ignored with filter='data'?
Lib/tarfile.py
Outdated
| except OSError as e: | ||
| raise ExtractError("could not change mode") from e |
There was a problem hiding this comment.
| except OSError as e: | |
| raise ExtractError("could not change mode") from e | |
| except OSError as exc3: | |
| raise ExtractError("could not change mode") from exc3 |
Lib/test/test_tarfile.py
Outdated
| @@ -690,6 +691,36 @@ def format_mtime(mtime): | |||
| tar.close() | |||
| os_helper.rmtree(DIR) | |||
|
|
|||
| @unittest.skipUnless(hasattr(errno, "EFTYPE"), "errno.EFTYPE required") | |||
There was a problem hiding this comment.
Would it make sense to mock this constant to test the code on other platforms?
There was a problem hiding this comment.
I decided to go with it. Maybe it's overkill for such a niche case, but more tests rarely hurt. All new tests never skip.
Lib/test/test_tarfile.py
Outdated
| self.addCleanup(os_helper.rmtree, DIR) | ||
| with tar: | ||
| # this should not raise: | ||
| tar.extract("sticky1", DIR, filter="fully_trusted") |
There was a problem hiding this comment.
I think that you should split this test in two tests:
- a method which doesn't mock anything and jsut check that tarfile works as expected: this line. -- functional test
- another method which mocks chmod() to test the complicated EFTYPE code path.
There was a problem hiding this comment.
Done. Again, maybe overkill, but it's three tests now.
Lib/test/test_tarfile.py
Outdated
| other_error = OSError(errno.EPERM, "different error") | ||
| mock.side_effect = [eftype_error, other_error] | ||
| with self.assertRaises(tarfile.ExtractError) as excinfo: | ||
| tar.extract("sticky2", DIR, filter="fully_trusted") |
There was a problem hiding this comment.
Since tarfile implementation now uses a complicated triple-nested try block, would it be possible to test also the othe code path: EFTYPE error but then the second chmod() succeed.
There was a problem hiding this comment.
I think this is done now.
| @@ -0,0 +1 @@ | |||
| On FreeBSD, :mod:`tarfile` will not attempt to set the sticky bit on extracted files when it's not possible, matching the behavior of other platforms. | |||
There was a problem hiding this comment.
Hum. I suggest to explain it differently.
:mod:`tarfile`: On FreeBSD, if the sticky bit cannot be set,
if the user is not root for example, clear the sticky bit,
matching the behavior of other platforms.
There was a problem hiding this comment.
Maybe it would be worth it to add a note about the sticky bit in https://docs.python.org/dev/library/tarfile.html#tarfile.TarFile.extract documentation?
There was a problem hiding this comment.
I went with Ethan's suggestion for the blurb.
I also added a note, but under the filters documentation, so if a user searches for "sticky" or "bits" or similar keywords they find relevant information right there. I can change it or add more, of course.
Misc/NEWS.d/next/Library/2023-09-05-17-11-17.gh-issue-108948.HL1Ttw.rst
Outdated
Show resolved
Hide resolved
|
The macOS runner failure is a funny bit of history because in the documentation for chmod they still state that "The sticky bit may only be set by the super user on shareable executable files" but in fact that's not true anymore. You can set the bit on any file as unprivileged user (assuming the file system supports it) and it has no special meaning. One free bit per file, in case you needed extra storage. As far as I can gather, this has been the case since OS X Tiger (10.4, 2005) and the last mention I see of a similar check was in the previous release where, apparently, it used to return EFTYPE :) Nobody ever bothered to fix the ancient BSD documentation, and we get the EFTYPE constant in the headers as a remnant, because I don't think it's used in any way in user space code. Anyway, I will need to add more branching and possibly double check what other BSDs are doing. |
|
Why is the test executed so many times? It cannot be defined somewhere else to only be executed once? For macOS, modify the test to check mode expected on macOS, no? |
Many/most of the tarfile tests are run once per compression format. But it's good that you asked the question because I've realized that I'm not using the compression format at all in the new test case. I had put it in a different place at first, then moved it closer to other similar tests, but forgot to adapt it. I will move it again.
Sure! I haven't decided yet how to keep the test readable, but I'll come up with something. The note was mostly for historical/fun purposes, and to write it down somewhere before I forget :) |
…L1Ttw.rst Co-authored-by: Ethan Furman <ethan@stoneleaf.us>
test_extract_chmod_eftype was run once per testtar.* file but it did not use those files
|
Following the insightful feedback from @vstinner I decided to be a lot less minimalist with my approach to testing. The fix itself is basically the same, but it's overly tested now. The mock tests make me a bit nervous because of course they depend on implementation details (when and how os.chmod gets called) but there are other tests. About the helper function I added: is it a bad idea? Does it belong in As a reminder:
|
Doc/library/tarfile.rst
Outdated
|
|
||
| Although the filter preserves all permission bits unchanged, the sticky | ||
| bit (:const:`~stat.S_ISVTX`) will not be set on extracted files when the | ||
| system does not allow it. For compatibility reasons, no error is raised |
There was a problem hiding this comment.
| system does not allow it. For compatibility reasons, no error is raised | |
| system does not allow it for the current user. For compatibility reasons, no error is raised |
There was a problem hiding this comment.
Uh, yeah, it's right that running as root will set the sticky bit in the cases we are testing. But I don't want to suggest it's a permissions issue (which would likely raise EPERM), or to imply more than is necessary. E.g. I think file system implementations on EED3 Linux are allowed to silently drop the bit if they don't support it regardless of the user. What do you think is worth clarifying in this context?
There was a problem hiding this comment.
You can ignore my suggestion if you think that it's irrelevant.
There was a problem hiding this comment.
Rephrased ever so slightly for readability, but I avoided the "for the current user" part because it feels out of place.
Lib/test/test_tarfile.py
Outdated
| mode = os.stat(f.fileno()).st_mode | ||
| return bool(mode & stat.S_ISVTX) | ||
| finally: | ||
| os.unlink(filename) |
There was a problem hiding this comment.
It's safer to use os_helper.unlink() here.
Lib/test/test_tarfile.py
Outdated
|
|
||
|
|
||
| def can_chmod_set_sticky(): | ||
| """Can chmod set the sticky bit on a file?""" |
There was a problem hiding this comment.
Please explain in the docstring that it returns False if chmod() doesn't fail but ignores the sticky bit.
Lib/test/test_tarfile.py
Outdated
| def can_chmod_set_sticky_inner(): | ||
| if not os_helper.can_chmod(): | ||
| return False | ||
| filename = os_helper.TESTFN + "-chmod-sticky" |
There was a problem hiding this comment.
honestly, just use "chmod-sticky", to avoid further troubles with non-ASCII filenames.
There was a problem hiding this comment.
Decided not to do it, and instead just use TESTFN like can_chmod() does. The concern is fair but I don't want to touch os_helper more than necessary.
Lib/test/test_tarfile.py
Outdated
| _can_chmod_set_sticky = None | ||
|
|
||
|
|
||
| def can_chmod_set_sticky(): |
There was a problem hiding this comment.
You should move it to support.os_helper.
I suggest to rename to can_chmod_set_sticky_bit().
|
I am not a tarfile expert, nor do I want to become one. My contribution -- filters -- was limited in scope. It did introduce tests that revealed some surprises, and I'm happy to modify the tests to work on OSs we don't formally support. With that out of the way: I'm wary of this change. Adding OS-specific chunks of code to tarfile makes it less maintainable and predictable.
But again: I'm not the expert. If another core dev wants to maintain the retry, go ahead. @sorcio, thank you for your work, and sorry that I didn't catch this before you made the PR. |
|
@encukou no worries, and thanks a lot for the detailed feedback! I had a hunch there could be a problem with this change, and I regret not bringing it up more explicitly (I don't care that much about the behavior, and I just wanted tests to pass on FreeBSD). The difficulty documenting the change in an unambiguous way should have been a red flag. I don't think chmod will ever have such an option (I doubt it would be a good idea). The issue is that this is an obscure difference that depends on OS and file system implementation. It's pretty opaque from the point of view of a high level library. So I thought that smoothing it out at the library level seemed a way to make it nicer to users. Your comment on backwards compatibility is what settles it for me. It seems unlikely that a program ever depended on this behavior; but if it did depend on it, the proposed change would be a nightmare to debug. And if a bug is caused by this, extraction filters are the solution available to users. I can change the PR to only modify the original failing test (TestExtractionFilters.test_modes) so that it succeeds on FreeBSD, if the new helper is still considered useful. Otherwise I'm also happy to drop it :) |
There was a problem hiding this comment.
Sorry, this PR took longer what I expected, since it's a core feature of tarfile, and a security sensitive topic. But I prefer to take my time :-) You enhanced the doc and the tests, that's great!
Here is I hope my last review :-) We should be good to merge soon.
|
|
||
| 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() |
There was a problem hiding this comment.
Hum, I suggest you to mimick code around in os_helper.py: other "can" functions also have a cache (global variable), but they don't use an "inner" function.
| 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): |
There was a problem hiding this comment.
On most platforms, this test willl be run without EFTYPE. I suggest to remove it:
| def test_extract_chmod_eftype_success(self): | |
| def test_extract_chmod_success(self): |
| self.assertEqual(got_mode, expected_mode) | ||
|
|
||
| @unittest.mock.patch.object(errno, "EFTYPE", fake_EFTYPE, create=True) | ||
| def test_extract_chmod_eftype_mock(self): |
There was a problem hiding this comment.
| def test_extract_chmod_eftype_mock(self): | |
| def test_extract_mock_chmod_eftype_success(self): |
| @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. |
There was a problem hiding this comment.
I would prefer to describe which code path is tested:
Test chmod() failing with OSError(EFTYPE) and then pass when re-run without the sticky bit.
| 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) |
There was a problem hiding this comment.
| # Extracting a file as non-root should skip the sticky bit (gh-108948) | |
| # gh-108948: Extracting a file as non-root should skip the sticky bit |
| 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): |
There was a problem hiding this comment.
| def test_extract_chmod_eftype_error(self): | |
| def test_extract_mock_chmod_eftype_error(self): |
|
|
||
| @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. |
There was a problem hiding this comment.
I suggest to add first (but preserve your longer comment):
Test chmod() failing with OSError(EFTYPE) and then raise PermissionError() when re-run without the sticky bit.
| self.assertEqual(excinfo.exception.__cause__, other_error) | ||
| self.assertEqual(excinfo.exception.__cause__.__cause__, eftype_error) |
There was a problem hiding this comment.
| self.assertEqual(excinfo.exception.__cause__, other_error) | |
| self.assertEqual(excinfo.exception.__cause__.__cause__, eftype_error) | |
| self.assertIs(excinfo.exception.__cause__, other_error) | |
| self.assertIs(excinfo.exception.__cause__.__cause__, eftype_error) |
|
Hey @vstinner, thanks again for the review feedback! Can we have a decision on #108950 (comment) before we go on? |
A sticky bit is a valid bit. The problem is that on FreeBSD, a regular user is not allowed to set it. Example: On Linux, I get: On FreeBSD, I get: where 79 is On FreeBSD, if I run the script as root, it works as expected: Now, if I create a tar archive, remove the file, and decompress the archive as a regular user... oh! the sticky bit is gone! This behavior is specific to the tar command. Also, if I decompress the archive as root, I get the sticky bit: If I look at syscalls calls by tar as a regular user, I get: The file The tar program on FreeBSD comes from the libarchive project. libarchive clears bits unless /*
* User didn't request full permissions, so don't
* restore SUID, SGID bits and obey umask.
*/
a->mode &= ~S_ISUID;
a->mode &= ~S_ISGID;
a->mode &= ~S_ISVTX;
a->mode &= ~a->user_umask;So libarchive implementation is different: it doesn't catch EFTYPE. It doesn't call chmod() with S_ISVTX. It clears S_ISVTX before calling chmod() unless |
|
Oh, Linux tar also clears the sticky bit: The sticky bit is only preserved if asked explicitly: |
|
While this PR mimicks Linux tar and FreeBSD tar, I'm no longer sure that trying chmod() with the sticky bit, catch EFTYPE error, and retry chmod() without the sticky mode, is the right approach. Maybe Python tarfile should move closer to FreeBSD tar (libarchive) and Linux tar (GNU tar) behavior: clear S_ISUID, S_ISGID and S_ISVTX bits, and respect the user umask, unless -p / --preserve-permissions / --same-permissions option is used. Problem: that would be a backward incompatible change, and currently Python doesn't have a --preserve-permissions option. To unblock the FreeBSD buildbot, I suggest a trivial change: PR #109697 skips test_modes() if chmod() fails with EFTYPE. I'm not sure that fixing test_tarfile on FreeBSD should drive tarfile design in general. |
Maybe I misunderstand you, but this is what extraction filters do. As I said, I don't think this change is a good idea anymore, and I'm sorry I didn't give it more thought before. Skipping the test on EFTYPE is fine. Maybe even better is to only skip setting the sticky bit, and not the whole test case. |
After testing FreeBSD tar and Linux tar, reading your PR and took time to think about the issue, I think that yes, EFTYPE is the expected behavior for FreeBSD tar and Linux tar both uses something like Python Changing the default filter should be discussed separately. I now think that skipping the test if chmod() cannot set the sticky bit is the right test_tarfile fix. I close your issue. Thanks for spending time to investigate this complicate issue! Thanks @encukou for your feedback! |
Instead of failing on FreeBSD, we emulate the behavior of other platforms. That means that if a file wants to set the sticky bit and we are not root, we just ignore it.