[PATCH 1/4] security: filesystem capabilities bugfix1

[PATCH 1/4] security: filesystem capabilities bugfix1

[Andrew G. Morgan]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Bugfix for the fragile setuid fixup code in the case that filesystem
capabilities are supported.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFIY1eZ+bHCR3gb8jsRAgneAJ4jvnswg0+5Rkr69YFbFYXexK8vNQCgnAS7
jF5ZqrBAAtU7RNVHia18ODk=
=cOzB
-----END PGP SIGNATURE-----

[-- Attachment #2: 0001-Security-fix-for-experimental-filesystem-capability.patch --]
[-- Type: text/plain, Size: 6343 bytes --]

From a44789119274e6596f08f7d7b967130cf1ae7bb7 Mon Sep 17 00:00:00 2001
From: Andrew G. Morgan <morgan@kernel.org>
Date: Wed, 25 Jun 2008 23:12:32 -0700
Subject: [PATCH] Security fix for experimental filesystem capability code.

This commit includes a bugfix for the fragile setuid fixup code in
the case that filesystem capabilities are supported (in access()).
The effect of this fix is gated on filesystem capability support
because changing securebits is only supported when filesystem
capabilities support is configured.)

Signed-off-by: Andrew G. Morgan <morgan@kernel.org>
---
 fs/open.c                  |   38 +++++++++++++++++++++++---------------
 include/linux/capability.h |    2 ++
 include/linux/securebits.h |   15 ++++++++-------
 kernel/capability.c        |   21 +++++++++++++++++++++
 4 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index a145008..3b53948 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -16,6 +16,7 @@
 #include <linux/namei.h>
 #include <linux/backing-dev.h>
 #include <linux/capability.h>
+#include <linux/securebits.h>
 #include <linux/security.h>
 #include <linux/mount.h>
 #include <linux/vfs.h>
@@ -425,7 +426,7 @@ asmlinkage long sys_faccessat(int dfd, const char __user *filename, int mode)
 {
 	struct nameidata nd;
 	int old_fsuid, old_fsgid;
-	kernel_cap_t old_cap;
+	kernel_cap_t uninitialized_var(old_cap);  /* !SECURE_NO_SETUID_FIXUP */
 	int res;
 
 	if (mode & ~S_IRWXO)	/* where's F_OK, X_OK, W_OK, R_OK? */
@@ -433,23 +434,27 @@ asmlinkage long sys_faccessat(int dfd, const char __user *filename, int mode)
 
 	old_fsuid = current->fsuid;
 	old_fsgid = current->fsgid;
-	old_cap = current->cap_effective;
 
 	current->fsuid = current->uid;
 	current->fsgid = current->gid;
 
-	/*
-	 * Clear the capabilities if we switch to a non-root user
-	 *
-	 * FIXME: There is a race here against sys_capset.  The
-	 * capabilities can change yet we will restore the old
-	 * value below.  We should hold task_capabilities_lock,
-	 * but we cannot because user_path_walk can sleep.
-	 */
-	if (current->uid)
-		cap_clear(current->cap_effective);
-	else
-		current->cap_effective = current->cap_permitted;
+	if (!issecure(SECURE_NO_SETUID_FIXUP)) {
+		/*
+		 * Clear the capabilities if we switch to a non-root user
+		 */
+#ifndef CONFIG_SECURITY_FILE_CAPABILITIES
+		/*
+		 * FIXME: There is a race here against sys_capset.  The
+		 * capabilities can change yet we will restore the old
+		 * value below.  We should hold task_capabilities_lock,
+		 * but we cannot because user_path_walk can sleep.
+		 */
+#endif /* ndef CONFIG_SECURITY_FILE_CAPABILITIES */
+		if (current->uid)
+			old_cap = cap_set_effective(__cap_empty_set);
+		else
+			old_cap = cap_set_effective(current->cap_permitted);
+	}
 
 	res = __user_walk_fd(dfd, filename, LOOKUP_FOLLOW|LOOKUP_ACCESS, &nd);
 	if (res)
@@ -478,7 +483,10 @@ out_path_release:
 out:
 	current->fsuid = old_fsuid;
 	current->fsgid = old_fsgid;
-	current->cap_effective = old_cap;
+
+	if (!issecure(SECURE_NO_SETUID_FIXUP)) {
+		(void) cap_set_effective(old_cap);
+	}
 
 	return res;
 }
diff --git a/include/linux/capability.h b/include/linux/capability.h
index fa830f8..0267384 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -501,6 +501,8 @@ extern const kernel_cap_t __cap_empty_set;
 extern const kernel_cap_t __cap_full_set;
 extern const kernel_cap_t __cap_init_eff_set;
 
+kernel_cap_t cap_set_effective(const kernel_cap_t pE_new);
+
 int capable(int cap);
 int __capable(struct task_struct *t, int cap);
 
diff --git a/include/linux/securebits.h b/include/linux/securebits.h
index c1f19db..92f09bd 100644
--- a/include/linux/securebits.h
+++ b/include/linux/securebits.h
@@ -7,14 +7,15 @@
    inheritance of root-permissions and suid-root executable under
    compatibility mode. We raise the effective and inheritable bitmasks
    *of the executable file* if the effective uid of the new process is
-   0. If the real uid is 0, we raise the inheritable bitmask of the
+   0. If the real uid is 0, we raise the effective (legacy) bit of the
    executable file. */
 #define SECURE_NOROOT			0
 #define SECURE_NOROOT_LOCKED		1  /* make bit-0 immutable */
 
-/* When set, setuid to/from uid 0 does not trigger capability-"fixes"
-   to be compatible with old programs relying on set*uid to loose
-   privileges. When unset, setuid doesn't change privileges. */
+/* When set, setuid to/from uid 0 does not trigger capability-"fixup".
+   When unset, to provide compatiblility with old programs relying on
+   set*uid to gain/lose privilege, transitions to/from uid 0 cause
+   capabilities to be gained/lost. */
 #define SECURE_NO_SETUID_FIXUP		2
 #define SECURE_NO_SETUID_FIXUP_LOCKED	3  /* make bit-2 immutable */
 
@@ -26,10 +27,10 @@
 #define SECURE_KEEP_CAPS		4
 #define SECURE_KEEP_CAPS_LOCKED		5  /* make bit-4 immutable */
 
-/* Each securesetting is implemented using two bits. One bit specify
+/* Each securesetting is implemented using two bits. One bit specifies
    whether the setting is on or off. The other bit specify whether the
-   setting is fixed or not. A setting which is fixed cannot be changed
-   from user-level. */
+   setting is locked or not. A setting which is locked cannot be
+   changed from user-level. */
 #define issecure_mask(X)	(1 << (X))
 #define issecure(X)		(issecure_mask(X) & current->securebits)
 
diff --git a/kernel/capability.c b/kernel/capability.c
index cfbe442..901e0fd 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -121,6 +121,27 @@ static int cap_validate_magic(cap_user_header_t header, unsigned *tocopy)
  * uninteresting and/or not to be changed.
  */
 
+/*
+ * Atomically modify the effective capabilities returning the original
+ * value. No permission check is performed here - it is assumed that the
+ * caller is permitted to set the desired effective capabilities.
+ */
+kernel_cap_t cap_set_effective(const kernel_cap_t pE_new)
+{
+	kernel_cap_t pE_old;
+
+	spin_lock(&task_capability_lock);
+
+	pE_old = current->cap_effective;
+	current->cap_effective = pE_new;
+
+	spin_unlock(&task_capability_lock);
+
+	return pE_old;
+}
+
+EXPORT_SYMBOL(cap_set_effective);
+
 /**
  * sys_capget - get the capabilities of a given process.
  * @header: pointer to struct that contains capability version and
-- 
1.5.3.7

Re: [PATCH 1/4] security: filesystem capabilities bugfix1

[Serge E. Hallyn]

Quoting Andrew G. Morgan (morgan@kernel.org):
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Bugfix for the fragile setuid fixup code in the case that filesystem
> capabilities are supported.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.2.6 (GNU/Linux)
>
> iD8DBQFIY1eZ+bHCR3gb8jsRAgneAJ4jvnswg0+5Rkr69YFbFYXexK8vNQCgnAS7
> jF5ZqrBAAtU7RNVHia18ODk=
> =cOzB
> -----END PGP SIGNATURE-----

> From a44789119274e6596f08f7d7b967130cf1ae7bb7 Mon Sep 17 00:00:00 2001
> From: Andrew G. Morgan <morgan@kernel.org>
> Date: Wed, 25 Jun 2008 23:12:32 -0700
> Subject: [PATCH] Security fix for experimental filesystem capability code.
> 
> This commit includes a bugfix for the fragile setuid fixup code in
> the case that filesystem capabilities are supported (in access()).
> The effect of this fix is gated on filesystem capability support
> because changing securebits is only supported when filesystem
> capabilities support is configured.)
> 
> Signed-off-by: Andrew G. Morgan <morgan@kernel.org>

Acked-by: Serge Hallyn <serue@us.ibm.com>

> ---
>  fs/open.c                  |   38 +++++++++++++++++++++++---------------
>  include/linux/capability.h |    2 ++
>  include/linux/securebits.h |   15 ++++++++-------
>  kernel/capability.c        |   21 +++++++++++++++++++++
>  4 files changed, 54 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index a145008..3b53948 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -16,6 +16,7 @@
>  #include <linux/namei.h>
>  #include <linux/backing-dev.h>
>  #include <linux/capability.h>
> +#include <linux/securebits.h>
>  #include <linux/security.h>
>  #include <linux/mount.h>
>  #include <linux/vfs.h>
> @@ -425,7 +426,7 @@ asmlinkage long sys_faccessat(int dfd, const char __user *filename, int mode)
>  {
>  	struct nameidata nd;
>  	int old_fsuid, old_fsgid;
> -	kernel_cap_t old_cap;
> +	kernel_cap_t uninitialized_var(old_cap);  /* !SECURE_NO_SETUID_FIXUP */
>  	int res;
>  
>  	if (mode & ~S_IRWXO)	/* where's F_OK, X_OK, W_OK, R_OK? */
> @@ -433,23 +434,27 @@ asmlinkage long sys_faccessat(int dfd, const char __user *filename, int mode)
>  
>  	old_fsuid = current->fsuid;
>  	old_fsgid = current->fsgid;
> -	old_cap = current->cap_effective;
>  
>  	current->fsuid = current->uid;
>  	current->fsgid = current->gid;
>  
> -	/*
> -	 * Clear the capabilities if we switch to a non-root user
> -	 *
> -	 * FIXME: There is a race here against sys_capset.  The
> -	 * capabilities can change yet we will restore the old
> -	 * value below.  We should hold task_capabilities_lock,
> -	 * but we cannot because user_path_walk can sleep.
> -	 */
> -	if (current->uid)
> -		cap_clear(current->cap_effective);
> -	else
> -		current->cap_effective = current->cap_permitted;
> +	if (!issecure(SECURE_NO_SETUID_FIXUP)) {
> +		/*
> +		 * Clear the capabilities if we switch to a non-root user
> +		 */
> +#ifndef CONFIG_SECURITY_FILE_CAPABILITIES
> +		/*
> +		 * FIXME: There is a race here against sys_capset.  The
> +		 * capabilities can change yet we will restore the old
> +		 * value below.  We should hold task_capabilities_lock,
> +		 * but we cannot because user_path_walk can sleep.
> +		 */
> +#endif /* ndef CONFIG_SECURITY_FILE_CAPABILITIES */
> +		if (current->uid)
> +			old_cap = cap_set_effective(__cap_empty_set);
> +		else
> +			old_cap = cap_set_effective(current->cap_permitted);
> +	}
>  
>  	res = __user_walk_fd(dfd, filename, LOOKUP_FOLLOW|LOOKUP_ACCESS, &nd);
>  	if (res)
> @@ -478,7 +483,10 @@ out_path_release:
>  out:
>  	current->fsuid = old_fsuid;
>  	current->fsgid = old_fsgid;
> -	current->cap_effective = old_cap;
> +
> +	if (!issecure(SECURE_NO_SETUID_FIXUP)) {
> +		(void) cap_set_effective(old_cap);
> +	}
>  
>  	return res;
>  }
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index fa830f8..0267384 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -501,6 +501,8 @@ extern const kernel_cap_t __cap_empty_set;
>  extern const kernel_cap_t __cap_full_set;
>  extern const kernel_cap_t __cap_init_eff_set;
>  
> +kernel_cap_t cap_set_effective(const kernel_cap_t pE_new);
> +
>  int capable(int cap);
>  int __capable(struct task_struct *t, int cap);
>  
> diff --git a/include/linux/securebits.h b/include/linux/securebits.h
> index c1f19db..92f09bd 100644
> --- a/include/linux/securebits.h
> +++ b/include/linux/securebits.h
> @@ -7,14 +7,15 @@
>     inheritance of root-permissions and suid-root executable under
>     compatibility mode. We raise the effective and inheritable bitmasks
>     *of the executable file* if the effective uid of the new process is
> -   0. If the real uid is 0, we raise the inheritable bitmask of the
> +   0. If the real uid is 0, we raise the effective (legacy) bit of the
>     executable file. */
>  #define SECURE_NOROOT			0
>  #define SECURE_NOROOT_LOCKED		1  /* make bit-0 immutable */
>  
> -/* When set, setuid to/from uid 0 does not trigger capability-"fixes"
> -   to be compatible with old programs relying on set*uid to loose
> -   privileges. When unset, setuid doesn't change privileges. */
> +/* When set, setuid to/from uid 0 does not trigger capability-"fixup".
> +   When unset, to provide compatiblility with old programs relying on
> +   set*uid to gain/lose privilege, transitions to/from uid 0 cause
> +   capabilities to be gained/lost. */
>  #define SECURE_NO_SETUID_FIXUP		2
>  #define SECURE_NO_SETUID_FIXUP_LOCKED	3  /* make bit-2 immutable */
>  
> @@ -26,10 +27,10 @@
>  #define SECURE_KEEP_CAPS		4
>  #define SECURE_KEEP_CAPS_LOCKED		5  /* make bit-4 immutable */
>  
> -/* Each securesetting is implemented using two bits. One bit specify
> +/* Each securesetting is implemented using two bits. One bit specifies
>     whether the setting is on or off. The other bit specify whether the
> -   setting is fixed or not. A setting which is fixed cannot be changed
> -   from user-level. */
> +   setting is locked or not. A setting which is locked cannot be
> +   changed from user-level. */
>  #define issecure_mask(X)	(1 << (X))
>  #define issecure(X)		(issecure_mask(X) & current->securebits)
>  
> diff --git a/kernel/capability.c b/kernel/capability.c
> index cfbe442..901e0fd 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -121,6 +121,27 @@ static int cap_validate_magic(cap_user_header_t header, unsigned *tocopy)
>   * uninteresting and/or not to be changed.
>   */
>  
> +/*
> + * Atomically modify the effective capabilities returning the original
> + * value. No permission check is performed here - it is assumed that the
> + * caller is permitted to set the desired effective capabilities.
> + */
> +kernel_cap_t cap_set_effective(const kernel_cap_t pE_new)
> +{
> +	kernel_cap_t pE_old;
> +
> +	spin_lock(&task_capability_lock);
> +
> +	pE_old = current->cap_effective;
> +	current->cap_effective = pE_new;
> +
> +	spin_unlock(&task_capability_lock);
> +
> +	return pE_old;
> +}
> +
> +EXPORT_SYMBOL(cap_set_effective);
> +
>  /**
>   * sys_capget - get the capabilities of a given process.
>   * @header: pointer to struct that contains capability version and
> -- 
> 1.5.3.7
> 

Re: [PATCH 1/4] security: filesystem capabilities bugfix1

[David Howells]

Andrew G. Morgan <morgan@kernel.org> wrote:

> +kernel_cap_t cap_set_effective(const kernel_cap_t pE_new)

Hmmm... kernel_cap_t is a structure that might not fit into a single register.
It occurs to me that you might be better off returing the old caps through a
pointer argument.  It should still work, though.

The SElinux and syscalls LTP testscripts work okay for these patches when
ported to linux-next.  The filecaps LTP testscript can't be used on Fedora 9
as the setcap command is used incorrectly.

Acked-by: David Howells <dhowells@redhat.com>

David

Re: [PATCH 1/4] security: filesystem capabilities bugfix1

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> > +kernel_cap_t cap_set_effective(const kernel_cap_t pE_new)
> 
> Hmmm... kernel_cap_t is a structure that might not fit into a single register.
> It occurs to me that you might be better off returing the old caps through a
> pointer argument.

Apply something like the attached, perhaps?  (Note the attached patch is
missing the change from linux/capability.h because my version is not
compatible with your submitted patch).

David

diff --git a/fs/open.c b/fs/open.c
index 3b53948..24b4a11 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -451,9 +451,9 @@ asmlinkage long sys_faccessat(int dfd, const char __user *filename, int mode)
 		 */
 #endif /* ndef CONFIG_SECURITY_FILE_CAPABILITIES */
 		if (current->uid)
-			old_cap = cap_set_effective(__cap_empty_set);
+			cap_set_effective(&__cap_empty_set, &old_cap);
 		else
-			old_cap = cap_set_effective(current->cap_permitted);
+			cap_set_effective(&current->cap_permitted, &old_cap);
 	}
 
 	res = __user_walk_fd(dfd, filename, LOOKUP_FOLLOW|LOOKUP_ACCESS, &nd);
@@ -484,9 +484,8 @@ out:
 	current->fsuid = old_fsuid;
 	current->fsgid = old_fsgid;
 
-	if (!issecure(SECURE_NO_SETUID_FIXUP)) {
-		(void) cap_set_effective(old_cap);
-	}
+	if (!issecure(SECURE_NO_SETUID_FIXUP))
+		cap_set_effective(&old_cap, NULL);
 
 	return res;
 }
diff --git a/kernel/capability.c b/kernel/capability.c
index c3bf957..13c496a 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -126,18 +126,16 @@ static int cap_validate_magic(cap_user_header_t header, unsigned *tocopy)
  * value. No permission check is performed here - it is assumed that the
  * caller is permitted to set the desired effective capabilities.
  */
-kernel_cap_t cap_set_effective(const kernel_cap_t pE_new)
+void cap_set_effective(const kernel_cap_t *pE_new,
+		       kernel_cap_t *_pE_old)
 {
-	kernel_cap_t pE_old;
-
 	spin_lock(&task_capability_lock);
 
-	pE_old = current->cap_effective;
-	current->cap_effective = pE_new;
+	if (_pE_old)
+		*_pE_old = current->cap_effective;
+	current->cap_effective = *pE_new;
 
 	spin_unlock(&task_capability_lock);
-
-	return pE_old;
 }
 
 EXPORT_SYMBOL(cap_set_effective);

Re: [PATCH 1/4] security: filesystem capabilities bugfix1

[Andrew Morton]

grumbles.

On Thu, 26 Jun 2008 01:47:21 -0700
"Andrew G. Morgan" <morgan@kernel.org> wrote:

> Subject: [PATCH 1/4] security: filesystem capabilities bugfix1

"bugfix1" is not exactly a high-quality description of this change. 
Please put more thought into the patch titles.  I cooked up something
randomly sensible-looking for these changes.

> Date: Thu, 26 Jun 2008 01:47:21 -0700
> User-Agent: Thunderbird 2.0.0.14 (X11/20080421)
> 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Bugfix for the fragile setuid fixup code in the case that filesystem
> capabilities are supported.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.2.6 (GNU/Linux)
> 
> iD8DBQFIY1eZ+bHCR3gb8jsRAgneAJ4jvnswg0+5Rkr69YFbFYXexK8vNQCgnAS7
> jF5ZqrBAAtU7RNVHia18ODk=
> =cOzB
> -----END PGP SIGNATURE-----

Would much prefer that the above gobbledygook not be there, please.  It
causes me to go through a spectacular number of manual steps to be able
to extricate the diff, and it makes it hard to reply to the diff when
commenting on it.

Plain old text/plain with an inlined patch is much more user-friendly.

And the PGP signing doesn't gain us anything because I'd cheerfully
apply a patch from a faked "Andrew Morgan" anyway.  I'd expect the
real Andrew Morgan to squawk when he sees me apply a patch which he
didn't send.

<clicks on attachments, right-clicks, does save-as to a temp file,
reads the temp file back in, does s/^/^> /g)

> +	if (!issecure(SECURE_NO_SETUID_FIXUP)) {
> +		(void) cap_set_effective(old_cap);
> +	}

The unneeded braces and void cast are not in the preferred style.  I
zapped them.

Re: [PATCH 1/4] security: filesystem capabilities bugfix1

[Andrew Morton]

I'll assume that patches 1 and 2 are needed in 2.6.26 and that
patches 3 and 4 are for 2.6.27.

Patches 1 and/or 2 caused a reject in
security-protect-legacy-applications-from-executing-with-insufficient-privilege.patch:

***************
*** 350,357 ****
  				bprm->e_gid = current->gid;
  			}
  			if (!capable (CAP_SETPCAP)) {
- 				new_permitted = cap_intersect (new_permitted,
- 							current->cap_permitted);
  			}
  		}
  	}
--- 364,372 ----
  				bprm->e_gid = current->gid;
  			}
  			if (!capable (CAP_SETPCAP)) {
+ 				bprm->cap_post_exec_permitted = cap_intersect(
+ 					bprm->cap_post_exec_permitted,
+ 					current->cap_permitted);
  			}
  		}
  	}

which looked simple to fix.  I'll resend the resulting diff - please
check.