[ linus-git ] prctl(PR_SET_KEEPCAPS, ...) is broken for some configs, e.g. CONFIG_SECURITY_SELINUX

[ linus-git ] prctl(PR_SET_KEEPCAPS, ...) is broken for some configs, e.g. CONFIG_SECURITY_SELINUX

[Dmitry Adamushko]

Hi,


the commit 3898b1b4ebff8dcfbcf1807e0661585e06c9a91c has broken (always
-EINVAL as a return value)

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


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.


p.s. perhaps, some would argue that such behavior might have its own
advantages. e.g. 'dhclient' on Ubuntu (for sure on 7.04) refuses to
work and, as a result, a crowd of Ubuntu followers turn their backs on
the virtual world and finally spend more time with their families. It
might be also good for the noble cause of fighting global warming...
heh, provided people don't escape into another virtual world by means
of shiny plasma-TVs :-)


-- 
Best regards,
Dmitry Adamushko

Re: [ linus-git ] prctl(PR_SET_KEEPCAPS, ...) is broken for some configs, e.g. CONFIG_SECURITY_SELINUX

[Chris Wright]

* Serge E. Hallyn (serue@us.ibm.com) wrote:
> 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.

We should get rid of it ASAP.

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

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

Re: [ linus-git ] prctl(PR_SET_KEEPCAPS, ...) is broken for some configs, e.g. CONFIG_SECURITY_SELINUX

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@linux-foundation.org):
> On Sun, 08 Jun 2008 15:34:44 -0700 Andrew Morgan <morgan@kernel.org> wrote:
> 
> > | On Sun, 08 Jun 2008 08:10:26 -0700 Andrew Morgan <morgan@kernel.org>
> > wrote:
> > |
> > |> Nacked-by: Andrew G. Morgan <morgan@kernel.org>
> > |>
> > |> 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)?
> >
> > 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.
> 
> (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

Re: [ linus-git ] prctl(PR_SET_KEEPCAPS, ...) is broken for some configs, e.g. CONFIG_SECURITY_SELINUX

[Andrew Morton]

On Sun, 08 Jun 2008 15:34:44 -0700 Andrew Morgan <morgan@kernel.org> wrote:

> | On Sun, 08 Jun 2008 08:10:26 -0700 Andrew Morgan <morgan@kernel.org>
> wrote:
> |
> |> Nacked-by: Andrew G. Morgan <morgan@kernel.org>
> |>
> |> 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)?
>
> 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.

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

Re: [ linus-git ] prctl(PR_SET_KEEPCAPS, ...) is broken for some configs, e.g. CONFIG_SECURITY_SELINUX

[Andrew Morgan]

-----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 <morgan@kernel.org>
wrote:
|
|> Nacked-by: Andrew G. Morgan <morgan@kernel.org>
|>
|> 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-----

Re: [ linus-git ] prctl(PR_SET_KEEPCAPS, ...) is broken for some configs, e.g. CONFIG_SECURITY_SELINUX

[Andrew Morton]

On Sun, 08 Jun 2008 08:10:26 -0700 Andrew Morgan <morgan@kernel.org> wrote:

> Nacked-by: Andrew G. Morgan <morgan@kernel.org>
> 
> 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)?

Re: [ linus-git ] prctl(PR_SET_KEEPCAPS, ...) is broken for some configs, e.g. CONFIG_SECURITY_SELINUX

[Andrew Morgan]

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

Nacked-by: Andrew G. Morgan <morgan@kernel.org>

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 <dmitry.adamushko@gmail.com>
| 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 <dmitry.adamushko@gmail.com>
|
| ----
|
| 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-----

Re: [ linus-git ] prctl(PR_SET_KEEPCAPS, ...) is broken for some configs, e.g. CONFIG_SECURITY_SELINUX

[Dmitry Adamushko]

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


---
From: Dmitry Adamushko <dmitry.adamushko@gmail.com>
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 <dmitry.adamushko@gmail.com>

----

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;

---