On Tue, Jan 30, 2024 at 7:09 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Mon, Jan 29, 2024 at 8:31 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> >
> > These two hooks currently work like this:
> > 1. If no LSM registers the hook, cap_inode_{set,remove}xattr() is
> > called.
> > 2. If an LSM hook call returns 0, the loop continues to call further
> > LSMs (and only stops on an error return value).
> > 3. The "default" return value is 0, so e.g. the default BPF LSM hook
> > just returns 0.
> >
> > This works if BPF LSM is enabled along with SELinux or SMACK (or not
> > enabled at all), but if it's the only LSM implementing the hook, then
> > the cap_inode_{set,remove}xattr() is erroneously skipped.
> >
> > Fix this by using 1 as the default return value and make the loop
> > recognize it as a no-op return value (i.e. if an LSM returns this value
> > it is treated as if it wasn't called at all). The final logic is similar
> > to that of security_fs_context_parse_param().
> >
> > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > ---
> > include/linux/lsm_hook_defs.h | 4 ++--
> > security/security.c | 45 +++++++++++++++++++++++++----------
> > 2 files changed, 35 insertions(+), 14 deletions(-)
>
> Thanks for working on this Ondrej, I've got a couple of thoughts on
> the approach taken here, but we definitely need to fix this.
>
> My first thought is that we really should move the
> cap_inode_setxattr() and cap_inode_removexattr() calls in security.c
> over to using the LSM hook infrastructure just as we do with other
> capability hooks in commoncap.c:
>
> LSM_HOOK_INIT(inode_setxattr, cap_inode_setxattr);
> LSM_HOOK_INIT(inode_removexattr, cap_inode_removexattr);
>
> ... of course we will need to adjust cap_inode_setxattr to take (and
> ignore the idmap) parameter, but that is easy enough. It looks like
> cap_inode_removexattr() can be used as-is. Modifications to the only
> two LSMs, SELinux and Smack, which explicitly call out to these
> capability hooks looks rather straightforward as well. Doing this
> should simplify the LSM hooks significantly, and lower the chance of a
> future LSM mistakenly not doing the required capability calls. There
> should also be a slight performance bump for the few (one? two?)
> people running both SELinux and Smack in a production environment.
>
> My second thought is that we *really* need to add to the function
> header block comment/description for both these hooks. Of course the
> details here will change depending on the bits above about the
> capability hooks, but if we need any special handling like you're
> proposing here we really should document it in the hook's header
> block.
A completely untested, other than compiling security/, patch is below
demonstrating what I was thinking. I've also attached the same patch
in case anyone wants to actually try it out as the cut-n-paste version
below is surely whitespace damaged. I will warn you that this was
hastily thrown together so it is very likely I screwed something up :)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..2d3c0af33b65 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -156,7 +156,8 @@ extern int cap_capset(struct cred *new, const
struct cred *old,
const kernel_cap_t *inheritable,
const kernel_cap_t *permitted);
extern int cap_bprm_creds_from_file(struct linux_binprm *bprm, const
struct file *file);
-int cap_inode_setxattr(struct dentry *dentry, const char *name,
+int cap_inode_setxattr(struct mnt_idmap *idmap,
+ struct dentry *dentry, const char *name,
const void *value, size_t size, int flags);
int cap_inode_removexattr(struct mnt_idmap *idmap,
struct dentry *dentry, const char *name);
@@ -888,7 +889,7 @@ static inline int security_inode_setxattr(struct
mnt_idmap *idmap,
struct dentry *dentry, const char *name, const void *value,
size_t size, int flags)
{
- return cap_inode_setxattr(dentry, name, value, size, flags);
+ return cap_inode_setxattr(idmap, dentry, name, value, size, flags);
}
static inline int security_inode_set_acl(struct mnt_idmap *idmap,
diff --git a/security/commoncap.c b/security/commoncap.c
index 162d96b3a676..34caf0e19b2f 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -975,6 +975,7 @@ int cap_bprm_creds_from_file(struct linux_binprm
*bprm, const struct file *file)
/**
* cap_inode_setxattr - Determine whether an xattr may be altered
+ * @idmap: idmap of the mount
* @dentry: The inode/dentry being altered
* @name: The name of the xattr to be changed
* @value: The value that the xattr will be changed to
@@ -987,7 +988,8 @@ int cap_bprm_creds_from_file(struct linux_binprm
*bprm, const struct file *file)
* This is used to make sure security xattrs don't get updated or set by those
* who aren't privileged to do so.
*/
-int cap_inode_setxattr(struct dentry *dentry, const char *name,
+int cap_inode_setxattr(struct mnt_idmap *idmap,
+ struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)
{
struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
@@ -1457,6 +1459,8 @@ static struct security_hook_list
capability_hooks[] __ro_after_init = {
LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv),
LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv),
LSM_HOOK_INIT(inode_getsecurity, cap_inode_getsecurity),
+ LSM_HOOK_INIT(inode_setxattr, cap_inode_setxattr),
+ LSM_HOOK_INIT(inode_removexattr, cap_inode_removexattr),
LSM_HOOK_INIT(mmap_addr, cap_mmap_addr),
LSM_HOOK_INIT(mmap_file, cap_mmap_file),
LSM_HOOK_INIT(task_fix_setuid, cap_task_fix_setuid),
diff --git a/security/security.c b/security/security.c
index 3aaad75c9ce8..6425d177b301 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2258,15 +2258,9 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return 0;
- /*
- * SELinux and Smack integrate the cap call,
- * so assume that all LSMs supplying this call do so.
- */
- ret = call_int_hook(inode_setxattr, 1, idmap, dentry, name, value,
- size, flags);
- if (ret == 1)
- ret = cap_inode_setxattr(dentry, name, value, size, flags);
+ ret = call_int_hook(inode_setxattr, 0, idmap, dentry, name, value,
+ size, flags);
if (ret)
return ret;
ret = ima_inode_setxattr(dentry, name, value, size);
@@ -2421,13 +2415,8 @@ int security_inode_removexattr(struct mnt_idmap *idmap,
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return 0;
- /*
- * SELinux and Smack integrate the cap call,
- * so assume that all LSMs supplying this call do so.
- */
- ret = call_int_hook(inode_removexattr, 1, idmap, dentry, name);
- if (ret == 1)
- ret = cap_inode_removexattr(idmap, dentry, name);
+
+ ret = call_int_hook(inode_removexattr, 0, idmap, dentry, name);
if (ret)
return ret;
ret = ima_inode_removexattr(dentry, name);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a6bf90ace84c..49cb331a0d84 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3193,10 +3193,6 @@ static int selinux_inode_setxattr(struct
mnt_idmap *idmap,
int rc = 0;
if (strcmp(name, XATTR_NAME_SELINUX)) {
- rc = cap_inode_setxattr(dentry, name, value, size, flags);
- if (rc)
- return rc;
-
/* Not an attribute we recognize, so just check the
ordinary setattr permission. */
return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
@@ -3350,10 +3346,6 @@ static int selinux_inode_removexattr(struct
mnt_idmap *idmap,
struct dentry *dentry, const char *name)
{
if (strcmp(name, XATTR_NAME_SELINUX)) {
- int rc = cap_inode_removexattr(idmap, dentry, name);
- if (rc)
- return rc;
-
/* Not an attribute we recognize, so just check the
ordinary setattr permission. */
return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0fdbf04cc258..34b74e442412 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1317,8 +1317,7 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap,
if (size != TRANS_TRUE_SIZE ||
strncmp(value, TRANS_TRUE, TRANS_TRUE_SIZE) != 0)
rc = -EINVAL;
- } else
- rc = cap_inode_setxattr(dentry, name, value, size, flags);
+ }
if (check_priv && !smack_privileged(CAP_MAC_ADMIN))
rc = -EPERM;
@@ -1426,12 +1425,8 @@ static int smack_inode_removexattr(struct
mnt_idmap *idmap,
strcmp(name, XATTR_NAME_SMACKTRANSMUTE) == 0 ||
strcmp(name, XATTR_NAME_SMACKMMAP) == 0) {
if (!smack_privileged(CAP_MAC_ADMIN))
- rc = -EPERM;
- } else
- rc = cap_inode_removexattr(idmap, dentry, name);
-
- if (rc != 0)
- return rc;
+ return -EPERM;
+ }
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_DENTRY);
smk_ad_setfield_u_fs_path_dentry(&ad, dentry);
--
paul-moore.com
lsm,selinux,smack: fix/cleanup security_inode_{set,remove}xattr()
From: Paul Moore <paul@xxxxxxxxxxxxxx>
XXX write a proper commit description
Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
---
include/linux/security.h | 5 +++--
security/commoncap.c | 6 +++++-
security/security.c | 19 ++++---------------
security/selinux/hooks.c | 8 --------
security/smack/smack_lsm.c | 11 +++--------
5 files changed, 15 insertions(+), 34 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..2d3c0af33b65 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -156,7 +156,8 @@ extern int cap_capset(struct cred *new, const struct cred *old,
const kernel_cap_t *inheritable,
const kernel_cap_t *permitted);
extern int cap_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file);
-int cap_inode_setxattr(struct dentry *dentry, const char *name,
+int cap_inode_setxattr(struct mnt_idmap *idmap,
+ struct dentry *dentry, const char *name,
const void *value, size_t size, int flags);
int cap_inode_removexattr(struct mnt_idmap *idmap,
struct dentry *dentry, const char *name);
@@ -888,7 +889,7 @@ static inline int security_inode_setxattr(struct mnt_idmap *idmap,
struct dentry *dentry, const char *name, const void *value,
size_t size, int flags)
{
- return cap_inode_setxattr(dentry, name, value, size, flags);
+ return cap_inode_setxattr(idmap, dentry, name, value, size, flags);
}
static inline int security_inode_set_acl(struct mnt_idmap *idmap,
diff --git a/security/commoncap.c b/security/commoncap.c
index 162d96b3a676..34caf0e19b2f 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -975,6 +975,7 @@ int cap_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file)
/**
* cap_inode_setxattr - Determine whether an xattr may be altered
+ * @idmap: idmap of the mount
* @dentry: The inode/dentry being altered
* @name: The name of the xattr to be changed
* @value: The value that the xattr will be changed to
@@ -987,7 +988,8 @@ int cap_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file)
* This is used to make sure security xattrs don't get updated or set by those
* who aren't privileged to do so.
*/
-int cap_inode_setxattr(struct dentry *dentry, const char *name,
+int cap_inode_setxattr(struct mnt_idmap *idmap,
+ struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)
{
struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
@@ -1457,6 +1459,8 @@ static struct security_hook_list capability_hooks[] __ro_after_init = {
LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv),
LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv),
LSM_HOOK_INIT(inode_getsecurity, cap_inode_getsecurity),
+ LSM_HOOK_INIT(inode_setxattr, cap_inode_setxattr),
+ LSM_HOOK_INIT(inode_removexattr, cap_inode_removexattr),
LSM_HOOK_INIT(mmap_addr, cap_mmap_addr),
LSM_HOOK_INIT(mmap_file, cap_mmap_file),
LSM_HOOK_INIT(task_fix_setuid, cap_task_fix_setuid),
diff --git a/security/security.c b/security/security.c
index 3aaad75c9ce8..6425d177b301 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2258,15 +2258,9 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return 0;
- /*
- * SELinux and Smack integrate the cap call,
- * so assume that all LSMs supplying this call do so.
- */
- ret = call_int_hook(inode_setxattr, 1, idmap, dentry, name, value,
- size, flags);
- if (ret == 1)
- ret = cap_inode_setxattr(dentry, name, value, size, flags);
+ ret = call_int_hook(inode_setxattr, 0, idmap, dentry, name, value,
+ size, flags);
if (ret)
return ret;
ret = ima_inode_setxattr(dentry, name, value, size);
@@ -2421,13 +2415,8 @@ int security_inode_removexattr(struct mnt_idmap *idmap,
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return 0;
- /*
- * SELinux and Smack integrate the cap call,
- * so assume that all LSMs supplying this call do so.
- */
- ret = call_int_hook(inode_removexattr, 1, idmap, dentry, name);
- if (ret == 1)
- ret = cap_inode_removexattr(idmap, dentry, name);
+
+ ret = call_int_hook(inode_removexattr, 0, idmap, dentry, name);
if (ret)
return ret;
ret = ima_inode_removexattr(dentry, name);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a6bf90ace84c..49cb331a0d84 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3193,10 +3193,6 @@ static int selinux_inode_setxattr(struct mnt_idmap *idmap,
int rc = 0;
if (strcmp(name, XATTR_NAME_SELINUX)) {
- rc = cap_inode_setxattr(dentry, name, value, size, flags);
- if (rc)
- return rc;
-
/* Not an attribute we recognize, so just check the
ordinary setattr permission. */
return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
@@ -3350,10 +3346,6 @@ static int selinux_inode_removexattr(struct mnt_idmap *idmap,
struct dentry *dentry, const char *name)
{
if (strcmp(name, XATTR_NAME_SELINUX)) {
- int rc = cap_inode_removexattr(idmap, dentry, name);
- if (rc)
- return rc;
-
/* Not an attribute we recognize, so just check the
ordinary setattr permission. */
return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0fdbf04cc258..34b74e442412 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1317,8 +1317,7 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap,
if (size != TRANS_TRUE_SIZE ||
strncmp(value, TRANS_TRUE, TRANS_TRUE_SIZE) != 0)
rc = -EINVAL;
- } else
- rc = cap_inode_setxattr(dentry, name, value, size, flags);
+ }
if (check_priv && !smack_privileged(CAP_MAC_ADMIN))
rc = -EPERM;
@@ -1426,12 +1425,8 @@ static int smack_inode_removexattr(struct mnt_idmap *idmap,
strcmp(name, XATTR_NAME_SMACKTRANSMUTE) == 0 ||
strcmp(name, XATTR_NAME_SMACKMMAP) == 0) {
if (!smack_privileged(CAP_MAC_ADMIN))
- rc = -EPERM;
- } else
- rc = cap_inode_removexattr(idmap, dentry, name);
-
- if (rc != 0)
- return rc;
+ return -EPERM;
+ }
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_DENTRY);
smk_ad_setfield_u_fs_path_dentry(&ad, dentry);
