msg333801 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-01-16 23:41 |
Currently, posixpath.defpath is equal to:
defpath = ':/bin:/usr/bin'
It gives 3 directories:
>>> posixpath.defpath.split(posixpath.pathsep)
['', '/bin', '/usr/bin']
where the empty string means "the current directory". Trying to locate an executable from the current directory can be security issue when an attacker tries to execute arbitrary command.
The Linux exec(3) manual page contains an interesting note about the removal of the empty string from glibc 2.24 by accident:
http://man7.org/linux/man-pages/man3/execvp.3.html
NOTES
The default search path (used when the environment does not contain
the variable PATH) shows some variation across systems. It generally
includes /bin and /usr/bin (in that order) and may also include the
current working directory. On some other systems, the current
working is included after /bin and /usr/bin, as an anti-Trojan-horse
measure. The glibc implementation long followed the traditional
default where the current working directory is included at the start
of the search path. However, some code refactoring during the
development of glibc 2.24 caused the current working directory to be
dropped altogether from the default search path. This accidental
behavior change is considered mildly beneficial, and won't be
reverted.
(...)
Context of this issue: This discussion started from my PR 11579 which modifies the subprocess module to use posix_spawnp():
https://github.com/python/cpython/pull/11579#pullrequestreview-193261299
So I propose to replace defpath = ':/bin:/usr/bin' with defpath = '/bin:/usr/bin' which gives 2 directories:
>>> '/bin:/usr/bin'.split(posixpath.pathsep)
['/bin', '/usr/bin']
This change would only affect os.get_exec_path(), and so indirectly the subprocess module (when the executable contains no directory), *when the PATH environmant variable is not set*.
|
msg333804 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-01-16 23:54 |
I wrote attached execv_curdir.py to check if os.execv() tries to find the executable in the current directory if it doesn't contain a directory: yes, it does.
$ python3 execv_curdir.py
execv() searchs in the current directory
I also wrote attached subprocess_curdir.py which confirms that subprocess runs a program from the current directory if it exists.
$ python3 subprocess_curdir.py
defpath = :/bin:/usr/bin
subprocess searchs in the current directory
Moreover, the current directory has the priority over /bin and /usr/bin.
|
msg333807 - (view) |
Author: Alexey Izbyshev (izbyshev) * |
Date: 2019-01-17 00:48 |
Would it make sense to use os.confstr('CS_PATH') instead of a hardcoded path, or is identical behavior on all POSIX platforms preferred to that?
|
msg333826 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2019-01-17 07:30 |
ntpath and macpath appear to have the same potential issue.
They've basically had this defpath value forever.
keep following it and you find a commit from 1994 https://github.com/python/cpython/commit/2979b01ff88ac4c5b316d9bf98edbaaaffac8e24
Changing them only alters behavior of users of os.defpath or in the (odd?) situation when the PATH environment variable is unset, anything using of shutil.which() or distutils.spawn.find_executabnle(), the only stdlib users of os.defpath.
|
msg333827 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2019-01-17 07:33 |
I'm not arguing against this change, just trying to figure out where it came from in the first place. We should fix the value on all OSes.
It would be a behavior change so probably only good for 3.8+.
|
msg333830 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2019-01-17 07:41 |
+1, /usr/bin:/bin sounds good to me.
My /usr/include/paths.h has #define _PATH_DEFPATH "/usr/bin:/bin" and #define _PATH_STDPATH "/usr/bin:/bin:/usr/sbin:/sbin". The file is pretty old and has copyright from 89 and 93, https://code.woboq.org/gcc/include/paths.h.html
|
msg333832 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-01-17 09:22 |
I'm working on PR but I found an issue.
shutil.which() behaves differently than subprocess, distutils.spawn.find_executable() and os.execv() when PATH is set but set to an empty string:
* os.get_exec_path() returns ['']
* shutil.which() returns None: DON'T RUN
* subprocess RUNS the program
* distutils.spawn.find_executable('program') returns 'program': RUN the program
* os.execv() RUNS the program
Using PATH=":", they all run the program and os.get_exec_path() returns ['', ''].
Who is right? Which behavior do we want for Python?
Note: When PATH is set to an empty string, shutil.which() and distutils.spawn.find_executable() use the empty string, they don't use os.defpath.
|
msg333834 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-01-17 09:28 |
I wrote PR 11586 to remove the current directory from os.defpath.
I would prefer to first decide how the os, subprocess, shutil and distutils modules have to handle a PATH variable set to an empty string, before merging my PR. I would prefer to have the same behavior for these modules.
Gregory P. Smith:
> I'm not arguing against this change, just trying to figure out where it came from in the first place. We should fix the value on all OSes.
Oh, I didn't know that Windows had the same behavior... I didn't know that Windows has a default search path! C:\bin? Who has such directory?
I agree that it's better to have the same behavior on all platforms.
Gregory P. Smith:
> It would be a behavior change so probably only good for 3.8+.
I concur. It's a backward incompatible change.
Christian Heimes:
> My /usr/include/paths.h has #define _PATH_DEFPATH "/usr/bin:/bin" and #define _PATH_STDPATH "/usr/bin:/bin:/usr/sbin:/sbin". The file is pretty old and has copyright from 89 and 93, https://code.woboq.org/gcc/include/paths.h.html
On my Fedora 29, this file comes from the glibc:
$ rpm -qf /usr/include/paths.h
glibc-headers-2.28-26.fc29.i686
According to execvp() manual page, the current directory has only been removed from glibc 2.24 (2016-08-05).
|
msg333835 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2019-01-17 09:35 |
This is how the which command behaves:
$ /usr/bin/which python
/usr/bin/python
$ PATH= /usr/bin/which python
$ PATH=. /usr/bin/which python
./python
$ PATH=: /usr/bin/which python
./python
I think shutil.which() should behave similarly unless there are good reasons for difference.
|
msg333837 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-01-17 09:46 |
Alexey Izbyshev:
> Would it make sense to use os.confstr('CS_PATH') instead of a hardcoded path, or is identical behavior on all POSIX platforms preferred to that?
I didn't know this variable. man confstr says:
_CS_PATH: A value for the PATH variable which indicates where all the POSIX.2 standard utilities can be found.
On my Fedora 29, it only returns '/usr/bin':
$ python3
>>> import os; os.confstr("CS_PATH")
'/usr/bin'
On Fedora 29, /bin is a symlink to /usr/bin:
$ ls -ld /bin
lrwxrwxrwx. 1 root root 7 13 juil. 2018 /bin -> usr/bin/
So it makes sense to omit /bin from the default search path :-)
On Debian Sid where /bin is still a distinct directory than /usr/bin, CS_PATH is equal to "/bin:/usr/bin".
On Fedora, using confstr() would have the advantage of avoiding one useless syscall stat("/bin/program") in addition to stat("/usr/bin/program") in shutil.which() if the program doesn't exist... It's really a micro optimization which has no impact on the correctness, for the specific case of Fedora.
About the correctness, FreeBSD has a different value:
>>> os.confstr("CS_PATH")
'/usr/bin:/bin:/usr/sbin:/sbin'
Not only it also includes /usr/sbin and /sbin, but /usr/bin has the preference over /bin (posixpath of Python 3 checks /bin before /usr/bin). I'm not sure if the different order has an impact about correctness. I only found two programs which are in /bin and /usr/bin, but the programs in /usr/bin are symbolic links to a program with the same name in /bin :-)
vstinner@freebsd$ python3
Python 3.6.6 (default, Nov 20 2018, 01:57:10)
>>> import os
>>> usr=os.listdir("/usr/bin")
>>> bin=os.listdir("/bin")
>>> set(usr) & set(bin)
{'pkill', 'pgrep'}
vstinner@freebsd$ file /usr/bin/pkill
/usr/bin/pkill: symbolic link to ../../bin/pkill
|
msg333905 - (view) |
Author: Alexey Izbyshev (izbyshev) * |
Date: 2019-01-17 22:38 |
Thanks for the info on CS_PATH, Victor. IMHO it'd make sense to use the libc-provided default PATH at least in shutil.which() since its intent is to emulate "which" from the default shell.
|
msg340352 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-16 16:11 |
I wrote PR 12858 to os.confstr("CS_PATH") if available in shutil.which() and distutils.spawn.find_executable(), but also change the behavior when the PATH environment variable is set to an empty string: use an empty string, don't use os.confstr("CS_PATH") nor os.defpath.
|
msg340357 - (view) |
Author: Jakub Wilk (jwilk) |
Date: 2019-04-16 16:59 |
which(1) is not standardized, and there are many[*] implementations with different behavior in corner cases. For example, this happens with zsh 5.7.1 on Debian:
% which python
/usr/bin/python
% PATH= which python
python
% PATH=. which python
./python
% PATH=: which python
python
[*] I'm aware of GNU which, which from debianutils, and zsh builtin. In addition to this, AFAICS every major BSD distro has a different implementation…
|
msg340366 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-16 21:30 |
My PR is consistent with the behavior you described in your zsh example,
no? which doesn't find python if PATH is empty or equal to ":".
|
msg340384 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 09:44 |
find_executable() first looks if the program exists in the current directory. My PR doesn't change that. I have no opinion if it's a good thing or not, but I don't want to change that in this PR. If someone wants to change it, please open a separated issue on bugs.python.org since it will be backward incompatible change not directly related to this issue.
|
msg340399 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 14:26 |
New changeset 228a3c99bdb2d02771bead66a0beabafad3a90d3 by Victor Stinner in branch 'master':
bpo-35755: shutil.which() uses os.confstr("CS_PATH") (GH-12858)
https://github.com/python/cpython/commit/228a3c99bdb2d02771bead66a0beabafad3a90d3
|
msg340400 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 14:51 |
CS_PATH value:
* Fedora 29: "/usr/bin"
* Ubuntu 16.04: "/bin:/usr/bin"
It seems like the current directory is usually not part of the CS_PATH value.
|
msg340401 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 15:05 |
New changeset 2c4c02f8a876fcf084575dcaf857a0236c81261a by Victor Stinner in branch 'master':
bpo-35755: Remove current directory from posixpath.defpath (GH-11586)
https://github.com/python/cpython/commit/2c4c02f8a876fcf084575dcaf857a0236c81261a
|
msg340402 - (view) |
Author: Jakub Wilk (jwilk) |
Date: 2019-04-17 15:06 |
(Note that in msg333835 another implementation, presumably GNU which, was tested.)
My point is that "which" implementations have different behavior, so justifying anything with "which" compatibility is weird at best. You can't be compatible with all them.
Also telling users that you "mimick Unix which command behavior" is unhelpful, because vast majority of users have no idea how it behaves in this corner case.
|
msg340403 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 15:15 |
> My point is that "which" implementations have different behavior
I don't understand this point. Your example is consistent with what I saw on my Fedora 29 and the Python implementation that I just merged. Would you mind to elaborate which corner case is handled differently and describe how?
|
msg340404 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 15:24 |
Anyway, I wrote PR 12861 to remove "to mimick Unix which command behavior".
|
msg340410 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 15:44 |
New changeset 197f0447e3bcfa4f529fedab09966d7e3d283979 by Victor Stinner in branch 'master':
bpo-35755: Don't say "to mimick Unix which command behavior" (GH-12861)
https://github.com/python/cpython/commit/197f0447e3bcfa4f529fedab09966d7e3d283979
|
msg340416 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 16:38 |
New changeset 394b991e41a2a4ce3afc8e6fde44de46e73bbb34 by Victor Stinner in branch '3.7':
[3.7] bpo-35755: shutil.which() uses os.confstr("CS_PATH") (GH-12862)
https://github.com/python/cpython/commit/394b991e41a2a4ce3afc8e6fde44de46e73bbb34
|
msg340418 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 16:50 |
Gregory:
> I'm not arguing against this change, just trying to figure out where it came from in the first place. We should fix the value on all OSes.
In the meanwhile, I reverted the ntpath change. I'm not sure that it's ok to change the Windows case.
shutil.which() *always* starts by checking if the searched program is the current directory:
if sys.platform == "win32":
# The current directory takes precedence on Windows.
...
if curdir not in path:
path.insert(0, curdir)
If someone cares about changing Windows, please open a separated issue.
|
msg340419 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 16:50 |
I'm still confused by distutils.spawn.find_executable() function which *always* first look the current directory. I don't know the rationale for this behavior, so I made the conservation choice of keeping it.
If someone wants to change distutils.spawn.find_executable(), please open a separated issue.
|
msg340420 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 16:53 |
I modified posixpath.defpath, shutil.which() and distutils.spawn.find_executable() in 3.7 and master (future Python 3.8) branches. I close the issue. Thanks everybody for the review and helping me to collect info about corner cases!
I chose to also change Python 3.7. IMHO there is a low risk of breaking applications: I expect that few users run Python with no PATH environment variable *and* expect that Python looks for programs in the current directory. But it enhances the security a little bit.
For Python 2.7... well, I don't think that this issue is important enough to justify a backport. I prefer to do nothing rather than having to deal with unhappy users complaining that Python 2.7 changed broke their application in a minor 2.7.x release :-) Even if, again, the risk of regression is very low.
|
msg340422 - (view) |
Author: Jakub Wilk (jwilk) |
Date: 2019-04-17 17:29 |
When PATH is empty string:
* zsh and FreeBSD which look for binaries in cwd.
* debianutils and GNU which always fail.
I suspect that the former is implementation accident. I can't imagine why would anyone want this behavior.
NB, POSIX says that when PATH is unset or empty, the path search is implementation-defined.
|
msg340446 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-17 20:58 |
> I suspect that the former is implementation accident. I can't imagine why
would anyone want this behavior.
>
> NB, POSIX says that when PATH is unset or empty, the path search is
implementation-defined.
Not thank you POSIX for being clueless. Let's say that Python is
opiniatied, as when we decided that file descriptors must be created
non-inheritable by default even if "it goes against POSIX".
If you want to look if the current directory, you now have to ask for it
kindly and explicitly ;-)
IHMO Using CS_PATH rather than hardcoded os.defpath is also a major step
forward ;-)
|
msg340485 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-04-18 12:54 |
> For Python 2.7... well, I don't think that this issue is important enough to justify a backport. I prefer to do nothing rather than having to deal with unhappy users complaining that Python 2.7 changed broke their application in a minor 2.7.x release :-) Even if, again, the risk of regression is very low.
Same rationale for Python 3.6. While I would call this change related to security, I'm not comfortable to backport the change. The issue is not important enough compared to the risk of regression.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:10 | admin | set | github: 79936 |
2019-04-24 10:54:33 | koobs | set | keywords:
patch, patch, patch nosy:
+ koobs
|
2019-04-18 12:54:13 | vstinner | set | keywords:
patch, patch, patch
messages:
+ msg340485 |
2019-04-17 20:58:47 | vstinner | set | messages:
+ msg340446 |
2019-04-17 17:29:24 | jwilk | set | messages:
+ msg340422 |
2019-04-17 16:53:09 | vstinner | set | status: open -> closed title: shutil.which() and subprocess no longer look for the executable in the current directory if PATH environment variable is not set -> On Unix, shutil.which() and subprocess no longer look for the executable in the current directory if PATH environment variable is not set messages:
+ msg340420
keywords:
patch, patch, patch resolution: fixed stage: patch review -> resolved |
2019-04-17 16:50:34 | vstinner | set | keywords:
patch, patch, patch
messages:
+ msg340419 |
2019-04-17 16:50:06 | vstinner | set | keywords:
patch, patch, patch
messages:
+ msg340418 |
2019-04-17 16:39:08 | vstinner | set | keywords:
patch, patch, patch title: shutil.which() and subprocess no longer looks the executable in the current directory if PATH environment variable is not set -> shutil.which() and subprocess no longer look for the executable in the current directory if PATH environment variable is not set |
2019-04-17 16:38:59 | vstinner | set | keywords:
patch, patch, patch title: Remove current directory from posixpath.defpath to enhance security -> shutil.which() and subprocess no longer looks the executable in the current directory if PATH environment variable is not set |
2019-04-17 16:38:10 | vstinner | set | messages:
+ msg340416 |
2019-04-17 15:49:01 | vstinner | set | pull_requests:
+ pull_request12786 |
2019-04-17 15:44:10 | vstinner | set | messages:
+ msg340410 |
2019-04-17 15:24:26 | vstinner | set | keywords:
patch, patch, patch
messages:
+ msg340404 |
2019-04-17 15:23:54 | vstinner | set | pull_requests:
+ pull_request12785 |
2019-04-17 15:15:37 | vstinner | set | keywords:
patch, patch, patch
messages:
+ msg340403 |
2019-04-17 15:06:12 | jwilk | set | messages:
+ msg340402 |
2019-04-17 15:05:43 | vstinner | set | messages:
+ msg340401 |
2019-04-17 14:51:49 | vstinner | set | keywords:
patch, patch, patch
messages:
+ msg340400 |
2019-04-17 14:26:41 | vstinner | set | messages:
+ msg340399 |
2019-04-17 09:44:56 | vstinner | set | keywords:
patch, patch, patch
messages:
+ msg340384 |
2019-04-16 21:30:11 | vstinner | set | messages:
+ msg340366 |
2019-04-16 16:59:00 | jwilk | set | messages:
+ msg340357 |
2019-04-16 16:11:11 | vstinner | set | keywords:
patch, patch, patch
messages:
+ msg340352 |
2019-04-16 16:08:27 | vstinner | set | pull_requests:
+ pull_request12782 |
2019-01-18 11:37:30 | jwilk | set | nosy:
+ jwilk
|
2019-01-17 23:16:04 | eryksun | link | issue26414 superseder |
2019-01-17 22:38:20 | izbyshev | set | messages:
+ msg333905 |
2019-01-17 09:46:20 | vstinner | set | keywords:
patch, patch, patch
messages:
+ msg333837 |
2019-01-17 09:35:25 | serhiy.storchaka | set | keywords:
patch, patch, patch nosy:
+ serhiy.storchaka messages:
+ msg333835
|
2019-01-17 09:28:36 | vstinner | set | keywords:
patch, patch, patch
messages:
+ msg333834 |
2019-01-17 09:25:47 | vstinner | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request11277 |
2019-01-17 09:25:39 | vstinner | set | keywords:
+ patch stage: (no value) pull_requests:
+ pull_request11276 |
2019-01-17 09:25:31 | vstinner | set | keywords:
+ patch stage: (no value) pull_requests:
+ pull_request11275 |
2019-01-17 09:22:28 | vstinner | set | files:
+ empty_path.py
messages:
+ msg333832 |
2019-01-17 07:41:13 | christian.heimes | set | messages:
+ msg333830 |
2019-01-17 07:33:14 | gregory.p.smith | set | messages:
+ msg333827 |
2019-01-17 07:30:46 | gregory.p.smith | set | messages:
+ msg333826 |
2019-01-17 00:48:49 | izbyshev | set | nosy:
+ izbyshev messages:
+ msg333807
|
2019-01-16 23:55:16 | vstinner | set | files:
+ subprocess_curdir.py |
2019-01-16 23:54:57 | vstinner | set | files:
+ execv_curdir.py
messages:
+ msg333804 |
2019-01-16 23:41:46 | vstinner | create | |