[PATCH 2/4] security: filesystem capabilities bugfix2

[PATCH 2/4] security: filesystem capabilities bugfix2

[Andrew G. Morgan]

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

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

Bugfix for strace, and CAP_SETPCAP, in the case that filesystem
capabilities are supported.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFIY1fr+bHCR3gb8jsRAph7AKDOlmeveIpQs1jhIs0TJxjCdMAS5ACgsml6
7UYR+FZpW2XdmG8PkiZzemU=
=+Ko+
-----END PGP SIGNATURE-----

[-- Attachment #2: 0002-Blunt-CAP_SETPCAP-on-strace-with-filesystem-capabili.patch --]
[-- Type: text/plain, Size: 1770 bytes --]

From f4419c78fff77c4fa3cdfa6b0a78edae92ddf467 Mon Sep 17 00:00:00 2001
From: Andrew G. Morgan <morgan@kernel.org>
Date: Wed, 25 Jun 2008 23:24:10 -0700
Subject: [PATCH] Blunt CAP_SETPCAP on strace with filesystem capability support

The filesystem capability support meaning for CAP_SETPCAP is less
powerful than the non-filesystem capability support. As such, when
filesystem capabilities are configured, we should not permit
CAP_SETPCAP to 'enhance' the current process through strace
manipulation of a child process.

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

diff --git a/security/commoncap.c b/security/commoncap.c
index 5edabc7..a9ea921 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -103,10 +103,16 @@ static inline int cap_inh_is_capped(void)
 	return (cap_capable(current, CAP_SETPCAP) != 0);
 }
 
+static inline int cap_limit_straced_target(void) { return 1; }
+
 #else /* ie., ndef CONFIG_SECURITY_FILE_CAPABILITIES */
 
 static inline int cap_block_setpcap(struct task_struct *t) { return 0; }
 static inline int cap_inh_is_capped(void) { return 1; }
+static inline int cap_limit_straced_target(void)
+{
+	return !capable(CAP_SETPCAP);
+}
 
 #endif /* def CONFIG_SECURITY_FILE_CAPABILITIES */
 
@@ -342,9 +348,10 @@ void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
 				bprm->e_uid = current->uid;
 				bprm->e_gid = current->gid;
 			}
-			if (!capable (CAP_SETPCAP)) {
-				new_permitted = cap_intersect (new_permitted,
-							current->cap_permitted);
+			if (cap_limit_straced_target()) {
+				new_permitted =
+					cap_intersect(new_permitted,
+						      current->cap_permitted);
 			}
 		}
 	}
-- 
1.5.3.7

Re: [PATCH 2/4] security: filesystem capabilities bugfix2

[Serge E. Hallyn]

Quoting Andrew G. Morgan (morgan@kernel.org):
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Bugfix for strace, and CAP_SETPCAP, in the case that filesystem
> capabilities are supported.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.2.6 (GNU/Linux)
>
> iD8DBQFIY1fr+bHCR3gb8jsRAph7AKDOlmeveIpQs1jhIs0TJxjCdMAS5ACgsml6
> 7UYR+FZpW2XdmG8PkiZzemU=
> =+Ko+
> -----END PGP SIGNATURE-----

> From f4419c78fff77c4fa3cdfa6b0a78edae92ddf467 Mon Sep 17 00:00:00 2001
> From: Andrew G. Morgan <morgan@kernel.org>
> Date: Wed, 25 Jun 2008 23:24:10 -0700
> Subject: [PATCH] Blunt CAP_SETPCAP on strace with filesystem capability support
> 
> The filesystem capability support meaning for CAP_SETPCAP is less
> powerful than the non-filesystem capability support. As such, when
> filesystem capabilities are configured, we should not permit
> CAP_SETPCAP to 'enhance' the current process through strace
> manipulation of a child process.
> 
> Signed-off-by: Andrew G. Morgan <morgan@kernel.org>

If I understand this right, then LSM_UNSAFE_PTRACE_CAP will only be set
if the tracer didn't have CAP_SYS_PTRACE.  So this seems sane to me.

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

> ---
>  security/commoncap.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 5edabc7..a9ea921 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -103,10 +103,16 @@ static inline int cap_inh_is_capped(void)
>  	return (cap_capable(current, CAP_SETPCAP) != 0);
>  }
>  
> +static inline int cap_limit_straced_target(void) { return 1; }
> +
>  #else /* ie., ndef CONFIG_SECURITY_FILE_CAPABILITIES */
>  
>  static inline int cap_block_setpcap(struct task_struct *t) { return 0; }
>  static inline int cap_inh_is_capped(void) { return 1; }
> +static inline int cap_limit_straced_target(void)
> +{
> +	return !capable(CAP_SETPCAP);
> +}
>  
>  #endif /* def CONFIG_SECURITY_FILE_CAPABILITIES */
>  
> @@ -342,9 +348,10 @@ void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
>  				bprm->e_uid = current->uid;
>  				bprm->e_gid = current->gid;
>  			}
> -			if (!capable (CAP_SETPCAP)) {
> -				new_permitted = cap_intersect (new_permitted,
> -							current->cap_permitted);
> +			if (cap_limit_straced_target()) {
> +				new_permitted =
> +					cap_intersect(new_permitted,
> +						      current->cap_permitted);
>  			}
>  		}
>  	}
> -- 
> 1.5.3.7
> 

Re: [PATCH 2/4] security: filesystem capabilities bugfix2

[David Howells]

Serge E. Hallyn <serue@us.ibm.com> wrote:

> If I understand this right, then LSM_UNSAFE_PTRACE_CAP will only be set
> if the tracer didn't have CAP_SYS_PTRACE.  So this seems sane to me.

Erm...  Firstly:

	int ptrace_attach(struct task_struct *task)
	{
	...
		if (capable(CAP_SYS_PTRACE))
			task->ptrace |= PT_PTRACE_CAP;
	...
	}

Then:

	static int unsafe_exec(struct task_struct *p)
	{
		int unsafe = 0;
		if (p->ptrace & PT_PTRACED) {
			if (p->ptrace & PT_PTRACE_CAP)
				unsafe |= LSM_UNSAFE_PTRACE_CAP;
			else
				unsafe |= LSM_UNSAFE_PTRACE;
		}
		if (atomic_read(&p->fs->count) > 1 ||
		    atomic_read(&p->files->count) > 1 ||
		    atomic_read(&p->sighand->count) > 1)
			unsafe |= LSM_UNSAFE_SHARE;

		return unsafe;
	}

So LSM_UNSAFE_PTRACE_CAP will only be set if the tracer _does_ have
CAP_SYS_PTRACE.  That will be irrelevant, however, if any of fs, files or
sighand are shared.

And finally:

	void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
	{
	...
		if (bprm->e_uid != current->uid ||
		    bprm->e_gid != current->gid ||
		    !cap_issubset (new_permitted, current->cap_permitted)) {
	...
			if (unsafe & ~LSM_UNSAFE_PTRACE_CAP) {
				if (!capable(CAP_SETUID)) {
					bprm->e_uid = current->uid;
					bprm->e_gid = current->gid;
				}
				if (!capable (CAP_SETPCAP)) {
					new_permitted = cap_intersect(
						new_permitted,
						current->cap_permitted);
				}
			}
	...
	}

So if it's a 'set-privilege' binary, then if the tracer _doesn't_ have
CAP_SYS_PTRACE, we look at downgrading the privileges of the process.

Without Andrew's patch, we only downgrade the capabilities if we don't have
CAP_SETPCAP (and aren't sharing inheritables).

With Andrew's patch, capabilities are downgraded regardless of whether we have
CAP_SETPCAP or not.  I guess that means that if you're tracing a binary whose
filecaps say that it wants CAP_SETPCAP, then it retains CAP_SETPCAP.

I wonder if the tracing task should be examined here, and any capability the
tracer isn't permitted should be denied the process doing the exec.

Anyway, in my commoncap.c prettification patch, I've dressed the limiter
function up as follows:

	/*
	 * Determine whether a exec'ing process's new permitted capabilities
	 * should be limited to just what it already has.
	 *
	 * This prevents processes that are being ptraced from gaining access
	 * to CAP_SETPCAP, unless the process they're tracing already has it,
	 * and the binary they're executing has filecaps that elevate it.
	 *
	 *  Returns 1 if they should be limited, 0 if they are not.
	 */
	static inline int cap_limit_ptraced_target(void)
	{
	#ifndef CONFIG_SECURITY_FILE_CAPABILITIES
		if (capable(CAP_SETPCAP))
			return 0;
	#endif
		return 1;
	}

David

Re: [PATCH 2/4] security: filesystem capabilities bugfix2

[Serge E. Hallyn]

Quoting David Howells (dhowells@redhat.com):
> Serge E. Hallyn <serue@us.ibm.com> wrote:
> 
> > If I understand this right, then LSM_UNSAFE_PTRACE_CAP will only be set
> > if the tracer didn't have CAP_SYS_PTRACE.  So this seems sane to me.
> 
> Erm...  Firstly:

Yeah, inverse of what I said...

> 	int ptrace_attach(struct task_struct *task)
> 	{
> 	...
> 		if (capable(CAP_SYS_PTRACE))
> 			task->ptrace |= PT_PTRACE_CAP;
> 	...
> 	}
> 
> Then:
> 
> 	static int unsafe_exec(struct task_struct *p)
> 	{
> 		int unsafe = 0;
> 		if (p->ptrace & PT_PTRACED) {
> 			if (p->ptrace & PT_PTRACE_CAP)
> 				unsafe |= LSM_UNSAFE_PTRACE_CAP;
> 			else
> 				unsafe |= LSM_UNSAFE_PTRACE;
> 		}
> 		if (atomic_read(&p->fs->count) > 1 ||
> 		    atomic_read(&p->files->count) > 1 ||
> 		    atomic_read(&p->sighand->count) > 1)
> 			unsafe |= LSM_UNSAFE_SHARE;
> 
> 		return unsafe;
> 	}
> 
> So LSM_UNSAFE_PTRACE_CAP will only be set if the tracer _does_ have
> CAP_SYS_PTRACE.  That will be irrelevant, however, if any of fs, files or
> sighand are shared.
> 
> And finally:
> 
> 	void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
> 	{
> 	...
> 		if (bprm->e_uid != current->uid ||
> 		    bprm->e_gid != current->gid ||
> 		    !cap_issubset (new_permitted, current->cap_permitted)) {
> 	...
> 			if (unsafe & ~LSM_UNSAFE_PTRACE_CAP) {
> 				if (!capable(CAP_SETUID)) {
> 					bprm->e_uid = current->uid;
> 					bprm->e_gid = current->gid;
> 				}
> 				if (!capable (CAP_SETPCAP)) {
> 					new_permitted = cap_intersect(
> 						new_permitted,
> 						current->cap_permitted);
> 				}
> 			}
> 	...
> 	}
> 
> So if it's a 'set-privilege' binary, then if the tracer _doesn't_ have
> CAP_SYS_PTRACE, we look at downgrading the privileges of the process.
> 
> Without Andrew's patch, we only downgrade the capabilities if we don't have
> CAP_SETPCAP (and aren't sharing inheritables).
> 
> With Andrew's patch, capabilities are downgraded regardless of whether we have
> CAP_SETPCAP or not.  I guess that means that if you're tracing a binary whose
> filecaps say that it wants CAP_SETPCAP, then it retains CAP_SETPCAP.

I don't understand where that last sentence comes from.  Why would it
retain CAP_SETPCAP?

> I wonder if the tracing task should be examined here, and any capability the
> tracer isn't permitted should be denied the process doing the exec.

That sounds reasonable on its own, but it opens up a dangerous ability
for the partially-privileged tracer to manipulate the capability set for
the traced task.

Note that (as of recently) we do not allow the execution of a file with
partial privileges in its pE', precisely because it is dangerous to
allow pick-and-choosing of capabilities in a capability-unaware binary.

So frankly I wonder whether the existing downgrade is really safe...

-serge

> Anyway, in my commoncap.c prettification patch, I've dressed the limiter
> function up as follows:
> 
> 	/*
> 	 * Determine whether a exec'ing process's new permitted capabilities
> 	 * should be limited to just what it already has.
> 	 *
> 	 * This prevents processes that are being ptraced from gaining access
> 	 * to CAP_SETPCAP, unless the process they're tracing already has it,
> 	 * and the binary they're executing has filecaps that elevate it.
> 	 *
> 	 *  Returns 1 if they should be limited, 0 if they are not.
> 	 */
> 	static inline int cap_limit_ptraced_target(void)
> 	{
> 	#ifndef CONFIG_SECURITY_FILE_CAPABILITIES
> 		if (capable(CAP_SETPCAP))
> 			return 0;
> 	#endif
> 		return 1;
> 	}
> 
> David

Re: [PATCH 2/4] security: filesystem capabilities bugfix2

[David Howells]

Serge E. Hallyn <serue@us.ibm.com> wrote:

> > With Andrew's patch, capabilities are downgraded regardless of whether we
> > have CAP_SETPCAP or not.  I guess that means that if you're tracing a
> > binary whose filecaps say that it wants CAP_SETPCAP, then it retains
> > CAP_SETPCAP.
> 
> I don't understand where that last sentence comes from.  Why would it
> retain CAP_SETPCAP?

It seems I missed a bit out.  It should've read:

	I guess that means that if you're tracing a binary that has
	CAP_SETPCAP already, and whose filecaps say that it wants CAP_SETPCAP,
	then it retains CAP_SETPCAP.

If the debugger has CAP_SYS_PTRACE, then it can attach to a binary that has
CAP_SETPCAP according to cap_ptrace(), even if the debugger doesn't.

> > I wonder if the tracing task should be examined here, and any capability the
> > tracer isn't permitted should be denied the process doing the exec.
> 
> That sounds reasonable on its own, but it opens up a dangerous ability
> for the partially-privileged tracer to manipulate the capability set for
> the traced task.

Does it, though?  It would only reduce the capabilities of the inferior
process; it wouldn't allow the inferior process or the debugger to get
additional capabilities, apart from what's available under CAP_SETPCAP.

David

Re: [PATCH 2/4] security: filesystem capabilities bugfix2

[Serge E. Hallyn]

Quoting David Howells (dhowells@redhat.com):
> Serge E. Hallyn <serue@us.ibm.com> wrote:
> 
> > > With Andrew's patch, capabilities are downgraded regardless of whether we
> > > have CAP_SETPCAP or not.  I guess that means that if you're tracing a
> > > binary whose filecaps say that it wants CAP_SETPCAP, then it retains
> > > CAP_SETPCAP.
> > 
> > I don't understand where that last sentence comes from.  Why would it
> > retain CAP_SETPCAP?
> 
> It seems I missed a bit out.  It should've read:
> 
> 	I guess that means that if you're tracing a binary that has
> 	CAP_SETPCAP already, and whose filecaps say that it wants CAP_SETPCAP,
> 	then it retains CAP_SETPCAP.
> 
> If the debugger has CAP_SYS_PTRACE, then it can attach to a binary that has
> CAP_SETPCAP according to cap_ptrace(), even if the debugger doesn't.

Ah.  Yes.  I think that's the desirable behavior in all proposals.

> > > I wonder if the tracing task should be examined here, and any capability the
> > > tracer isn't permitted should be denied the process doing the exec.
> > 
> > That sounds reasonable on its own, but it opens up a dangerous ability
> > for the partially-privileged tracer to manipulate the capability set for
> > the traced task.
> 
> Does it, though?  It would only reduce the capabilities of the inferior
> process; it wouldn't allow the inferior process or the debugger to get
> additional capabilities, apart from what's available under CAP_SETPCAP.

And the uids won't change unless capable(CAP_SETUID)...  so I think
you're right, it does sound safe.

thanks,
-serge

Re: [PATCH 2/4] security: filesystem capabilities bugfix2

[David Howells]

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

> +static inline int cap_limit_straced_target(void) { return 1; }

Calling it cap_limit_ptraced_target() would probably be better.

David

Re: [PATCH 2/4] security: filesystem capabilities bugfix2

[Andrew G. Morgan]

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

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

David Howells wrote:
| Andrew G. Morgan <morgan@kernel.org> wrote:
|
|> +static inline int cap_limit_straced_target(void) { return 1; }
|
| Calling it cap_limit_ptraced_target() would probably be better.

You are right. Fix-up patch attached.

Cheers

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

iD8DBQFIaHH8+bHCR3gb8jsRAgKHAJ4uu7Xd6PZObvIwzvJPSA6lGBpY1QCfUb20
bw1vY5cX8qW7UBMel6qgi5I=
=iacs
-----END PGP SIGNATURE-----

[-- Attachment #2: 0001-ptraced-straced.patch --]
[-- Type: text/plain, Size: 1497 bytes --]

From 0b3f48f509bf934f9fdc83a6e851bfd69c779952 Mon Sep 17 00:00:00 2001
From: Andrew G. Morgan <morgan@kernel.org>
Date: Sun, 29 Jun 2008 22:37:40 -0700
Subject: [PATCH] ptraced -> straced

Within the kernel, ptraced is a more appropriate name.

Reported-by: David Howells <dhowells@redhat.com>
Signed-off-by: Andrew G. Morgan <morgan@kernel.org>
---
 security/commoncap.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index a9ea921..33d3433 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -103,13 +103,13 @@ static inline int cap_inh_is_capped(void)
 	return (cap_capable(current, CAP_SETPCAP) != 0);
 }
 
-static inline int cap_limit_straced_target(void) { return 1; }
+static inline int cap_limit_ptraced_target(void) { return 1; }
 
 #else /* ie., ndef CONFIG_SECURITY_FILE_CAPABILITIES */
 
 static inline int cap_block_setpcap(struct task_struct *t) { return 0; }
 static inline int cap_inh_is_capped(void) { return 1; }
-static inline int cap_limit_straced_target(void)
+static inline int cap_limit_ptraced_target(void)
 {
 	return !capable(CAP_SETPCAP);
 }
@@ -348,7 +348,7 @@ void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
 				bprm->e_uid = current->uid;
 				bprm->e_gid = current->gid;
 			}
-			if (cap_limit_straced_target()) {
+			if (cap_limit_ptraced_target()) {
 				new_permitted =
 					cap_intersect(new_permitted,
 						      current->cap_permitted);
-- 
1.5.6.1

Re: [PATCH 2/4] security: filesystem capabilities bugfix2

[David Howells]

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

> Within the kernel, ptraced is a more appropriate name.
> 
> Reported-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Andrew G. Morgan <morgan@kernel.org>

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