| Message ID | 1560473087-27754-1-git-send-email-ilubashe@akamai.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve | expand |
[Adding David and Al] On Thu, 13 Jun 2019, Igor Lubashev wrote: > I've posted this in March but received no response. Reposting. > > This patch introduces SECURE_KEEP_FSUID to allow fsuid/fsgid to be > preserved across execve. It is currently impossible to execve a > program such that effective and filesystem uid differ. > > The need for this functionality arose from a desire to allow certain > non-privileged users to run perf. To do this, we install perf without > set-uid-root and have a set-uid-root wrapper decide who is allowed to > run perf (and with what arguments). > > The wrapper must execve perf with real and effective root uid, because > perf and KASLR require this. However, that presently resets fsuid to > root, giving the user ability to read and overwrite any file owned by > root (perf report -i, perf record -o). Also, perf record will create > perf.data that cannot be deleted by the user. > > We cannot reset /proc/sys/kernel/perf_event_paranoid to a permissive > level, since we must be selective which users have the permissions. > > Of course, we could fix our problem by a patch to perf to allow > passing a username on the command line and having perf execute > setfsuid before opening files. However, perf is not the only program > that uses kernel features that require root uid/euid, so a general > solution that does not involve updating all such programs seems > warranted. > > I will update man pages, if this patch is deemed a good idea. > > Igor Lubashev (1): > security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve > > include/uapi/linux/securebits.h | 10 +++++++++- > security/commoncap.c | 9 +++++++-- > 2 files changed, 16 insertions(+), 3 deletions(-) > >
On Thu, 13 Jun 2019, Igor Lubashev wrote: > I've posted this in March but received no response. Reposting. > > This patch introduces SECURE_KEEP_FSUID to allow fsuid/fsgid to be > preserved across execve. It is currently impossible to execve a > program such that effective and filesystem uid differ. > > The need for this functionality arose from a desire to allow certain > non-privileged users to run perf. To do this, we install perf without > set-uid-root and have a set-uid-root wrapper decide who is allowed to > run perf (and with what arguments). > > The wrapper must execve perf with real and effective root uid, because > perf and KASLR require this. However, that presently resets fsuid to > root, giving the user ability to read and overwrite any file owned by > root (perf report -i, perf record -o). Also, perf record will create > perf.data that cannot be deleted by the user. > > We cannot reset /proc/sys/kernel/perf_event_paranoid to a permissive > level, since we must be selective which users have the permissions. > > Of course, we could fix our problem by a patch to perf to allow > passing a username on the command line and having perf execute > setfsuid before opening files. However, perf is not the only program > that uses kernel features that require root uid/euid, so a general > solution that does not involve updating all such programs seems > warranted. This seems like a very specific corner case, depending on fsuid!=0 for an euid=0 process, along with a whitelist policy for perf arguments. It would be a great way to escalate to root via a bug in an executed app or via a wrapper misconfiguration. It also adds complexity to kernel credential handling -- it's yet another thing to consider when trying to reason about this. Have you considered the example security configuration in Documentation/admin-guide/perf-security.rst ? What are some other examples of programs that could utilize this scheme?
> On Friday, June 14, 2019, James Morris wrote: > On Thu, 13 Jun 2019, Igor Lubashev wrote: > > > I've posted this in March but received no response. Reposting. > > > > This patch introduces SECURE_KEEP_FSUID to allow fsuid/fsgid to be > > preserved across execve. It is currently impossible to execve a > > program such that effective and filesystem uid differ. > > > > The need for this functionality arose from a desire to allow certain > > non-privileged users to run perf. To do this, we install perf without > > set-uid-root and have a set-uid-root wrapper decide who is allowed to > > run perf (and with what arguments). > > > > The wrapper must execve perf with real and effective root uid, because > > perf and KASLR require this. However, that presently resets fsuid to > > root, giving the user ability to read and overwrite any file owned by > > root (perf report -i, perf record -o). Also, perf record will create > > perf.data that cannot be deleted by the user. > > > > We cannot reset /proc/sys/kernel/perf_event_paranoid to a permissive > > level, since we must be selective which users have the permissions. > > > > Of course, we could fix our problem by a patch to perf to allow > > passing a username on the command line and having perf execute > > setfsuid before opening files. However, perf is not the only program > > that uses kernel features that require root uid/euid, so a general > > solution that does not involve updating all such programs seems > > warranted. > This seems like a very specific corner case, depending on fsuid!=0 for an > euid=0 process, along with a whitelist policy for perf arguments. It would be a > great way to escalate to root via a bug in an executed app or via a wrapper > misconfiguration. Any set-uid-root app is a hazard. This wrapper's purpose is to reduce the risk inherent in running apps with elevated privs. It removes all capabilities (CAT_SETUID, CAT_SETPCAP, CAP_DAC_OVERRIDE, etc.) except for the required ones before execve(). It has a list of users allowed run apps (and a per-user whitelist of arguments, and it manages resources and time used by apps). The wrapper works great for things like tcpdump -- it is executed with the user's uid and just CAP_NET_CAP and CAP_NET_ADMIN set. Unfortunately, perf is using uid==0 and euid==0 as a "capability bits". In tools/perf/util/evsel.c: static bool perf_event_can_profile_kernel(void) { return geteuid() == 0 || perf_event_paranoid() == -1; } In tools/perf/util/symbol.c: static bool symbol__read_kptr_restrict(void) { ... value = ((geteuid() != 0) || (getuid() != 0)) ? (atoi(line) != 0) : (atoi(line) == 2); ... } > Have you considered the example security configuration in > Documentation/admin-guide/perf-security.rst ? Unfortunately, this configuration does not work, unless you reset /proc/sys/kernel/perf_event_paranoid to a permissive level (see code above). We have perf_event_paranoid set to 2. If it worked, we could had implemented the same capability-based policy in the wrapper. > It also adds complexity to kernel credential handling -- it's yet another thing > to consider when trying to reason about this. I really wish that there were only two concepts that mattered: capability sets and fsuid/fsgid. The proposed patch allows one to switch to such mode -- a much simpler mode. Yes, the patch does add a "new feature", but what matters most for the complexity question is whether this feature is a move in the right direction. I am leaning that way, but I am not 100% positive -- hence this RFC patch. > What are some other examples of programs that could utilize this scheme? That's everything, like our wrapper, that needs to allow non-root users to run apps (like perf) that use uid/euid as capabilities. It is a required, if the apps could interact with a filesystem (and accessing root-owned files is not a desired effect). It is a good idea from the security perspective even if those apps do not normally interact with a filesystem. I do not have a clear view about a variety of Linux apps ever written, but I suspect that there are many apps that fall into "use uid/euid as capabilities" category. There are at least two in the kernel's tools directory. There is also use of uid/eiud in the kernel itself, and anything that uses this functionality cannot be fixed w/o fixing the kernel. It may be a bit hard to find all such uses, but a good start is: grep -rE '(uid_eq|uid\(\)).*\b(GLOBAL_ROOT_ID|0)\b' - Igor
On Sat, 15 Jun 2019, Lubashev, Igor wrote: > > On Friday, June 14, 2019, James Morris wrote: > Unfortunately, perf is using uid==0 and euid==0 as a "capability bits". > > > In tools/perf/util/evsel.c: > static bool perf_event_can_profile_kernel(void) > { > return geteuid() == 0 || perf_event_paranoid() == -1; > } > > In tools/perf/util/symbol.c: > static bool symbol__read_kptr_restrict(void) > { > ... > value = ((geteuid() != 0) || (getuid() != 0)) ? > (atoi(line) != 0) : > (atoi(line) == 2); > ... > } These are bugs. They should be checking for CAP_SYS_ADMIN. > > > Have you considered the example security configuration in > > Documentation/admin-guide/perf-security.rst ? > > Unfortunately, this configuration does not work, unless you reset > /proc/sys/kernel/perf_event_paranoid to a permissive level (see code > above). We have perf_event_paranoid set to 2. If it worked, we could had > implemented the same capability-based policy in the wrapper. This is not necessary for a process which has CAP_SYS_ADMIN.
> From: James Morris <jmorris@namei.org> on Friday, June 14, 2019 11:54 PM: > On Sat, 15 Jun 2019, Lubashev, Igor wrote: > > > Unfortunately, perf is using uid==0 and euid==0 as a "capability bits". > > > > > > In tools/perf/util/evsel.c: > > static bool perf_event_can_profile_kernel(void) > > { > > return geteuid() == 0 || perf_event_paranoid() == -1; > > } > > > > In tools/perf/util/symbol.c: > > static bool symbol__read_kptr_restrict(void) > > { > > ... > > value = ((geteuid() != 0) || (getuid() != 0)) ? > > (atoi(line) != 0) : > > (atoi(line) == 2); > > ... > > } > > These are bugs. They should be checking for CAP_SYS_ADMIN. Thanks for the suggestion. Actually, the former one should be checking CAP_SYS_ADMIN, while the latter -- CAP_SYSLOG (see lib/vsprintf.c). Just posted a patch to perf (http://lists.infradead.org/pipermail/linux-arm-kernel/2019-July/664552.html). Thank you, - Igor