classification
Title: Remove current directory from posixpath.defpath to enhance security
Type: security Stage: patch review
Components: Library (Lib) Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, giampaolo.rodola, gregory.p.smith, izbyshev, jwilk, serhiy.storchaka, vstinner
Priority: normal Keywords: patch, patch, patch

Created on 2019-01-16 23:41 by vstinner, last changed 2019-01-18 11:37 by jwilk.

Files
File name Uploaded Description Edit
execv_curdir.py vstinner, 2019-01-16 23:54
subprocess_curdir.py vstinner, 2019-01-16 23:55
empty_path.py vstinner, 2019-01-17 09:22
Pull Requests
URL Status Linked Edit
PR 11586 open vstinner, 2019-01-17 09:25
PR 11586 open vstinner, 2019-01-17 09:25
PR 11586 open vstinner, 2019-01-17 09:25
Messages (11)
msg333801 - (view) Author: STINNER Victor (vstinner) * (Python committer) 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) * (Python committer) 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) * (Python triager) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python triager) 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.
History
Date User Action Args
2019-01-18 11:37:30jwilksetnosy: + jwilk
2019-01-17 23:16:04eryksunlinkissue26414 superseder
2019-01-17 22:38:20izbyshevsetmessages: + msg333905
2019-01-17 09:46:20vstinnersetkeywords: patch, patch, patch

messages: + msg333837
2019-01-17 09:35:25serhiy.storchakasetkeywords: patch, patch, patch
nosy: + serhiy.storchaka
messages: + msg333835

2019-01-17 09:28:36vstinnersetkeywords: patch, patch, patch

messages: + msg333834
2019-01-17 09:25:47vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request11277
2019-01-17 09:25:39vstinnersetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11276
2019-01-17 09:25:31vstinnersetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11275
2019-01-17 09:22:28vstinnersetfiles: + empty_path.py

messages: + msg333832
2019-01-17 07:41:13christian.heimessetmessages: + msg333830
2019-01-17 07:33:14gregory.p.smithsetmessages: + msg333827
2019-01-17 07:30:46gregory.p.smithsetmessages: + msg333826
2019-01-17 00:48:49izbyshevsetnosy: + izbyshev
messages: + msg333807
2019-01-16 23:55:16vstinnersetfiles: + subprocess_curdir.py
2019-01-16 23:54:57vstinnersetfiles: + execv_curdir.py

messages: + msg333804
2019-01-16 23:41:46vstinnercreate