prctl(PR_SET_KEEPCAPS, ...) is broken for some configs, e.g. CONFIG_SECURITY_SELINUX

The following move-it-back-to-generic-place patch fixes the problem.

---
From: Dmitry Adamushko <[email protected]>
Subject: fix prctl()'s handling of PR_{SET,GET}_KEEPCAPS

with the commit 3898b1b4ebff8dcfbcf1807e0661585e06c9a91c

prctl(PR_SET_KEEPCAPS, {1 | 0}, 0, 0, 0);

always returns -EINVAL for the following configs:

1) CONFIG_SECURITY but without any of CONFIG_SECURITY_* modules;

2) CONFIG_SECURITY + CONFIG_SECURITY_SELINUX + CONFIG_SECURITY_SELINUX_DISABLE

both fall back to 'dummy' implementation.

3) CONFIG_SECURITY + CONFIG_SECURITY_SELINUX

for this config it will work when there is a secondary security module.

Here is what happens:

Processing of PR_SET_KEEPCAPS (and a couple of other options) has been moved from kernel/sys.c::sys_prctl() to security/commoncap.c::cap_task_prctl().

For the aforementioned configs cap_task_prctl() is not called (moreover, security/commoncap.c is not compiled).

SELinux's implementation of .task_prctl callback resorts to secondary_ops->task_prctl() which is dummy_task_prctl() (in the absence of CONFIG_SECURITY_CAPABILITIES (or any other) as a secondary module).

So the relevant code should be either moved back to sys_prctl() or placed in some generic function (not in security/commoncap.c) which is accessible for all configs.

Move it back to sys_prctl().

----

index 14e9728..5b8e583 100644
@@ -24,6 +24,7 @@
 #include <linux/times.h>
 #include <linux/posix-timers.h>
 #include <linux/security.h>
+#include <linux/securebits.h>
 #include <linux/dcookies.h>
 #include <linux/suspend.h>
 #include <linux/tty.h>

@@ -1658,6 +1659,21 @@ asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3,
 		return error;
 
 	switch (option) {
+		case PR_GET_KEEPCAPS:
+			if (issecure(SECURE_KEEP_CAPS))
+				error = 1;
+			break;
+		case PR_SET_KEEPCAPS:
+			if (arg2 > 1) /* Note, we rely on arg2 being unsigned here */
+				error = -EINVAL;
+			else if (issecure(SECURE_KEEP_CAPS_LOCKED))
+				error = -EPERM;
+			else if (arg2)
+				current->securebits |= issecure_mask(SECURE_KEEP_CAPS);
+			else
+				current->securebits &=
+					~issecure_mask(SECURE_KEEP_CAPS);
+			break;
 		case PR_SET_PDEATHSIG:
 			if (!valid_signal(arg2)) {
 				error = -EINVAL;

@@ -1744,6 +1760,12 @@ asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3,
 		case PR_SET_TSC:
 			error = SET_TSC_CTL(arg2);
 			break;
+		case PR_CAPBSET_READ:
+			if (!cap_valid(arg2))
+				error = -EINVAL;
+			else
+				error = !!cap_raised(current->cap_bset, arg2);
+			break;
 		default:
 			error = -EINVAL;
 			break;

index 5edabc7..76f3a76 100644
@@ -576,12 +576,6 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 	long error = 0;
 
 	switch (option) {
-	case PR_CAPBSET_READ:
-		if (!cap_valid(arg2))
-			error = -EINVAL;
-		else
-			error = !!cap_raised(current->cap_bset, arg2);
-		break;
 #ifdef CONFIG_SECURITY_FILE_CAPABILITIES
 	case PR_CAPBSET_DROP:
 		error = cap_prctl_drop(arg2);

@@ -631,22 +625,6 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 
 #endif /* def CONFIG_SECURITY_FILE_CAPABILITIES */
 
-	case PR_GET_KEEPCAPS:
-		if (issecure(SECURE_KEEP_CAPS))
-			error = 1;
-		break;
-	case PR_SET_KEEPCAPS:
-		if (arg2 > 1) /* Note, we rely on arg2 being unsigned here */
-			error = -EINVAL;
-		else if (issecure(SECURE_KEEP_CAPS_LOCKED))
-			error = -EPERM;
-		else if (arg2)
-			current->securebits |= issecure_mask(SECURE_KEEP_CAPS);
-		else
-			current->securebits &=
-				~issecure_mask(SECURE_KEEP_CAPS);
-		break;
-
 	default:
 		/* No functionality available - continue with default */
 		return 0;

---

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

In a configuration in which you are not using capabilities, what is the "keep capabilities" operation supposed to do? Lie to you?

http://bugzilla.kernel.org/show_bug.cgi?id=10748

Cheers

Andrew

Dmitry Adamushko wrote:

|
| The following move-it-back-to-generic-place patch fixes the problem.
|
|
| ---
| From: Dmitry Adamushko <[email protected]>
| Subject: fix prctl()'s handling of PR_{SET,GET}_KEEPCAPS
|
| with the commit 3898b1b4ebff8dcfbcf1807e0661585e06c9a91c
|
| prctl(PR_SET_KEEPCAPS, {1 | 0}, 0, 0, 0);
|
| always returns -EINVAL for the following configs:
|
| 1) CONFIG_SECURITY but without any of CONFIG_SECURITY_* modules;
|
| 2) CONFIG_SECURITY + CONFIG_SECURITY_SELINUX +
CONFIG_SECURITY_SELINUX_DISABLE
|
| both fall back to 'dummy' implementation.
|
| 3) CONFIG_SECURITY + CONFIG_SECURITY_SELINUX
|
| for this config it will work when there is a secondary security module.
|
| Here is what happens:
|
| Processing of PR_SET_KEEPCAPS (and a couple of other options) has been
| moved from kernel/sys.c::sys_prctl() to
security/commoncap.c::cap_task_prctl().
|
| For the aforementioned configs cap_task_prctl() is not called
| (moreover, security/commoncap.c is not compiled).
|
| SELinux's implementation of .task_prctl callback resorts to
| secondary_ops->task_prctl() which is dummy_task_prctl() (in the
| absence of CONFIG_SECURITY_CAPABILITIES (or any other) as a secondary
| module).
|
| So the relevant code should be either moved back to sys_prctl() or
| placed in some generic function (not in security/commoncap.c) which is
| accessible for all configs.
|
| Move it back to sys_prctl().
|
| Signed-off-by: Dmitry Adamushko <[email protected]>
|
| ----
|
| diff --git a/kernel/sys.c b/kernel/sys.c
| index 14e9728..5b8e583 100644
| --- a/kernel/sys.c
| +++ b/kernel/sys.c
| @@ -24,6 +24,7 @@
|  #include <linux/times.h>
|  #include <linux/posix-timers.h>
|  #include <linux/security.h>
| +#include <linux/securebits.h>
|  #include <linux/dcookies.h>
|  #include <linux/suspend.h>
|  #include <linux/tty.h>
| @@ -1658,6 +1659,21 @@ asmlinkage long sys_prctl(int option, unsigned
long arg2, unsigned long arg3,
|  		return error;
|
|  	switch (option) {
| +		case PR_GET_KEEPCAPS:
| +			if (issecure(SECURE_KEEP_CAPS))
| +				error = 1;
| +			break;
| +		case PR_SET_KEEPCAPS:
| +			if (arg2 > 1) /* Note, we rely on arg2 being unsigned here */
| +				error = -EINVAL;
| +			else if (issecure(SECURE_KEEP_CAPS_LOCKED))
| +				error = -EPERM;
| +			else if (arg2)
| +				current->securebits |= issecure_mask(SECURE_KEEP_CAPS);
| +			else
| +				current->securebits &=
| +					~issecure_mask(SECURE_KEEP_CAPS);
| +			break;
|  		case PR_SET_PDEATHSIG:
|  			if (!valid_signal(arg2)) {
|  				error = -EINVAL;
| @@ -1744,6 +1760,12 @@ asmlinkage long sys_prctl(int option, unsigned
long arg2, unsigned long arg3,
|  		case PR_SET_TSC:
|  			error = SET_TSC_CTL(arg2);
|  			break;
| +		case PR_CAPBSET_READ:
| +			if (!cap_valid(arg2))
| +				error = -EINVAL;
| +			else
| +				error = !!cap_raised(current->cap_bset, arg2);
| +			break;
|  		default:
|  			error = -EINVAL;
|  			break;
| diff --git a/security/commoncap.c b/security/commoncap.c
| index 5edabc7..76f3a76 100644
| --- a/security/commoncap.c
| +++ b/security/commoncap.c
| @@ -576,12 +576,6 @@ int cap_task_prctl(int option, unsigned long
arg2, unsigned long arg3,
|  	long error = 0;
|
|  	switch (option) {
| -	case PR_CAPBSET_READ:
| -		if (!cap_valid(arg2))
| -			error = -EINVAL;
| -		else
| -			error = !!cap_raised(current->cap_bset, arg2);
| -		break;
|  #ifdef CONFIG_SECURITY_FILE_CAPABILITIES
|  	case PR_CAPBSET_DROP:
|  		error = cap_prctl_drop(arg2);
| @@ -631,22 +625,6 @@ int cap_task_prctl(int option, unsigned long
arg2, unsigned long arg3,
|
|  #endif /* def CONFIG_SECURITY_FILE_CAPABILITIES */
|
| -	case PR_GET_KEEPCAPS:
| -		if (issecure(SECURE_KEEP_CAPS))
| -			error = 1;
| -		break;
| -	case PR_SET_KEEPCAPS:
| -		if (arg2 > 1) /* Note, we rely on arg2 being unsigned here */
| -			error = -EINVAL;
| -		else if (issecure(SECURE_KEEP_CAPS_LOCKED))
| -			error = -EPERM;
| -		else if (arg2)
| -			current->securebits |= issecure_mask(SECURE_KEEP_CAPS);
| -		else
| -			current->securebits &=
| -				~issecure_mask(SECURE_KEEP_CAPS);
| -		break;
| -
|  	default:
|  		/* No functionality available - continue with default */
|  		return 0;
|
| ---
|

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIS/Zf+bHCR3gb8jsRAqY0AKCX9tOKFdyc8IuCZS22JQH36SzVTQCfTtuS GKGXZut41bhPGj2WPeh61DU=
=NvfJ
-----END PGP SIGNATURE-----

I totally agree with comment 11 there. Quite a number of people have already hit this and more surely will. How can we help them (and hence us)?

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

What do people think about comment #8 from Stephen Smalley?:

The dummy module is generally in the untenable position of having to lie to userspace or break the existing capability-related system call interface. It should just go away, and make capability the default module (w/ stubs for the rest of the LSM hooks as with dummy). Then CONFIG_SECURITY=n will yield the same result as CONFIG_SECURITY=y w/o any further options.

Cheers

Andrew

Andrew Morton wrote:
| On Sun, 08 Jun 2008 08:10:26 -0700 Andrew Morgan <[email protected]> wrote:

|
|> Nacked-by: Andrew G. Morgan <[email protected]>
|>
|> In a configuration in which you are not using capabilities, what is the
|> "keep capabilities" operation supposed to do? Lie to you?
|>
|> http://bugzilla.kernel.org/show_bug.cgi?id=10748
|
| I totally agree with comment 11 there. Quite a number of people have

already
| hit this and more surely will. How can we help them (and hence us)? -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFITF6D+bHCR3gb8jsRAlLKAKCjwR7JthL+7+EOWEHzGXv1XptfPACgvT0v n9WeEJNiuVp0usx1EzKkT8A=
=/D3k
-----END PGP SIGNATURE-----

(removed pgp crap, undid top-posting. Your emails are very hard to reply to)

It's a fine comment, but I am not knowledgeable enough in this area to say whether it's a desirable thing to do for 2.6.26.

I fear that nothing will happen, and we'll end up wasting a lot of peoples' time sending hey-why-did-my-dhcp-break reports.

If we decide to get rid of dummy long-term, then it's far less distasteful to have it lie and claim the keepcaps worked in the meantime.

So for 2.6.26 we could have dummy lie, then plan to make capabilities the default for 2.6.27?

-serge

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

I agree. Short term, here is a patch to add dummy support for KEEPCAPS.

Cheers

Andrew

Serge E. Hallyn wrote:

|>> I fear that nothing will happen, and we'll end up wasting a lot of
|> peoples' time sending hey-why-did-my-dhcp-break reports.
|
| If we decide to get rid of dummy long-term, then it's far less
| distasteful to have it lie and claim the keepcaps worked in the
| meantime.
|
| So for 2.6.26 we could have dummy lie, then plan to make capabilities
| the default for 2.6.27?

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFITgKA+bHCR3gb8jsRAiQYAJ47VnlBq2GSvLQv40tymjybLhNAtQCgya8G YZQN/5w1uq+X2MYv1x4T4D4=
=NhwX
-----END PGP SIGNATURE-----

From be19a4716c97c5aaf4c9721eeccfab2d44897ce2 Mon Sep 17 00:00:00 2001
From: Andrew G. Morgan <[email protected]>
Date: Mon, 9 Jun 2008 21:22:18 -0700
Subject: [PATCH] Add (back) dummy support for KEEPCAPS.

See: http://bugzilla.kernel.org/show_bug.cgi?id=10748

---
 security/dummy.c |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

index f50c6c3..b891688 100644
@@ -27,6 +27,8 @@
 #include <linux/hugetlb.h>
 #include <linux/ptrace.h>
 #include <linux/file.h>
+#include <linux/prctl.h>
+#include <linux/securebits.h>
 
 static int dummy_ptrace (struct task_struct *parent, struct task_struct *child)
 {

@@ -607,7 +609,27 @@ static int dummy_task_kill (struct task_struct *p, struct siginfo *info,
 static int dummy_task_prctl (int option, unsigned long arg2, unsigned long arg3,
 			     unsigned long arg4, unsigned long arg5, long *rc_p)
 {
-	return 0;
+	switch (option) {
+	case PR_CAPBSET_READ:
+		*rc_p = (cap_valid(arg2) ? 1 : -EINVAL);
+		break;
+	case PR_GET_KEEPCAPS:
+		*rc_p = issecure(SECURE_KEEP_CAPS);
+		break;
+	case PR_SET_KEEPCAPS:
+		if (arg2 > 1)
+			*rc_p = -EINVAL;
+		else if (arg2)
+			current->securebits |= issecure_mask(SECURE_KEEP_CAPS);
+		else
+			current->securebits &=
+				~issecure_mask(SECURE_KEEP_CAPS);
+		break;
+	default:
+		return 0;
+	}
+
+	return 1;
 }
 
 static void dummy_task_reparent_to_init (struct task_struct *p)

1.5.3.7

mutter. Could we have a real changelog please?

Thanks, Andrew. Just one question inline. Nevertheless,

Dmitry, does this fix the problem for you?

(Not sure why I'm feeling queasy about this given that find . -name "*.c" -exec "grep" "-Hn" "issecure" "{}" \; returns only hits in security/commoncap.c...)

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

Serge E. Hallyn wrote:
+ else
+ current->securebits &=
+ ~issecure_mask(SECURE_KEEP_CAPS);

| In these last two conditions, don't you need to set *rc_p?

| Oh, or my kernel tree may be out of date, as I seem to recall a recent
| patch initializing error to 0 in sys_prctl(), so this wouldn't
| technically be a problem?  Still would seem correct...

Yes, update your kernel and you will find this defaults to zero. I thought about redundantly forcing it, but decided that it wasn't worth the pain of working around the 80-column challenged extra indenting. :-(

Cheers

Andrew
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFITx7E+bHCR3gb8jsRAulwAJ0U6TwsWMZRs6epQNNWs+f27UuUfgCdEGut 4x/mUDuIX6TvCVnip5sZWVk=
=1PdZ
-----END PGP SIGNATURE-----

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

Chris Wright wrote:

|> +	switch (option) {
|> +	case PR_CAPBSET_READ:
|> +		*rc_p = (cap_valid(arg2) ? 1 : -EINVAL);
|> +		break;
|
| Do we need this one?  It's new to 2.6.25, so I think we could not
| worry about emulating it here.

We're talking about 'fixing' 2.6.26 no? I'd rather not open up the possibility that I have to 'fix' it again because of dropping a feature of 2.6.25... (Forgive me if I sound like I'm climbing out of a septic tank here.)

Dmitry: please verify this change addresses your problem...

Cheers

Andrew
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFITx5i+bHCR3gb8jsRAi8dAJ9A8v+wbSiqikGaW8vIY2HcS0Hs5QCfU2gT 1z9FHzHTgSeBMqHak6+VCtU=
=jKT2
-----END PGP SIGNATURE-----

2008/6/11 Andrew G. Morgan <[email protected]>:

well, I fixed it on my side with another approach before sending a report for this "problem".

It was not immediatelly clear to me that the concept of "process capabilities" (as described in "man prctl" as follows "Set the state of the process's "keep capabilities" flag...") is not applicable to all possible configuration, meaning that each configuration have to support it in some way or another.

Moreover, according to commit's description the changes were supposed to be 'nop' for all configs besides when one freshly ntroduced is enabled.

Should it have been explicitly specified that thanks to this commit prctl(KEEPCAPS, ...) turns into "a good citizen" (i.e. stops lying to userspace about its support of capabilities -- if that's what is desired), it'd change a further flow of events :-)

ok, anyway, I don't have access to my machine at the moment and can't guarantee that I'll be able to do a test today.
You may try it with one of the configs I mentioened + a program doing prctl(KEEPCAPS, 1, ...).

btw., if I recall right capget() is still "lying" now. capset() did give an error but Ubuntu's dhclient is somewhat inconsistent as it checks for a return value of prctl() but not capset().

Best regards,
Dmitry Adamushko

We should get rid of it ASAP.

It is already lying, so this isn't too big a stretch. I'd expect it not to help since either capset will fail, or it would fail when it tried to use the cap after dropping uid.

thanks,
-chris