Alexey Izbyshev <izbys...@ispras.ru> added the comment:
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
----------
assignee: gregory.p.smith ->
_______________________________________
Python tracker <rep...@bugs.python.org>
<https://bugs.python.org/issue36046>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
