Avoid adding an empty directory to sys.path when running a module with -m

@brettcannon

BPO	33053
Nosy	@brettcannon, @ncoghlan, @vstinner, @jwilk, @ned-deily, @njsmith, @ericsnowcurrently, @eryksun, @ztane
PRs	bpo-33053: -m adds starting directory to sys.path #6231 bpo-33053: Remove test_cmd_line_script debugging print #6237 [3.7] bpo-33053: -m now adds starting directory to sys.path (GH-6231) #6236 [3.6] bpo-28655: Fix test_import.test_missing_source_legacy() #9589

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/ncoghlan'
closed_at = <Date 2018-03-25.13:50:03.376>
created_at = <Date 2018-03-12.11:57:14.902>
labels = ['interpreter-core', '3.8', 'type-feature', '3.7']
title = 'Avoid adding an empty directory to sys.path when running a module with `-m`'
updated_at = <Date 2018-09-26.16:09:42.678>
user = 'https://github.com/ztane'

bugs.python.org fields:

activity = <Date 2018-09-26.16:09:42.678>
actor = 'vstinner'
assignee = 'ncoghlan'
closed = True
closed_date = <Date 2018-03-25.13:50:03.376>
closer = 'ncoghlan'
components = ['Interpreter Core']
creation = <Date 2018-03-12.11:57:14.902>
creator = 'ztane'
dependencies = []
files = []
hgrepos = []
issue_num = 33053
keywords = ['patch']
message_count = 30.0
messages = ['313641', '313966', '314020', '314021', '314023', '314025', '314028', '314036', '314040', '314081', '314088', '314089', '314092', '314130', '314192', '314214', '314233', '314248', '314276', '314403', '314404', '314411', '314412', '314414', '314415', '314431', '314432', '315043', '315079', '326481']
nosy_count = 10.0
nosy_names = ['brett.cannon', 'ncoghlan', 'vstinner', 'jwilk', 'ned.deily', 'njs', 'eric.snow', 'eryksun', 'ztane', 'Viv']
pr_nums = ['6231', '6237', '6236', '9589']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue33053'
versions = ['Python 3.7', 'Python 3.8']

I think this is a really stupid security bug. Running a module with -mmodule seems to add '' as a path in sys.path, and in front. This is doubly wrong, because '' will stand for whatever the current working directory might happen to be at the time of the subsequent import statements, i.e. it is far worse than https://bugs.python.org/issue16202

I.e. whereas python3 /usr/lib/module.py wouldn't do that, python3 -mmodule would make it so that following a chdirs in code, imports would be executed from arbitrary locations. Verified on MacOS X, Ubuntu 17.10, using variety of Python versions up to 3.7.

FWIW, this behavior is documented:

https://docs.python.org/3/using/cmdline.html#cmdoption-m
"As with the -c option, the current directory will be added to the start of sys.path."

With the -c option, at least you could easily remove the sys.path element yourself:

python -c 'import sys; sys.path.remove(""); ...'

(This works, because sys is always a builtin module, so it won't be imported from cwd.)

I don't see any obvious way to make "python -m foo" secure in untrusted cwd, though.
The best I could come up with is:

python -c 'import sys; sys.path.remove(""); import runpy; runpy._run_module_as_main("foo")'

which is quite insane.

This isn't considered a security issue, as running "python3" interactively behaves in exactly the same way (i.e. tracking changes to the current working directory for the duration of the session), and running "python3 script.py" adds the full path to the current directory.

In all cases, the expectation is that end users will at least enable isolated mode if they don't want to risk importing arbitrary code from user controlled directories.

    $ echo "print('Hello')" > foo.py
    $ python3 -m foo
    Hello
    $ python3 -Im foo
    /usr/bin/python3: No module named foo

However, I'm flagging this as an enhancement request for 3.8+ (with a reworded issue title), as the non-isolated -m switch algorithm for sys.path[0] calculation could be made more robust as follows:

Start out with "os.getcwd()" rather than the empty string
Once __main__.__file__ has been calculated, delete sys.path[0] if main was found somewhere else

A potentially related enhancement would be to modify directory & zipfile execution to only look for __main__.py in sys.path[0] rather than searching the whole of sys.path (which is what currently happens).

Also, a small upstream community interaction tip: if you want people to seriously consider your requests for changes in default behaviour (which inevitably risk backwards compatibility breaks), don't start out by insulting them.

Python's defaults are currently set up for a *trusted personal automation tool*, where the person writing the code is also the person running it.

By starting out with an insult like "I think this is a really stupid security bug", you're actually saying "I know very little about Python's history, or the audiences it was originally written to serve, and instead of politely suggesting an alternative behaviour that would be more robust in the face of system configuration errors, I'm going to try to use shame, guilt, and embarrassment to get people to do work for me". That kind of behaviour *isn't* a good way to get your issues addressed, but it *is* a good way to encourage people to decide that volunteering as an open source maintainer isn't worth the associated hassles.

The opening insult added nothing to your issue report, and could more productively have been replaced with an explanation of the expectations you had of the default behaviour, how you came by those expectations, and how the current behaviour failed to meet them.

I've also separated out https://bugs.python.org/issue33095 (a docs issue about making isolated mode more discoverable) based on the jwilk's comment that it wasn't clear how to disable the default "add the current directory to sys.path" behaviour.

Whoa, wait, what?

I agree that the original post is not as diplomatic as it could be, but my reaction to learning about this just now is also shock and confusion, so I guess I can sympathize with the OP a bit...

The reason I'm surprised is that -- while this probably wasn't fully anticipated when -m was designed -- it's turned out to be a bit of a meme to replace calls like 'pip ...' with 'python -m pip ...', or 'virtualenv ...' with 'python -m virtualenv ...', etc. I thought these were generally pretty much equivalent. I definitely did *not* know that running 'python -m pip' could lead to executing arbitrary code from the cwd, and I'm sure I've run it inside e.g. random git checkouts. If someone had tried to spearphish me with this they would totally have succeeded. (I hope they haven't?)

If you want to run a file in the current directory, is there any advantage to doing 'python -m myscript' instead of 'python myscript.py'? Could we declare that the latter is the One Obvious Way and remove support for the former entirely?

"python -m mypkg.myscript" does the right thing as far as local packages are concerned, whereas "python -m mypkg/myscript.py" will set you up for double-import bugs.

Note that you can almost always trigger arbitrary non-obvious code execution just by writing sitecustomize.py to the current directory, and any package you install can add a "<installation-site-packages>/arbitrary-code.pth" or "<user-site-packages>/arbitrary-code.pth" file that gets run at startup (setuptools has long relied on this to implement various features).

Opting in to isolated mode turns *all* of those features off by saying "I'm expecting to run system code only here, not custom user code".

-I implies -s, which is not something I want.

https://bugs.python.org/issue13475 is the existing enhancement request to expose sys.path[0] management independently of the other execution isolation features.

PR posted with the change to use an absolute path for the starting working directory in the "-m" case.

That PR also includes a change to improve the fidelity of the test suite: back when I first wrote test_cmd_line_script, I was mainly focused on testing the runpy aspects, and not the sys.path initialisation aspects, so the way the tests worked didn't really check the latter properly.

The test updates get rid of the launch script that was previously confusing matters, and instead test sys.path[0] initialisation properly (relying on PYTHONPATH for the zipimport related cases where just changing the working directory isn't sufficient).

It turned out some tests in CPython's own test suite were implicitly relying on the old behaviour where the current working directory automatically ended up on sys.path (see the changes to test_bdb and test_doctest in the updated PR).

Initial fix has been merged to master, CI runs pending for the backport to 3.7 and a follow-up master branch PR to remove a debugging print I noticed when resolving a test_import conflict in the backport.

I won't get to merging those until some time after work tomorrow (probably 8 pm'ish in UTC+10), so if anyone wanted to merge them before that, it would likely be a good idea :)

3.7 CI finished before I logged off for the night, so this is good to go for 3.7.0b3 now :)

New changeset a9e5d0e by Nick Coghlan in branch 'master':
bpo-33053: Remove test_cmd_line_script debugging print (GH-6237)
a9e5d0e

Marking as fixed, since this is now the version likely to go out in 3.7.0b3 - if we find further problems with it (beyond the potential enhancement discussed above to make local directory usage opt-in), then those can go in a new issue.

New changeset d5d9e02 by Nick Coghlan in branch 'master':
bpo-33053: -m now adds *starting* directory to sys.path (GH-6231)
d5d9e02

New changeset ee37845 by Nick Coghlan in branch '3.7':
bpo-33053: -m now adds *starting* directory to sys.path (GH-6231) (bpo-6236)
ee37845

(See bpo-33185 for regression.)

Some notes from my investigation of bpo-33185 that seem more appropriate here, rather than on that issue:

several of the developer-centric utilities in the standard library have a shared need to be friendly to imports from the current working directory.
timeit uses os.curdir, but could be switched to os.getcwd()
pydoc uses a literal '.', but could be switched to os.getcwd()
trace, profile, cProfile, pdb, doctest, and IDLE's pyshell all add the directory containing the file under test

Aside from switching pydoc from a literal '.' to os.curdir, I'm not going to change any of those (hence why I'm putting these notes here), but I wanted to capture this info in case does decide to follow through on a "less isolated than isolated mode, but still omits the current directory from sys.path" execution mode.

New changeset 43500a5 by Victor Stinner in branch '3.6':
bpo-28655: Fix test_import.test_missing_source_legacy() (GH-9589)
43500a5

Navigation Menu

Repository navigation

Avoid adding an empty directory to sys.path when running a module with `-m` #77234

12 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Avoid adding an empty directory to sys.path when running a module with -m #77234

Description

Activity

ztane commented on Mar 12, 2018

jwilk commented on Mar 16, 2018

ncoghlan commented on Mar 18, 2018

ncoghlan commented on Mar 18, 2018

ncoghlan commented on Mar 18, 2018

njsmith commented on Mar 18, 2018

ncoghlan commented on Mar 18, 2018

jwilk commented on Mar 18, 2018

ncoghlan commented on Mar 18, 2018

12 remaining items

ncoghlan commented on Mar 25, 2018

ncoghlan commented on Mar 25, 2018

ncoghlan commented on Mar 25, 2018

ncoghlan commented on Mar 25, 2018

ncoghlan commented on Mar 25, 2018

ncoghlan commented on Mar 25, 2018

ncoghlan commented on Mar 25, 2018

ncoghlan commented on Mar 25, 2018

ned-deily commented on Apr 6, 2018

ncoghlan commented on Apr 8, 2018

vstinner commented on Sep 26, 2018

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Avoid adding an empty directory to sys.path when running a module with `-m` #77234