This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author izbyshev
Recipients giampaolo.rodola, gregory.p.smith, izbyshev, patrick.mclean
Date 2019-02-22.21:48:46
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1550872127.12.0.591009141083.issue36046@roundup.psfhosted.org>
In-reply-to
Content
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
History
Date User Action Args
2019-02-22 21:48:47izbyshevsetrecipients: + izbyshev, gregory.p.smith, giampaolo.rodola, patrick.mclean
2019-02-22 21:48:47izbyshevsetmessageid: <1550872127.12.0.591009141083.issue36046@roundup.psfhosted.org>
2019-02-22 21:48:47izbyshevlinkissue36046 messages
2019-02-22 21:48:46izbyshevcreate