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.

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:

1) 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?

2) 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?

3) 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

Alexey, here are my responses to your points:

1) 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.

2) 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.

> 1) 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.

> 2) 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?

> 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.

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

I like the separate parameters. :)

New changeset 2b2ead74382513d0bb9ef34504e283a71e6a706f by Gregory P. Smith (Patrick McLean) in branch 'master':
bpo-36046: Add user and group parameters to subprocess (GH-11950)
https://github.com/python/cpython/commit/2b2ead74382513d0bb9ef34504e283a71e6a706f

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

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.

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'

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'"

AIX has the same error as RHEL and Fedora

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

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

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)

New changeset 693aa80a434590ea7dcd35c000209e53d01b9425 by T. Wouters (Gregory P. Smith) in branch 'master':
bpo-36046: Fix buildbot failures (GH-16091)
https://github.com/python/cpython/commit/693aa80a434590ea7dcd35c000209e53d01b9425

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).

New changeset faca8553425c231d867dcabf6a69a9dd21118b6c by T. Wouters (Victor Stinner) in branch 'master':
bpo-36046: posix_spawn() doesn't support uid/gid (GH-16384)
https://github.com/python/cpython/commit/faca8553425c231d867dcabf6a69a9dd21118b6c

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.

> 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?

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 ;-)