gh-113659: Skip hidden .pth files #113660
Conversation
serhiy-storchaka
commented
Jan 2, 2024
•
serhiy-storchaka
commented
- Issue: Security risk of hidden pth files #113659
serhiy-storchaka
added
type-security
|
!buildbot BSD |
|
🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit 7da1ed4 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
There was a problem hiding this comment.
Not looking at hidden pth files looks like a clear security improvement to me. I'll leave it to wiser heads than me if it is enough of an improvement to risk breaking anyone using this intentionally for harmless reasons.
BTW. I noticed some flags missing in stat.py, will file an issue about those :-)
| try: | ||
| st = os.lstat(fullname) | ||
| except OSError: | ||
| return |
There was a problem hiding this comment.
I'm slightly worried about the performance impact of calling stat on all .pth files. pip install -e will install a .pth file for every installed editable package.
On the other hand, the overhead should be neglectable as the inode needs to be read in memory anyway to actually read the contents.
| st = os.lstat(fullname) | ||
| except OSError: | ||
| return | ||
| if ((getattr(st, 'st_flags', 0) & stat.UF_HIDDEN) or |
There was a problem hiding this comment.
Which platforms support UF_HIDDEN?
On macOS the flag is a hint that's ignored by command-line tools.
E.g.:
ronald@Pelargir[0]% touch foo.txt
[~/Projects/Forks/cpython/build/X]
ronald@Pelargir[0]% chflags hidden foo.txt
[~/Projects/Forks/cpython/build/X]
ronald@Pelargir[0]% ls -l (gh-53502-long-times)cpython
total 0
-rw-r--r-- 1 ronald staff 0 Jan 2 21:10 foo.txt
ronald@Pelargir[0]% ls -lO
total 0
-rw-r--r-- 1 ronald staff hidden 0 Jan 2 21:10 foo.txtThis matches the documentation in sys/stat.h:
#define UF_HIDDEN 0x00008000 /* hint that this item should not be */
/* displayed in a GUI */
Co-authored-by: Ronald Oussoren <ronaldoussoren@mac.com>
|
!buildbot BSD |
|
🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit dfbc6ef 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
|
Could you please check what effect has setting UF_HIDDEN on macOS GUI? I am especially curious about symlinks:
|
A file with UF_HIDDEN is not shown at all.
The symlink is not shown in the GUI, the target is
The symlink is visible in the GUI, the target is not visible. I can open the hidden file through the link.
Neither are shown in the the GUI. And for completeness sake: A "." prefixed file is not shown in the GUI. And finally... there's another way to hide files, although I don't know if there's a convenient API for doing this: The "~/Library" directory is hidden, and this is accomplished by a finder attribute (kMDItemFSInvisible == True). That's something I learned while researching this, t.b.h. I was convinced hidden the user library folder was something hardcoded in the file manager (Finder). I don't think it is worthwhile to try to detect this in site.py, doing that requires exposing APIs we currently don't expose (https://github.com/RhetTbull/osxmetadata is a project that can show this information) 
|
|
One thing to consider here is the threat model for this. The most important risk is (IMHO) hidden pth files installed though pip and similar tools. AFAIK those tools currently don't have a way to set flags like UF_HIDDEN, let alone finder attributes on macOS. Both tarfile and zipfile currently don't support st_flags (from a cursory inspection), and IIRC the ZIP spec also doesn't have support for this. That said, the code to deal with UF_HIDDEN flags is simple enough to just do the check even if this only protects against more complicated scenario's. |
Then we perhaps need to check also flags of the target. On Windows only file attributes of the symlink have effect. |
I didn't pay enough attention while editing my response (no blank line between the quoted text and my response), it should be clearer now.
It's the same on macOS, but that wasn't clear in my response. No need to check the symlink target. If the symlink does not have the UF_HIDDEN flag it will be shown in the GUI, regardless of the flag on the target. Sorry about my confusing reply. |
It is a great relief. |
This reverts commit 59ee5f3.
| try: | ||
| pth_file.create() | ||
| site.addsitedir(pth_file.base_dir, set()) | ||
| self.assertNotIn(site.makepath(pth_file.good_dir_path)[0], sys.path) |
There was a problem hiding this comment.
Maybe mock site.addpackage() and check that it's not called, rather than checking effects of site.addpackage() implementation. Same remark for the other added tests.
There was a problem hiding this comment.
It is not called for dotfiles, but is called for files with hidden attribute. It is rather an implementation detail where we add a filter.
| @@ -168,6 +169,14 @@ def addpackage(sitedir, name, known_paths): | |||
| else: | |||
| reset = False | |||
| fullname = os.path.join(sitedir, name) | |||
| try: | |||
| st = os.lstat(fullname) | |||
| except OSError: | |||
There was a problem hiding this comment.
Hum, maybe add a _trace() call to help debugging such issue. Something like:
_trace(f"Skipping .pth file, stat failed: {exc!r}")
There was a problem hiding this comment.
When open() fails below, it is simply silently returns.
If we want to add a _trace() for OSError, we should do this in both cases (and maybe in more cases). But this is a different issue. It can add a noise where it was silent.
