[patch] security: fix dummy xattr functions - Kernel

This is a discussion on [patch] security: fix dummy xattr functions - Kernel ; Hi James, If this (untested) patch looks OK, could you please apply it to your tree? Thanks, Miklos ---- From: Miklos Szeredi Replace open coded xattr checks with cap_inode_xxx() function calls in dummy_inode_setxattr() and dummy_inode_removexattr(). The old ones were out ...

+ Reply to Thread
Results 1 to 8 of 8

Thread: [patch] security: fix dummy xattr functions

  1. [patch] security: fix dummy xattr functions

    Hi James,

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

    Thanks,
    Miklos

    ----
    From: Miklos Szeredi

    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
    Signed-off-by: Miklos Szeredi
    ---
    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)
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [patch] security: fix dummy xattr functions

    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
    > Signed-off-by: Miklos Szeredi
    > ---
    > 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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [patch] security: fix dummy xattr functions

    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
    > > Signed-off-by: Miklos Szeredi
    > > ---
    > > 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
    >
    >

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [patch] security: fix dummy xattr functions

    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [patch] security: fix dummy xattr functions

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [patch] security: fix dummy xattr functions

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [patch] security: fix dummy xattr functions


    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [patch] security: fix dummy xattr functions


    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread