[RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve

[RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve

[Igor Lubashev]

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

-- 
2.7.4

[RFC PATCH 1/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve

[Igor Lubashev]

Many kernel interfaces require real and/or effective root uid instead
of relying solely of capabilities. An executable that uses such
interfaces has to be set-uid-root or be executed by a thread with
effective root uid. Presently, fsuid and saved uid will reset to the
effective uid during execve. As a result, it is not possible to
execute a binary such that it has effective root uid but retains
fsuid/fsgid of the actual user. Retaining fsuid/fsgid of the actual
user could be required if the executable needs to access the
filesystem. It may also be desired from the security perspective of
delegating the minimal set of privileges.

Setting SECURE_KEEP_FSUID bit ensures that the current fsuid/fsgiud is
retained by execve.

Signed-off-by: Igor Lubashev <ilubashe@akamai.com>
---
 include/uapi/linux/securebits.h | 10 +++++++++-
 security/commoncap.c            |  9 +++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
index d6d98877ff1a..6c20b2287d6f 100644
--- a/include/uapi/linux/securebits.h
+++ b/include/uapi/linux/securebits.h
@@ -52,10 +52,18 @@
 #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
 			(issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
 
+/* When set, execve does not reset fsuid/fsgid to euid/egid */
+#define SECURE_KEEP_FSUID		8
+#define SECURE_KEEP_FSUID_LOCKED	9  /* make bit-8 immutable */
+
+#define SECBIT_KEEP_FSUID	 (issecure_mask(SECURE_KEEP_FSUID))
+#define SECBIT_KEEP_FSUID_LOCKED (issecure_mask(SECURE_KEEP_FSUID_LOCKED))
+
 #define SECURE_ALL_BITS		(issecure_mask(SECURE_NOROOT) | \
 				 issecure_mask(SECURE_NO_SETUID_FIXUP) | \
 				 issecure_mask(SECURE_KEEP_CAPS) | \
-				 issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
+				 issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
+				 issecure_mask(SECURE_KEEP_FSUID))
 #define SECURE_ALL_LOCKS	(SECURE_ALL_BITS << 1)
 
 #endif /* _UAPI_LINUX_SECUREBITS_H */
diff --git a/security/commoncap.c b/security/commoncap.c
index c0b9664ee49e..e4de823a1d4e 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -847,8 +847,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 						   old->cap_permitted);
 	}
 
-	new->suid = new->fsuid = new->euid;
-	new->sgid = new->fsgid = new->egid;
+	new->suid = new->euid;
+	new->sgid = new->egid;
+
+	if (!issecure(SECURE_KEEP_FSUID)) {
+		new->fsuid = new->euid;
+		new->fsgid = new->egid;
+	}
 
 	/* File caps or setid cancels ambient. */
 	if (has_fcap || is_setid)
-- 
2.7.4

Re: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve

[James Morris]

[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
<jmorris@namei.org>

Re: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve

[James Morris]

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
<jmorris@namei.org>

RE: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve

[Lubashev, Igor]

> 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

[James Morris]

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
<jmorris@namei.org>

RE: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve

[Lubashev, Igor]

> 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