Hi James,
If this (untested) patch looks OK, could you please apply it to your
tree?
Thanks,
Miklos
----
From: Miklos Szeredi <[email protected]>
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 <[email protected]>
Signed-off-by: Miklos Szeredi <[email protected]>
---
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)
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 <[email protected]>
> Signed-off-by: Miklos Szeredi <[email protected]>
> ---
> 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
<[email protected]>
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 <[email protected]>
> > Signed-off-by: Miklos Szeredi <[email protected]>
> > ---
> > 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
> <[email protected]>
>
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
<[email protected]>
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
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
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
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