[PATCH 0/2] file capabilities: two bugfixes

[PATCH 0/2] file capabilities: two bugfixes

[Serge E. Hallyn]

In an lwn.net article, Jonathan Corbet made two very helpful comments
about the file capabilities patch currently being tested in -mm.  The
first is that capabilities are being honored on nosuid filesystems.
The other is that root can lose capabilities by executing files with
only some capabilities set.  The next two patches change these
behaviors.

thanks,
-serge

[PATCH 1/2] file capabilities: don't do file caps if MNT_NOSUID

[Serge E. Hallyn]

From: Serge E. Hallyn <serue@us.ibm.com>
Subject: [PATCH 1/2] file capabilities: don't do file caps if MNT_NOSUID

A file system mounted NOSUID is likely a removable filesystem.
Allowing file capabilities from such an fs is an easy attack
vector, so don't honor file capabilities for a NOSUID
filesystem.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 security/commoncap.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index ce91d9f..fde9695 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -23,6 +23,7 @@ #include <linux/netlink.h>
 #include <linux/ptrace.h>
 #include <linux/xattr.h>
 #include <linux/hugetlb.h>
+#include <linux/mount.h>
 
 int cap_netlink_send(struct sock *sk, struct sk_buff *skb)
 {
@@ -152,6 +153,9 @@ static int set_file_caps(struct linux_bi
 	struct inode *inode;
 	int err;
 
+	if (bprm->file->f_vfsmnt->mnt_flags & MNT_NOSUID)
+		return 0;
+
 	dentry = dget(bprm->file->f_dentry);
 	inode = dentry->d_inode;
 	if (!inode->i_op || !inode->i_op->getxattr) {
-- 
1.4.1

[PATCH 2/2] file capabilities: honor !SECURE_NOROOT

[Serge E. Hallyn]

From: Serge E. Hallyn <serue@us.ibm.com>
Subject: [PATCH 2/2] file capabilities: honor !SECURE_NOROOT

When the SECURE_NOROOT securebit is not set, allow root to
keep it's capabilities over exec, rather than compute the
capabilities based on file capabilities.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 security/commoncap.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index fde9695..be86acb 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -202,12 +202,16 @@ #endif
 
 int cap_bprm_set_security (struct linux_binprm *bprm)
 {
+	int ret;
+
 	/* Copied from fs/exec.c:prepare_binprm. */
 
 	cap_clear (bprm->cap_inheritable);
 	cap_clear (bprm->cap_permitted);
 	cap_clear (bprm->cap_effective);
 
+	ret = set_file_caps(bprm);
+
 	/*  To support inheritance of root-permissions and suid-root
 	 *  executables under compatibility mode, we raise all three
 	 *  capability sets for the file.
@@ -225,7 +229,7 @@ int cap_bprm_set_security (struct linux_
 			cap_set_full (bprm->cap_effective);
 	}
 
-	return set_file_caps(bprm);
+	return ret;
 }
 
 void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
-- 
1.4.1

Re: [PATCH 0/2] file capabilities: two bugfixes

[Casey Schaufler]

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

> ...
> The other is that root can lose capabilities by
> executing files with
> only some capabilities set.  The next two patches
> change these
> behaviors.

It was the intention of the POSIX group that
capabilities be independent of uid. I would
argue that the old bevavior was correct, that
a program marked to lose a capability ought
to even if the uid is 0.


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH 0/2] file capabilities: two bugfixes

[Serge E. Hallyn]

Quoting Casey Schaufler (casey@schaufler-ca.com):
> 
> --- "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> 
> > ...
> > The other is that root can lose capabilities by
> > executing files with
> > only some capabilities set.  The next two patches
> > change these
> > behaviors.
> 
> It was the intention of the POSIX group that
> capabilities be independent of uid. I would
> argue that the old bevavior was correct, that
> a program marked to lose a capability ought
> to even if the uid is 0.

Agreed, and if SECURE_NOROOT is set, that is what happens.
But by default SECURE_NOROOT is not set, in which case linux's
implementation of capabilities behaves differently for root.

Without this latest patch, with SECURE_NOROOT not set, what was
actually happening was that the kernel behaved as though
SECURE_NOROOT was not set so long as there was no
security.capability xattr, and always behaved as though
SECURE_NOROOT was set if there was an xattr.  That's inconsistent
and confusing behavior.

The worst part is that root can get around running the code
with limited caps by just copying the file and running the
copy.  So it adds no security benefit, and adds an
inconsistency/complication which could cause security risks.

-serge

Re: [PATCH 0/2] file capabilities: two bugfixes

[Casey Schaufler]

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

> Quoting Casey Schaufler (casey@schaufler-ca.com):
> > 
> > --- "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> > 
> > > ...
> > > The other is that root can lose capabilities by
> > > executing files with
> > > only some capabilities set.  The next two
> patches
> > > change these
> > > behaviors.
> > 
> > It was the intention of the POSIX group that
> > capabilities be independent of uid. I would
> > argue that the old bevavior was correct, that
> > a program marked to lose a capability ought
> > to even if the uid is 0.
> 
> Agreed, and if SECURE_NOROOT is set, that is what
> happens.
> But by default SECURE_NOROOT is not set, in which
> case linux's
> implementation of capabilities behaves differently
> for root.
> 
> Without this latest patch, with SECURE_NOROOT not
> set, what was
> actually happening was that the kernel behaved as
> though
> SECURE_NOROOT was not set so long as there was no
> security.capability xattr, and always behaved as
> though
> SECURE_NOROOT was set if there was an xattr.  That's
> inconsistent
> and confusing behavior.
> 
> The worst part is that root can get around running
> the code
> with limited caps by just copying the file and
> running the
> copy.  So it adds no security benefit, and adds an
> inconsistency/complication which could cause
> security risks.

OK, no worries then.


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH 0/2] file capabilities: two bugfixes

[Seth Arnold]

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

On Fri, Dec 08, 2006 at 01:36:57PM -0600, Serge E. Hallyn wrote:
> The other is that root can lose capabilities by executing files with
> only some capabilities set.  The next two patches change these
> behaviors.

I saw this in my code review and thought that this behaviour was
intentional. :) It seemed like a good idea to me..

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 0/2] file capabilities: two bugfixes

[Crispin Cowan]

Seth Arnold wrote:
> On Fri, Dec 08, 2006 at 01:36:57PM -0600, Serge E. Hallyn wrote:
>   
>> The other is that root can lose capabilities by executing files with
>> only some capabilities set.  The next two patches change these
>> behaviors.
>>     
> I saw this in my code review and thought that this behaviour was
> intentional. :) It seemed like a good idea to me..
>   
It really depends on which threat you are trying to defend against.

Serge is correct that it does not prevent root from copying the file,
messing with the attributes, and running it anyway. However, I don't see
file capabilities as being intended to try to confine root.

Rather, it seems like it is intended to make it easier to manage what
capabilities should usually be present when the program is run. E.g. the
file has a limited capability set so that root can run it and not have
to think about overtly dropping privs or su'ing to a non-privileged user
to be able to run it safely.

So I'm with Seth; it sounds like a feature, not a bug.

Caveat: I have no clue what the POSIX.1e committee intended. But since
none of the POSIX-alike implementations are compatible with each other
anyway, I don't really care :)

Crispin

-- 
Crispin Cowan, Ph.D.                      http://crispincowan.com/~crispin/
Director of Software Engineering, Novell  http://novell.com
     Hacking is exploiting the gap between "intent" and "implementation"