classification
Title: [Security] support.args_from_interpreter_flags() doesn't inherit -I (isolated) flag
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: danishprakash, miss-islington, vstinner, xtreak
Priority: normal Keywords: patch

Created on 2018-09-26 16:07 by vstinner, last changed 2018-11-23 18:03 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 10675 merged vstinner, 2018-11-23 14:01
PR 10684 merged miss-islington, 2018-11-23 16:54
PR 10688 merged vstinner, 2018-11-23 17:10
Messages (19)
msg326478 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-26 16:07
The support.args_from_interpreter_flags() function recreates Python command line arguments from sys.flags, but it omits -I (sys.flags.isolated).

Because of that, "./python -I -m test ..." behaves differently than "./python -I -m test -j0 ...":
https://bugs.python.org/issue28655#msg326477
msg326669 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-09-29 07:09
Thanks Victor for the details. Can this be classified as an easy issue? I guess the fix will be as below : 

1. Add an entry for '-I' at https://github.com/python/cpython/blob/4b430e5f6954ef4b248e95bfb4087635dcdefc6d/Lib/subprocess.py#L260

2. Add a test for '-I' at https://github.com/python/cpython/blob/4b430e5f6954ef4b248e95bfb4087635dcdefc6d/Lib/test/test_support.py#L472. 
The only thing here is that '-I' returns '-s -E -I' unlike other options where args can be used for comparison logic in check_options. check_options should be changed so that this can take given args and the expected args for comparison to accommodate -I. Maybe there is a better way?

Off topic : I don't know why '-I' is not documented as sys.flags.isolated at https://docs.python.org/3.7/library/sys.html#sys.flags . Maybe I will open up a separate issue for this?
msg326787 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-01 10:21
In the C code, sys.flags.isolated clearly documented as linked to the -I option:

static PyStructSequence_Field flags_fields[] = {
    {"debug",                   "-d"},
    {"inspect",                 "-i"},
    {"interactive",             "-i"},
    {"optimize",                "-O or -OO"},
    {"dont_write_bytecode",     "-B"},
    {"no_user_site",            "-s"},
    {"no_site",                 "-S"},
    {"ignore_environment",      "-E"},
    {"verbose",                 "-v"},
    /* {"unbuffered",                   "-u"}, */
    /* {"skip_first",                   "-x"}, */
    {"bytes_warning",           "-b"},
    {"quiet",                   "-q"},
    {"hash_randomization",      "-R"},
    {"isolated",                "-I"},
    {"dev_mode",                "-X dev"},
    {"utf8_mode",               "-X utf8"},
    {0}
};

> The only thing here is that '-I' returns '-s -E -I' unlike other options where args can be used for comparison logic in check_options.

I expect to get:

$ python3 -I -c 'import subprocess; print(subprocess._args_from_interpreter_flags())'
['-I']

instead of:

['-s', '-E']

-I is different from -s -E: it also avoids to add the script directory or an empty string to sys.path.
msg326790 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-10-01 10:36
Thanks Victor for the details. 

> In the C code, sys.flags.isolated clearly documented as linked to the -I option:

With respect to documentation I was talking about '-I' not being documented in the table at https://docs.python.org/3.7/library/sys.html#sys.flags though it's present in the C code and in sys.flags.isolated.

> -I is different from -s -E: it also avoids to add the script directory or an empty string to sys.path.

'-I' also implies '-s -E' and hence adding isolated to args_from_interpreter_flags will also return ['-s', '-E', '-I'] as output and hence I suggested modifying the comparison logic.

# Since '-I' implies '-s' and '-E' those flags are also set returning '-s -E -I'

./python.exe --help | rg '\-I'
-I     : isolate Python from the user's environment (implies -E and -s)

./python.exe -I -c 'import sys; print(sys.flags)'
sys.flags(debug=0, inspect=0, interactive=0, optimize=0, dont_write_bytecode=0, no_user_site=1, no_site=0, ignore_environment=1, verbose=0, bytes_warning=0, quiet=0, hash_randomization=1, isolated=1, dev_mode=False, utf8_mode=0)

# patching args_from_interpreter_flags to support '-I' would return below

./python.exe -I -c 'import subprocess; print(subprocess._args_from_interpreter_flags())'
['-s', '-E', '-I']


Thanks
msg326792 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-01 11:10
> ./python.exe -I -c 'import subprocess; print(subprocess._args_from_interpreter_flags())'
> ['-s', '-E', '-I']

This looks wrong, I would prefer to only get ['-I'].
msg326875 - (view) Author: Danish Prakash (danishprakash) * Date: 2018-10-02 12:16
> With respect to documentation I was talking about '-I' not being documented in the table at https://docs.python.org/3.7/library/sys.html#sys.flags though it's present in the C code and in sys.flags.isolated.

Thanks for bringing this up Karthikeyan, however, could there be another reason why -I would be left out. Also, have you filed an issue for this?


Also, Victor and Karthikeyan, since this issue has been categorized as an easy issue, I would like to fix this if none of you have started working on this.
msg326876 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-10-02 12:25
> Thanks for bringing this up Karthikeyan, however, could there be another reason why -I would be left out. Also, have you filed an issue for this?

I couldn't see any related issue for this though the table was changed in 3.7.0

> Also, Victor and Karthikeyan, since this issue has been categorized as an easy issue, I would like to fix this if none of you have started working on this.

I am not working on this. Feel free to pick it up.
msg326879 - (view) Author: Danish Prakash (danishprakash) * Date: 2018-10-02 12:46
Thank you Karthikeyan, I'm going to take care of both of these issues.
msg327099 - (view) Author: Danish Prakash (danishprakash) * Date: 2018-10-05 04:28
Linking this[1] here in case someone else stumbles upon this thread. I've created an issue and a PR for the documentation issue regarding the absence of -I flag from the sys.flags table which came into picture from the discussions in this thread.

[1]: https://bugs.python.org/issue34901
msg327280 - (view) Author: Danish Prakash (danishprakash) * Date: 2018-10-07 13:40
> I expect to get: ['-I'] instead of ['-s', '-E', '-I']

From what I understand, this can be done in one of two ways. First, we could edit https://github.com/python/cpython/blob/ad73a9cf97770023665a1bb1c6390a3c99478139/Modules/main.c#L430 and not incrementing -s and -E. But I believe this would have consequences that I'm unable to think of right now, so I'd like some inputs on this. 

Secondly, we could handle this condition in `_args_from_interpreted_flags()` itself but that might be looked upon as bad practice.
msg327289 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-10-07 16:47
> From what I understand, this can be done in one of two ways. First, we could edit https://github.com/python/cpython/blob/ad73a9cf97770023665a1bb1c6390a3c99478139/Modules/main.c#L430 and not incrementing -s and -E. But I believe this would have consequences that I'm unable to think of right now, so I'd like some inputs on this. 

As in the docs for -I it implies -s and -E so removing the increment is not a good solution in my opinion and will break code. 

I don't know how this can be handled since -I sets -s and -E implicitly and _args_from_interpreted_flags just looks for the set flag. This could also get a little complex if we remove -s and -E based on -I since one might pass -I and -s. Maybe we can do an intersection of the command line arguments passes and the set bits in _args_from_interpreted_flags so that only -I remains? Victor prefers -I only and maybe has an approach to solve this?
msg327318 - (view) Author: Danish Prakash (danishprakash) * Date: 2018-10-08 03:37
You're right Karthikeyan, although I personally think that returning ['-s', '-E', '-I'] should be a plausible solution here since it has been stated explicitly that it implies '-s' and '-E' but I'm still waiting for what Victor has to say on this. 

> The only thing here is that '-I' returns '-s -E -I' unlike other options where args can be used for comparison logic in check_options.

Karthikeyan, do you happen to have a use case where this might come into action?
msg327321 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-10-08 04:08
>> The only thing here is that '-I' returns '-s -E -I' unlike other options where args can be used for comparison logic in check_options.

> Karthikeyan, do you happen to have a use case where this might come into action?

I don't have a use case in mind. The comment was that returning '-s -E -I' would need the helper function used in the test to be changed.

Thanks
msg330295 - (view) Author: Danish Prakash (danishprakash) * Date: 2018-11-23 03:02
Sorry for bumping this thread but Victor, could you please share your inputs on this if you have the time for it, thanks.
msg330325 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-23 14:03
I tried to explain how to fix the bug, but nobody came up with a working change 2 months, so I wrote the PR myself. It's an important security issue, since the function is used by multiprocessing and distutils modules to spawn new child processes.
msg330340 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-23 16:54
New changeset 9de363271519e0616f4a7b59427057c4810d3acc by Victor Stinner in branch 'master':
bpo-34812: subprocess._args_from_interpreter_flags(): add isolated (GH-10675)
https://github.com/python/cpython/commit/9de363271519e0616f4a7b59427057c4810d3acc
msg330344 - (view) Author: miss-islington (miss-islington) Date: 2018-11-23 17:13
New changeset 01e579949ab546cd4cdd0d6d18e3ef41ce94f46e by Miss Islington (bot) in branch '3.7':
bpo-34812: subprocess._args_from_interpreter_flags(): add isolated (GH-10675)
https://github.com/python/cpython/commit/01e579949ab546cd4cdd0d6d18e3ef41ce94f46e
msg330351 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-23 18:02
New changeset cc0e0a2214d6515cf6ba4c7b164902a87e321b45 by Victor Stinner in branch '3.6':
bpo-34812: subprocess._args_from_interpreter_flags(): add isolated (GH-10675) (GH-10688)
https://github.com/python/cpython/commit/cc0e0a2214d6515cf6ba4c7b164902a87e321b45
msg330352 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-23 18:03
Ok, the bug is now fixed in Python 3.6, 3.7 and master branches ;-)
History
Date User Action Args
2018-11-23 18:03:21vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg330352

stage: patch review -> resolved
2018-11-23 18:02:28vstinnersetmessages: + msg330351
2018-11-23 17:13:35miss-islingtonsetnosy: + miss-islington
messages: + msg330344
2018-11-23 17:10:22vstinnersetpull_requests: + pull_request9940
2018-11-23 16:54:37miss-islingtonsetpull_requests: + pull_request9936
2018-11-23 16:54:23vstinnersetmessages: + msg330340
2018-11-23 14:03:59vstinnersettitle: [EASY] support.args_from_interpreter_flags() doesn't inherit -I (isolated) flag -> [Security] support.args_from_interpreter_flags() doesn't inherit -I (isolated) flag
messages: + msg330325

components: - Tests
keywords: - easy
type: security
2018-11-23 14:01:48vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request9931
2018-11-23 03:02:43danishprakashsetmessages: + msg330295
2018-10-08 04:08:45xtreaksetmessages: + msg327321
2018-10-08 03:37:37danishprakashsetmessages: + msg327318
2018-10-07 16:47:59xtreaksetmessages: + msg327289
2018-10-07 13:40:54danishprakashsetmessages: + msg327280
2018-10-05 04:28:19danishprakashsetmessages: + msg327099
2018-10-02 12:46:14danishprakashsetmessages: + msg326879
2018-10-02 12:25:36xtreaksetmessages: + msg326876
2018-10-02 12:16:22danishprakashsetnosy: + danishprakash
messages: + msg326875
2018-10-01 11:10:17vstinnersetmessages: + msg326792
2018-10-01 10:36:37xtreaksetmessages: + msg326790
2018-10-01 10:25:43vstinnersetkeywords: + easy
title: support.args_from_interpreter_flags() doesn't inherit -I (isolated) flag -> [EASY] support.args_from_interpreter_flags() doesn't inherit -I (isolated) flag
2018-10-01 10:21:45vstinnersetmessages: + msg326787
2018-09-29 07:09:30xtreaksetmessages: + msg326669
2018-09-26 16:10:02xtreaksetnosy: + xtreak
2018-09-26 16:07:03vstinnercreate