RE: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
> From: James Morris 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
RE: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
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.
--
James Morris
RE: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
> 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
Re: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
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? -- James Morris
Re: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
[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(-) > > -- James Morris
