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

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;

---

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

[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

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

[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

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

[Andrew G. Morgan]

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

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

[-- Attachment #2: dummy-prctl.patch --]
[-- Type: text/plain, Size: 1559 bytes --]

From be19a4716c97c5aaf4c9721eeccfab2d44897ce2 Mon Sep 17 00:00:00 2001
From: Andrew G. Morgan <morgan@kernel.org>
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

Signed-off-by: Andrew G. Morgan <morgan@kernel.org>
---
 security/dummy.c |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/security/dummy.c b/security/dummy.c
index f50c6c3..b891688 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -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

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

[Andrew Morton]

On Mon, 09 Jun 2008 21:26:40 -0700 "Andrew G. Morgan" <morgan@kernel.org> wrote:

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

mutter.  Could we have a real changelog please?

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

[Serge E. Hallyn]

Quoting Andrew G. Morgan (morgan@kernel.org):
> -----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 <morgan@kernel.org>
> 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
> 
> Signed-off-by: Andrew G. Morgan <morgan@kernel.org>

Thanks, Andrew.  Just one question inline.  Nevertheless,

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

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

> ---
>  security/dummy.c |   24 +++++++++++++++++++++++-
>  1 files changed, 23 insertions(+), 1 deletions(-)
> 
> diff --git a/security/dummy.c b/security/dummy.c
> index f50c6c3..b891688 100644
> --- a/security/dummy.c
> +++ b/security/dummy.c
> @@ -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);

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

> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return 1;
>  }
>  
>  static void dummy_task_reparent_to_init (struct task_struct *p)
> -- 
> 1.5.3.7
> 

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

[Andrew G. Morgan]

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

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

[Chris Wright]

* Andrew G. Morgan (morgan@kernel.org) wrote:
> --- a/security/dummy.c
> +++ b/security/dummy.c
> @@ -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;

Do we need this one?  It's new to 2.6.25, so I think we could not
worry about emulating it here.

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

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

[Andrew G. Morgan]

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

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

[Dmitry Adamushko]

2008/6/11 Andrew G. Morgan <morgan@kernel.org>:
> -----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...

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, ...).

>From what I see, yes, it should address this issue.

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


>
> Cheers
>
> Andrew


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

[ 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