[patch] security: fix dummy xattr functions

[patch] security: fix dummy xattr functions

[Miklos Szeredi]

Hi James,

If this (untested) patch looks OK, could you please apply it to your
tree?

Thanks,
Miklos

----
From: Miklos Szeredi <mszeredi@suse.cz>

Replace open coded xattr checks with cap_inode_xxx() function calls in
dummy_inode_setxattr() and dummy_inode_removexattr().  The old ones
were out of sync with the cap_inode_xxx() implementation, which could
even be a security problem.

Noticed by John Johansen.

CC: John Johansen <jjohansen@suse.de>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 security/dummy.c |   12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

Index: linux-2.6/security/dummy.c
===================================================================
--- linux-2.6.orig/security/dummy.c	2008-07-01 21:44:03.000000000 +0200
+++ linux-2.6/security/dummy.c	2008-07-01 21:51:08.000000000 +0200
@@ -370,11 +370,7 @@ static void dummy_inode_delete (struct i
 static int dummy_inode_setxattr (struct dentry *dentry, const char *name,
 				 const void *value, size_t size, int flags)
 {
-	if (!strncmp(name, XATTR_SECURITY_PREFIX,
-		     sizeof(XATTR_SECURITY_PREFIX) - 1) &&
-	    !capable(CAP_SYS_ADMIN))
-		return -EPERM;
-	return 0;
+	return cap_inode_setxattr(dentry, name, value, size, flags);
 }
 
 static void dummy_inode_post_setxattr (struct dentry *dentry, const char *name,
@@ -395,11 +391,7 @@ static int dummy_inode_listxattr (struct
 
 static int dummy_inode_removexattr (struct dentry *dentry, const char *name)
 {
-	if (!strncmp(name, XATTR_SECURITY_PREFIX,
-		     sizeof(XATTR_SECURITY_PREFIX) - 1) &&
-	    !capable(CAP_SYS_ADMIN))
-		return -EPERM;
-	return 0;
+	return cap_inode_removexattr(dentry, name);
 }
 
 static int dummy_inode_need_killpriv(struct dentry *dentry)

Re: [patch] security: fix dummy xattr functions

[James Morris]

On Tue, 1 Jul 2008, Miklos Szeredi wrote:

> Hi James,
> 
> If this (untested) patch looks OK, could you please apply it to your
> tree?

As I understand it, filesystem capabilities is only enabled when either 
LSM is disabled, or the LSM capabilities module is built.

In both cases, security/commoncap.o is built.  With LSM disabled, the 
correct cap_inode_xxx functions will be linked.  With LSM+capabilities, 
either the capability LSM will be loaded, or another LSM will need to 
deliberately stack it.

So, I think the existing code is correct.

> 
> Replace open coded xattr checks with cap_inode_xxx() function calls in
> dummy_inode_setxattr() and dummy_inode_removexattr().  The old ones
> were out of sync with the cap_inode_xxx() implementation, which could
> even be a security problem.
> 
> Noticed by John Johansen.
> 
> CC: John Johansen <jjohansen@suse.de>
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> ---
>  security/dummy.c |   12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> Index: linux-2.6/security/dummy.c
> ===================================================================
> --- linux-2.6.orig/security/dummy.c	2008-07-01 21:44:03.000000000 +0200
> +++ linux-2.6/security/dummy.c	2008-07-01 21:51:08.000000000 +0200
> @@ -370,11 +370,7 @@ static void dummy_inode_delete (struct i
>  static int dummy_inode_setxattr (struct dentry *dentry, const char *name,
>  				 const void *value, size_t size, int flags)
>  {
> -	if (!strncmp(name, XATTR_SECURITY_PREFIX,
> -		     sizeof(XATTR_SECURITY_PREFIX) - 1) &&
> -	    !capable(CAP_SYS_ADMIN))
> -		return -EPERM;
> -	return 0;
> +	return cap_inode_setxattr(dentry, name, value, size, flags);
>  }
>  
>  static void dummy_inode_post_setxattr (struct dentry *dentry, const char *name,
> @@ -395,11 +391,7 @@ static int dummy_inode_listxattr (struct
>  
>  static int dummy_inode_removexattr (struct dentry *dentry, const char *name)
>  {
> -	if (!strncmp(name, XATTR_SECURITY_PREFIX,
> -		     sizeof(XATTR_SECURITY_PREFIX) - 1) &&
> -	    !capable(CAP_SYS_ADMIN))
> -		return -EPERM;
> -	return 0;
> +	return cap_inode_removexattr(dentry, name);
>  }
>  
>  static int dummy_inode_need_killpriv(struct dentry *dentry)
> 

-- 
James Morris
<jmorris@namei.org>

Re: [patch] security: fix dummy xattr functions

[Miklos Szeredi]

On Wed, 2 Jul 2008, James Morris wrote:
> On Tue, 1 Jul 2008, Miklos Szeredi wrote:
> 
> > Hi James,
> > 
> > If this (untested) patch looks OK, could you please apply it to your
> > tree?
> 
> As I understand it, filesystem capabilities is only enabled when either 
> LSM is disabled, or the LSM capabilities module is built.
> 
> In both cases, security/commoncap.o is built.  With LSM disabled, the 
> correct cap_inode_xxx functions will be linked.  With LSM+capabilities, 
> either the capability LSM will be loaded, or another LSM will need to 
> deliberately stack it.
> 
> So, I think the existing code is correct.

So where do the dummy_ functions figure into this?  As I understand,
they are called whenever LSM is disabled, but the LSM doesn't define a
particular hook, so there's a default implementation.  Is that correct?

If so, then in theory it is still theoretically possible that with
LSM+capabilities, the LSM doesn't explicitly stack inode_setxattr and
inode_removexattr, and so the dummy implementation should do that
instead.  What am I missing?

Thanks,
Miklos

> 
> > 
> > Replace open coded xattr checks with cap_inode_xxx() function calls in
> > dummy_inode_setxattr() and dummy_inode_removexattr().  The old ones
> > were out of sync with the cap_inode_xxx() implementation, which could
> > even be a security problem.
> > 
> > Noticed by John Johansen.
> > 
> > CC: John Johansen <jjohansen@suse.de>
> > Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> > ---
> >  security/dummy.c |   12 ++----------
> >  1 file changed, 2 insertions(+), 10 deletions(-)
> > 
> > Index: linux-2.6/security/dummy.c
> > ===================================================================
> > --- linux-2.6.orig/security/dummy.c	2008-07-01 21:44:03.000000000 +0200
> > +++ linux-2.6/security/dummy.c	2008-07-01 21:51:08.000000000 +0200
> > @@ -370,11 +370,7 @@ static void dummy_inode_delete (struct i
> >  static int dummy_inode_setxattr (struct dentry *dentry, const char *name,
> >  				 const void *value, size_t size, int flags)
> >  {
> > -	if (!strncmp(name, XATTR_SECURITY_PREFIX,
> > -		     sizeof(XATTR_SECURITY_PREFIX) - 1) &&
> > -	    !capable(CAP_SYS_ADMIN))
> > -		return -EPERM;
> > -	return 0;
> > +	return cap_inode_setxattr(dentry, name, value, size, flags);
> >  }
> >  
> >  static void dummy_inode_post_setxattr (struct dentry *dentry, const char *name,
> > @@ -395,11 +391,7 @@ static int dummy_inode_listxattr (struct
> >  
> >  static int dummy_inode_removexattr (struct dentry *dentry, const char *name)
> >  {
> > -	if (!strncmp(name, XATTR_SECURITY_PREFIX,
> > -		     sizeof(XATTR_SECURITY_PREFIX) - 1) &&
> > -	    !capable(CAP_SYS_ADMIN))
> > -		return -EPERM;
> > -	return 0;
> > +	return cap_inode_removexattr(dentry, name);
> >  }
> >  
> >  static int dummy_inode_need_killpriv(struct dentry *dentry)
> > 
> 
> -- 
> James Morris
> <jmorris@namei.org>
> 

Re: [patch] security: fix dummy xattr functions

[James Morris]

On Wed, 2 Jul 2008, Miklos Szeredi wrote:

> So where do the dummy_ functions figure into this?  As I understand,
> they are called whenever LSM is disabled, but the LSM doesn't define a
> particular hook, so there's a default implementation.  Is that correct?

If LSM is disabled, nothing is called (the security hooks are optimized 
away).  It's for when LSM is enabled, but there is either no LSM module 
selected, or as fallbacks for hooks which are not implemented by an LSM 
module.

> If so, then in theory it is still theoretically possible that with
> LSM+capabilities, the LSM doesn't explicitly stack inode_setxattr and
> inode_removexattr, and so the dummy implementation should do that
> instead.  What am I missing?

The LSM is responsible for performing this stacking (or not), depending on 
which particular security models are desired.  It may, for example, not 
want filesystem capabilities.

I guess it might be safer to force the LSM to override fs capabilities if 
it doesn't want them, but I'd like to see what others think.


- James
-- 
James Morris
<jmorris@namei.org>

Re: [patch] security: fix dummy xattr functions

[Miklos Szeredi]

On Wed, 2 Jul 2008, James Morris wrote:
> On Wed, 2 Jul 2008, Miklos Szeredi wrote:
> 
> > So where do the dummy_ functions figure into this?  As I understand,
> > they are called whenever LSM is disabled, but the LSM doesn't define a
> > particular hook, so there's a default implementation.  Is that correct?
> 
> If LSM is disabled, nothing is called (the security hooks are optimized 
> away).

Right.  I meant to say "enabled", instead of "disabled" above :)

>  It's for when LSM is enabled, but there is either no LSM module 
> selected, or as fallbacks for hooks which are not implemented by an LSM 
> module.

Yes, we were thinking of the fallback case.  When falling back to the
default, that should be equivalent to the "no LSM" case, no?

Currently it's not.

> > If so, then in theory it is still theoretically possible that with
> > LSM+capabilities, the LSM doesn't explicitly stack inode_setxattr and
> > inode_removexattr, and so the dummy implementation should do that
> > instead.  What am I missing?
> 
> The LSM is responsible for performing this stacking (or not), depending on 
> which particular security models are desired.  It may, for example, not 
> want filesystem capabilities.
> 
> I guess it might be safer to force the LSM to override fs capabilities if 
> it doesn't want them, but I'd like to see what others think.

No, this patch is _not_ forcing anything is LSM defines its own
inode_{set,remove}xattr methods.  I agree, that should be decided by
the LSM.

The patch is just fixing the fallback dummy functions to be in sync
with the the disabled LSM case.

Miklos

Re: [patch] security: fix dummy xattr functions

[Stephen Smalley]

On Wed, 2008-07-02 at 19:16 +1000, James Morris wrote:
> On Wed, 2 Jul 2008, Miklos Szeredi wrote:
> 
> > So where do the dummy_ functions figure into this?  As I understand,
> > they are called whenever LSM is disabled, but the LSM doesn't define a
> > particular hook, so there's a default implementation.  Is that correct?
> 
> If LSM is disabled, nothing is called (the security hooks are optimized 
> away).  It's for when LSM is enabled, but there is either no LSM module 
> selected, or as fallbacks for hooks which are not implemented by an LSM 
> module.
> 
> > If so, then in theory it is still theoretically possible that with
> > LSM+capabilities, the LSM doesn't explicitly stack inode_setxattr and
> > inode_removexattr, and so the dummy implementation should do that
> > instead.  What am I missing?
> 
> The LSM is responsible for performing this stacking (or not), depending on 
> which particular security models are desired.  It may, for example, not 
> want filesystem capabilities.
> 
> I guess it might be safer to force the LSM to override fs capabilities if 
> it doesn't want them, but I'd like to see what others think.

As discussed elsewhere, the dummy module just needs to die, and
capability needs to become the default module.  Then we no longer need
to deal with keeping them in sync or figuring out to emulate/fake
capabilities for userspace from dummy (since userspace expects them to
exist in Linux ever since they were first introduced long ago).

BTW, SELinux does not invoke the cap_ or dummy_
inode_setxattr/removexattr hooks as that would cause CAP_SYS_ADMIN to be
checked on the security.selinux attribute.  But
selinux_inode_setotherxattr() has the right logic.

-- 
Stephen Smalley
National Security Agency

Re: [patch] security: fix dummy xattr functions

[Miklos Szeredi]

On Wed, 02 Jul 2008, Stephen Smalley wrote:
> As discussed elsewhere, the dummy module just needs to die, and
> capability needs to become the default module.

How hard would that be to do in your opinion?

It would help some if we didn't have to update the dummy module with
the changes required by apparmor.

Thanks,
Miklos

Re: [patch] security: fix dummy xattr functions

[Stephen Smalley]

On Wed, 2008-07-02 at 13:54 +0200, Miklos Szeredi wrote:
> On Wed, 02 Jul 2008, Stephen Smalley wrote:
> > As discussed elsewhere, the dummy module just needs to die, and
> > capability needs to become the default module.
> 
> How hard would that be to do in your opinion?
> 
> It would help some if we didn't have to update the dummy module with
> the changes required by apparmor.

It shouldn't be difficult (famous last words ;).  And the LSM maintainer
already said he was ok with it,
http://marc.info/?l=linux-kernel&m=121311583732168&w=2

-- 
Stephen Smalley
National Security Agency