subprocess: add user, group and extra_groups parameters #80227

patrick-mclean · 2019-02-20T01:24:19Z

BPO	36046
Nosy	@Yhg1s, @rhettinger, @gpshead, @vstinner, @giampaolo, @desbma, @izbyshev, @patrick-mclean
PRs	bpo-36046: Add user and group parameters to subprocess #11950 bpo-36046: Fix buildbot failures from the PR. #16091 bpo-36046: posix_spawn() doesn't support uid/gid #16384

^{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/gpshead'
closed_at = <Date 2019-09-25.13:57:50.689>
created_at = <Date 2019-02-20.01:24:18.539>
labels = ['type-feature', 'library', '3.9']
title = 'subprocess: add user, group and extra_groups parameters'
updated_at = <Date 2019-09-25.13:57:50.688>
user = 'https://github.com/patrick-mclean'

bugs.python.org fields:

activity = <Date 2019-09-25.13:57:50.688>
actor = 'vstinner'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2019-09-25.13:57:50.689>
closer = 'vstinner'
components = ['Library (Lib)']
creation = <Date 2019-02-20.01:24:18.539>
creator = 'patrick.mclean'
dependencies = []
files = []
hgrepos = []
issue_num = 36046
keywords = ['patch']
message_count = 21.0
messages = ['336033', '336346', '336579', '336713', '336720', '336794', '337021', '352216', '352217', '352233', '352235', '352236', '352237', '352265', '352268', '352333', '353168', '353195', '353197', '353198', '353199']
nosy_count = 8.0
nosy_names = ['twouters', 'rhettinger', 'gregory.p.smith', 'vstinner', 'giampaolo.rodola', 'desbma', 'izbyshev', 'patrick.mclean']
pr_nums = ['11950', '16091', '16384']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue36046'
versions = ['Python 3.9']

patrick-mclean · 2019-02-20T01:24:18Z

Currently when using python to automate system administration tasks, it is useful to drop privileges sometimes. Currently the only way to do this is via a preexec_fn, which has well-documented problems. It would be useful to be able to pass a user and groups arguments to subprocess.popen.

izbyshev · 2019-02-22T21:48:47Z

Patrick, could you provide more background that would explain your choice of setreuid/setregid functions and the desired handling of supplementary groups? I'm not a security expert, so I may not have sufficient expertise to judge on that, but maybe my questions could be helpful for others:

setreuid/setregid, as used in your PR, will set the real, effective and saved ids to the specified value [1]. So this precludes the use-case where a user wants to temporarily switch the effective id to the real id ("drop privileges") and then switch it back to the old effective (saved) id in the child. This use case is supported for non-root processes by POSIX_SPAWN_RESETIDS flag used with posix_spawn() (the flag is implemented by simply calling setuid(getuid()) in the child). Is it okay that the proposed API doesn't support this?
While setreuid/setregid on Linux permit setting the real id to either the effective or the real id, POSIX leaves it unspecified [2]. Are we okay with potential portability problems?
setgroups() always requires privileges on Linux [3]. So, if we always call setgroups() if subprocess.Popen(groups=...) is used, we preclude the use case where a non-privileged process wants to switch its gid but doesn't want to touch its supplementary groups. Is this a valid use case we want to care about?

The current workaround for the above is to call setgroups() only if geteuid() == 0, but I'm not sure it's a good one:
(a) ISTM we're basically guessing the intent here: what if a process really wants to change its supplementary groups, but a user run it without appropriate privileges by mistake? I think such process would like to get an exception instead of silently ignored call to setgroups().
(b) geteuid() == 0 test is not precise. The Linux man page documents that the caller needs the CAP_SETGID capability, which doesn't necessarily imply that the effective id is zero.

Another solution would be to split groups into two arguments: gid and supplementary groups. This way users can explicitly tell us whether they want to switch supplementary groups or not.

Overall, I'd really like to have security experts review this PR if possible.

[1] http://man7.org/linux/man-pages/man2/setreuid.2.html
[2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/setreuid.html
[3] http://man7.org/linux/man-pages/man2/setgroups.2.html

patrick-mclean · 2019-02-25T23:41:01Z

Alexey, here are my responses to your points:

This is intentional, this is for dropping privileges before running some (possibly untrusted) command, we do not want to leave a path for the subprocess to gain root back. If there is a subprocess that needs root for some operations, it would presumably have the ability to drop privileges itself, and would not need the python script to do it before running it.
While POSIX leaves it unspecified what changes are permitted for an unprivileged process, these are permitted for a privileged process, which is the main use case for this functionality. In the case the OS does not support it for an unpriviliged process, the syscall would fail with EPERM, which can be handled from the calling python code.

I am fine removing the workaround, and allowing it to fail with EPERM. Perhaps we could find another way around this for running the tests in an unprivileged environment, or just leave the test only running the EPERM case...

I could change the API to have have group= and supp_groups= if you prefer.

izbyshev · 2019-02-26T21:21:22Z

This is intentional, this is for dropping privileges before running some (possibly untrusted) command, we do not want to leave a path for the subprocess to gain root back. If there is a subprocess that needs root for some operations, it would presumably have the ability to drop privileges itself, and would not need the python script to do it before running it.

While POSIX leaves it unspecified what changes are permitted for an unprivileged process, these are permitted for a privileged process, which is the main use case for this functionality. In the case the OS does not support it for an unpriviliged process, the syscall would fail with EPERM, which can be handled from the calling python code.

Thanks for your explanation. In case of a privileged process, the behavior of setreuid/setregid/setgroups does seem well-defined. But setuid/setgid change all ids (real, effective, saved) too in this case. Do you prefer setreuid/setregid because they provide stricter semantics in non-privileged processes compared to setuid/setgid? (The latter ones change the effective id only, potentially preserving the process ability to switch ids later).

I am fine removing the workaround, and allowing it to fail with EPERM. Perhaps we could find another way around this for running the tests in an unprivileged environment, or just leave the test only running the EPERM case...

I could change the API to have have group= and supp_groups= if you prefer.

Personally, I do prefer separate arguments because subprocess is a relatively low-level module and tends to expose underlying operations, so it seems logical to have a knob that maps to setgid() and another one that maps to setgroups(). @gregory.p.smith: what do you think about it?

patrick-mclean · 2019-02-26T23:04:20Z

Thanks for your explanation. In case of a privileged process, the behavior of setreuid/setregid/setgroups does seem well-defined. But setuid/setgid change all ids (real, effective, saved) too in this case. Do you prefer setreuid/setregid because they provide stricter semantics in non-privileged processes compared to setuid/setgid? (The latter ones change the effective id only, potentially preserving the process ability to switch ids later).

Yes, exactly. The stricter semantics provide stronger security guarantees. The idea is to run code in an unprivileged context in a way that the code has no way to regain privileges.

patrick-mclean · 2019-02-28T02:42:41Z

I have updated the pull request to include 'group' and 'extra_groups' as separate parameters.

gpshead · 2019-03-02T23:11:34Z

I like the separate parameters. :)

gpshead · 2019-09-12T17:15:47Z

New changeset 2b2ead7 by Gregory P. Smith (Patrick McLean) in branch 'master':
bpo-36046: Add user and group parameters to subprocess (GH-11950)
2b2ead7

gpshead · 2019-09-12T17:16:19Z

I'm curious to see what weird things the various buildbot platforms find for this one. crossing my fingers.

rhettinger · 2019-09-13T01:20:06Z

Thanks for working on this. Least privilege is an important security consideration. The world will be a better place if we limit harm from bad or broken actors.

gpshead · 2019-09-13T01:57:03Z

The 'Debian root' buildbot exposed a unittest issue to deal with:

https://buildbot.python.org/all/#/builders/27/builds/3702/steps/5/logs/stdio

======================================================================
ERROR: test_user (test.test_subprocess.POSIXProcessTestCase) (user=65534)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_subprocess.py", line 1761, in test_user
    output = subprocess.check_output(
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 419, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 497, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 941, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 1797, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
PermissionError: [Errno 13] Permission denied: '/root/buildarea/3.x.angelico-debian-amd64/build/python'

======================================================================
ERROR: test_user (test.test_subprocess.POSIXProcessTestCase) (user='nobody')
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_subprocess.py", line 1761, in test_user
    output = subprocess.check_output(
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 419, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 497, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 941, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 1797, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
PermissionError: [Errno 13] Permission denied: '/root/buildarea/3.x.angelico-debian-amd64/build/python'

gpshead · 2019-09-13T01:58:58Z

and some Fedora and RHEL bots are failing with:

======================================================================
ERROR: test_extra_groups (test.test_subprocess.POSIXProcessTestCase)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/test_subprocess.py", line 1840, in test_extra_groups
    output = subprocess.check_output(
  File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/subprocess.py", line 419, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/subprocess.py", line 497, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/subprocess.py", line 889, in __init__
    gids.append(grp.getgrnam(extra_group).gr_gid)
KeyError: "getgrnam(): name not found: 'nogroup'"

======================================================================
ERROR: test_group (test.test_subprocess.POSIXProcessTestCase) (group='nogroup')
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/test_subprocess.py", line 1800, in test_group
    output = subprocess.check_output(
  File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/subprocess.py", line 419, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/subprocess.py", line 497, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/subprocess.py", line 862, in __init__
    gid = grp.getgrnam(group).gr_gid
KeyError: "getgrnam(): name not found: 'nogroup'"

gpshead · 2019-09-13T02:01:10Z

AIX has the same error as RHEL and Fedora

https://buildbot.python.org/all/#/builders/161/builds/1615/steps/5/logs/stdio

vstinner · 2019-09-13T09:45:07Z

The test fails on Fedora:
https://buildbot.python.org/all/#/builders/185/builds/1028

vstinner@apu$ ./python -m test test_subprocess -m test_extra_groups -m test_group -v
== CPython 3.9.0a0 (heads/master:7cad53e6b0, Sep 13 2019, 11:42:25) [GCC 9.2.1 20190827 (Red Hat 9.2.1-1)]
== Linux-5.2.9-200.fc30.x86_64-x86_64-with-glibc2.29 little-endian
== cwd: /home/vstinner/python/master/build/test_python_14292
== CPU count: 8
== encodings: locale=UTF-8, FS=utf-8
Run tests sequentially
0:00:00 load avg: 0.38 [1/1] test_subprocess
test_extra_groups (test.test_subprocess.POSIXProcessTestCase) ... ERROR
test_group (test.test_subprocess.POSIXProcessTestCase) ...
======================================================================
ERROR: test_extra_groups (test.test_subprocess.POSIXProcessTestCase)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/vstinner/python/master/Lib/test/test_subprocess.py", line 1840, in test_extra_groups
    output = subprocess.check_output(
  File "/home/vstinner/python/master/Lib/subprocess.py", line 419, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/home/vstinner/python/master/Lib/subprocess.py", line 497, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/home/vstinner/python/master/Lib/subprocess.py", line 889, in __init__
    gids.append(grp.getgrnam(extra_group).gr_gid)
KeyError: "getgrnam(): name not found: 'nogroup'"

======================================================================
ERROR: test_group (test.test_subprocess.POSIXProcessTestCase) (group='nogroup')
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/vstinner/python/master/Lib/test/test_subprocess.py", line 1800, in test_group
    output = subprocess.check_output(
  File "/home/vstinner/python/master/Lib/subprocess.py", line 419, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/home/vstinner/python/master/Lib/subprocess.py", line 497, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/home/vstinner/python/master/Lib/subprocess.py", line 862, in __init__
    gid = grp.getgrnam(group).gr_gid
KeyError: "getgrnam(): name not found: 'nogroup'"

Ran 2 tests in 0.046s

FAILED (errors=2)
test test_subprocess failed
test_subprocess failed

== Tests result: FAILURE ==

1 test failed:
test_subprocess

Total duration: 175 ms
Tests result: FAILURE

vstinner · 2019-09-13T09:46:52Z

Failures on Debian:
https://buildbot.python.org/all/#/builders/27/builds/3699

======================================================================
ERROR: test_user (test.test_subprocess.POSIXProcessTestCase) (user=65534)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_subprocess.py", line 1761, in test_user
    output = subprocess.check_output(
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 419, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 497, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 941, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 1797, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
PermissionError: [Errno 13] Permission denied: '/root/buildarea/3.x.angelico-debian-amd64/build/python'

======================================================================
ERROR: test_user (test.test_subprocess.POSIXProcessTestCase) (user='nobody')
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_subprocess.py", line 1761, in test_user
    output = subprocess.check_output(
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 419, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 497, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 941, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/subprocess.py", line 1797, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
PermissionError: [Errno 13] Permission denied: '/root/buildarea/3.x.angelico-debian-amd64/build/python'

Ran 306 tests in 35.920s

FAILED (errors=2, skipped=32)

Yhg1s · 2019-09-13T13:43:38Z

New changeset 693aa80 by T. Wouters (Gregory P. Smith) in branch 'master':
bpo-36046: Fix buildbot failures (GH-16091)
693aa80

vstinner · 2019-09-25T10:55:38Z

Hum, this change has a big (security?) issue:

import subprocess
subprocess.Popen(["/usr/bin/id"], user=1000, group=1000).wait()

gives:

uid=1000(vstinner) gid=1000(vstinner) groupes=1000(vstinner),0(root) contexte=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023

But:

import subprocess
subprocess.Popen(["/usr/bin/id"], user=1000, group=1000, close_fds=False).wait()

gives:

uid=0(root) gid=0(root) groupes=0(root) contexte=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023

=> user and group arguments are ignored when using close_fds=False: when subprocess uses posix_spawn().

Note: test_subprocess test_group() is skipped on my Fedora 30.

I wrote PR 16384 to fix the bug and enable test_group() on my Fedora (check also for "nobody" and "nfsnobody" groups).

Yhg1s · 2019-09-25T13:52:53Z

New changeset faca855 by T. Wouters (Victor Stinner) in branch 'master':
bpo-36046: posix_spawn() doesn't support uid/gid (GH-16384)
faca855

Yhg1s · 2019-09-25T13:55:36Z

As mentioned in the code review for Victor's fix, it feels like the big conditional on the use of self._posix_spawn in Popen._execute_child should perhaps be pulled into _posix_spawn, or at least refactored so it's closer to _posix_spawn, with some explanation about what is or isn't supported by _posix_spawn and why.

vstinner · 2019-09-25T13:57:13Z

As mentioned in the code review for Victor's fix, it feels like the big conditional on the use of self._posix_spawn in Popen._execute_child should perhaps be pulled into _posix_spawn, or at least refactored so it's closer to _posix_spawn, with some explanation about what is or isn't supported by _posix_spawn and why.

Maybe, yes. But having tests in the caller avoids to pass the very long list of arguments. Especially, it avoids to pass arguments which are not supposed by posix_spawn() :-)

_execute_child() has 21 arguments... is that really reasonable? Maybe we should put these stuff into a temporary object?

vstinner · 2019-09-25T13:57:51Z

The posix_spawn() bug is now fixed, I close again the issue.

Feel free to open a new issue if you want to follow-up on reworking the posix_spawn() code path ;-)

patrick-mclean mannequin added 3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Feb 20, 2019

gpshead self-assigned this Feb 21, 2019

izbyshev mannequin assigned gpshead and unassigned gpshead Feb 22, 2019

gpshead added 3.9 only security fixes and removed 3.8 only security fixes labels Sep 13, 2019

gpshead closed this as completed Sep 25, 2019

vstinner reopened this Sep 25, 2019

vstinner changed the title ~~support dropping privileges when running subprocesses~~ subprocess: add user, group and extra_groups parameters Sep 25, 2019

vstinner closed this as completed Sep 25, 2019

ezio-melotti transferred this issue from another repository Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subprocess: add user, group and extra_groups parameters #80227

subprocess: add user, group and extra_groups parameters #80227

patrick-mclean mannequin commented Feb 20, 2019

patrick-mclean mannequin commented Feb 20, 2019

izbyshev mannequin commented Feb 22, 2019

patrick-mclean mannequin commented Feb 25, 2019

izbyshev mannequin commented Feb 26, 2019

patrick-mclean mannequin commented Feb 26, 2019

patrick-mclean mannequin commented Feb 28, 2019

gpshead commented Mar 2, 2019

gpshead commented Sep 12, 2019

gpshead commented Sep 12, 2019

rhettinger commented Sep 13, 2019

gpshead commented Sep 13, 2019

gpshead commented Sep 13, 2019

gpshead commented Sep 13, 2019

vstinner commented Sep 13, 2019

vstinner commented Sep 13, 2019

Yhg1s commented Sep 13, 2019

vstinner commented Sep 25, 2019

Yhg1s commented Sep 25, 2019

Yhg1s commented Sep 25, 2019

vstinner commented Sep 25, 2019

vstinner commented Sep 25, 2019

subprocess: add user, group and extra_groups parameters #80227

subprocess: add user, group and extra_groups parameters #80227

Comments

patrick-mclean mannequin commented Feb 20, 2019

patrick-mclean mannequin commented Feb 20, 2019

izbyshev mannequin commented Feb 22, 2019

patrick-mclean mannequin commented Feb 25, 2019

izbyshev mannequin commented Feb 26, 2019

patrick-mclean mannequin commented Feb 26, 2019

patrick-mclean mannequin commented Feb 28, 2019

gpshead commented Mar 2, 2019

gpshead commented Sep 12, 2019

gpshead commented Sep 12, 2019

rhettinger commented Sep 13, 2019

gpshead commented Sep 13, 2019

gpshead commented Sep 13, 2019

gpshead commented Sep 13, 2019

vstinner commented Sep 13, 2019

vstinner commented Sep 13, 2019

Yhg1s commented Sep 13, 2019

vstinner commented Sep 25, 2019

Yhg1s commented Sep 25, 2019

Yhg1s commented Sep 25, 2019

vstinner commented Sep 25, 2019

vstinner commented Sep 25, 2019