On 5/2/2024 5:58 PM, Paul Moore wrote: > The current security_inode_setxattr() and security_inode_removexattr() > hooks rely on individual LSMs to either call into the associated > capability hooks (cap_inode_setxattr() or cap_inode_removexattr()), or > return a magic value of 1 to indicate that the LSM layer itself should > perform the capability checks. Unfortunately, with the default return > value for these LSM hooks being 0, an individual LSM hook returning a > 1 will cause the LSM hook processing to exit early, potentially > skipping a LSM. Thankfully, with the exception of the BPF LSM, none > of the LSMs which currently register inode xattr hooks should end up > returning a value of 1, and in the BPF LSM case, with the BPF LSM hooks > executing last there should be no real harm in stopping processing of > the LSM hooks. However, the reliance on the individual LSMs to either > call the capability hooks themselves, or signal the LSM with a return > value of 1, is fragile and relies on a specific set of LSMs being > enabled. This patch is an effort to resolve, or minimize, these > issues. > > Before we discuss the solution, https://lore.kernel.org/lkml/20231215221636.105680-1-casey@xxxxxxxxxxxxxxxx/T/#mac61625dc1983d13ee5e8015fd22e1165381f079 ... or am I missing something? > there are a few observations and > considerations that we need to take into account: > * BPF LSM registers an implementation for every LSM hook, and that > implementation simply returns the hook's default return value, a > 0 in this case. We want to ensure that the default BPF LSM behavior > results in the capability checks being called. > * SELinux and Smack do not expect the traditional capability checks > to be applied to the xattrs that they "own". > * SELinux and Smack are currently written in such a way that the > xattr capability checks happen before any additional LSM specific > access control checks. SELinux does apply SELinux specific access > controls to all xattrs, even those not "owned" by SELinux. > * IMA and EVM also register xattr hooks but assume that the LSM layer > and specific LSMs have already authorized the basic xattr operation. > > In order to ensure we perform the capability based access controls > before the individual LSM access controls, perform only one capability > access control check for each operation, and clarify the logic around > applying the capability controls, we need a mechanism to determine if > any of the enabled LSMs "own" a particular xattr and want to take > responsibility for controlling access to that xattr. The solution in > this patch is to create a new LSM hook, 'inode_xattr_skipcap', that is > not exported to the rest of the kernel via a security_XXX() function, > but is used by the LSM layer to determine if a LSM wants to control > access to a given xattr and avoid the traditional capability controls. > Registering an inode_xattr_skipcap hook is optional, if a LSM declines > to register an implementation, or uses an implementation that simply > returns the default value (0), there is no effect as the LSM continues > to enforce the capability based controls (unless another LSM takes > ownership of the xattr). If none of the LSMs signal that the > capability checks should be skipped, the capability check is performed > and if access is granted the individual LSM xattr access control hooks > are executed, keeping with the DAC-before-LSM convention. > > Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx> > --- > include/linux/lsm_hook_defs.h | 1 + > security/security.c | 70 ++++++++++++++++++++++++----------- > security/selinux/hooks.c | 28 ++++++++++---- > security/smack/smack_lsm.c | 31 +++++++++++++++- > 4 files changed, 98 insertions(+), 32 deletions(-) > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index 334e00efbde4..6e54dae3256b 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -144,6 +144,7 @@ LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry, > LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap, > struct dentry *dentry, int ia_valid) > LSM_HOOK(int, 0, inode_getattr, const struct path *path) > +LSM_HOOK(int, 0, inode_xattr_skipcap, const char *name) > LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap, > struct dentry *dentry, const char *name, const void *value, > size_t size, int flags) > diff --git a/security/security.c b/security/security.c > index 7e118858b545..1f5c68e2a62a 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2278,7 +2278,20 @@ int security_inode_getattr(const struct path *path) > * @size: size of xattr value > * @flags: flags > * > - * Check permission before setting the extended attributes. > + * This hook performs the desired permission checks before setting the extended > + * attributes (xattrs) on @dentry. It is important to note that we have some > + * additional logic before the main LSM implementation calls to detect if we > + * need to perform an additional capability check at the LSM layer. > + * > + * Normally we enforce a capability check prior to executing the various LSM > + * hook implementations, but if a LSM wants to avoid this capability check, > + * it can register a 'inode_xattr_skipcap' hook and return a value of 1 for > + * xattrs that it wants to avoid the capability check, leaving the LSM fully > + * responsible for enforcing the access control for the specific xattr. If all > + * of the enabled LSMs refrain from registering a 'inode_xattr_skipcap' hook, > + * or return a 0 (the default return value), the capability check is still > + * performed. If no 'inode_xattr_skipcap' hooks are registered the capability > + * check is performed. > * > * Return: Returns 0 if permission is granted. > */ > @@ -2286,20 +2299,20 @@ int security_inode_setxattr(struct mnt_idmap *idmap, > struct dentry *dentry, const char *name, > const void *value, size_t size, int flags) > { > - int ret; > + int rc; > > 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, idmap, dentry, name, value, size, > - flags); > > - if (ret == 1) > - ret = cap_inode_setxattr(dentry, name, value, size, flags); > - return ret; > + /* enforce the capability checks at the lsm layer, if needed */ > + if (!call_int_hook(inode_xattr_skipcap, name)) { > + rc = cap_inode_setxattr(dentry, name, value, size, flags); > + if (rc) > + return rc; > + } > + > + return call_int_hook(inode_setxattr, idmap, dentry, name, value, size, > + flags); > } > > /** > @@ -2452,26 +2465,39 @@ int security_inode_listxattr(struct dentry *dentry) > * @dentry: file > * @name: xattr name > * > - * Check permission before removing the extended attribute identified by @name > - * for @dentry. > + * This hook performs the desired permission checks before setting the extended > + * attributes (xattrs) on @dentry. It is important to note that we have some > + * additional logic before the main LSM implementation calls to detect if we > + * need to perform an additional capability check at the LSM layer. > + * > + * Normally we enforce a capability check prior to executing the various LSM > + * hook implementations, but if a LSM wants to avoid this capability check, > + * it can register a 'inode_xattr_skipcap' hook and return a value of 1 for > + * xattrs that it wants to avoid the capability check, leaving the LSM fully > + * responsible for enforcing the access control for the specific xattr. If all > + * of the enabled LSMs refrain from registering a 'inode_xattr_skipcap' hook, > + * or return a 0 (the default return value), the capability check is still > + * performed. If no 'inode_xattr_skipcap' hooks are registered the capability > + * check is performed. > * > * Return: Returns 0 if permission is granted. > */ > int security_inode_removexattr(struct mnt_idmap *idmap, > struct dentry *dentry, const char *name) > { > - int ret; > + int rc; > > 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, idmap, dentry, name); > - if (ret == 1) > - ret = cap_inode_removexattr(idmap, dentry, name); > - return ret; > + > + /* enforce the capability checks at the lsm layer, if needed */ > + if (!call_int_hook(inode_xattr_skipcap, name)) { > + rc = cap_inode_removexattr(idmap, dentry, name); > + if (rc) > + return rc; > + } > + > + return call_int_hook(inode_removexattr, idmap, dentry, name); > } > > /** > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 3448454c82d0..7be385ebf09b 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3181,6 +3181,23 @@ static bool has_cap_mac_admin(bool audit) > return true; > } > > +/** > + * selinux_inode_xattr_skipcap - Skip the xattr capability checks? > + * @name: name of the xattr > + * > + * Returns 1 to indicate that SELinux "owns" the access control rights to xattrs > + * named @name; the LSM layer should avoid enforcing any traditional > + * capability based access controls on this xattr. Returns 0 to indicate that > + * SELinux does not "own" the access control rights to xattrs named @name and is > + * deferring to the LSM layer for further access controls, including capability > + * based controls. > + */ > +static int selinux_inode_xattr_skipcap(const char *name) > +{ > + /* require capability check if not a selinux xattr */ > + return !strcmp(name, XATTR_NAME_SELINUX); > +} > + > static int selinux_inode_setxattr(struct mnt_idmap *idmap, > struct dentry *dentry, const char *name, > const void *value, size_t size, int flags) > @@ -3192,15 +3209,9 @@ static int selinux_inode_setxattr(struct mnt_idmap *idmap, > u32 newsid, sid = current_sid(); > 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. */ > + /* if not a selinux xattr, only check the ordinary setattr perm */ > + if (strcmp(name, XATTR_NAME_SELINUX)) > return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); > - } > > if (!selinux_initialized()) > return (inode_owner_or_capable(idmap, inode) ? 0 : -EPERM); > @@ -7185,6 +7196,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = { > LSM_HOOK_INIT(inode_permission, selinux_inode_permission), > LSM_HOOK_INIT(inode_setattr, selinux_inode_setattr), > LSM_HOOK_INIT(inode_getattr, selinux_inode_getattr), > + LSM_HOOK_INIT(inode_xattr_skipcap, selinux_inode_xattr_skipcap), > LSM_HOOK_INIT(inode_setxattr, selinux_inode_setxattr), > LSM_HOOK_INIT(inode_post_setxattr, selinux_inode_post_setxattr), > LSM_HOOK_INIT(inode_getxattr, selinux_inode_getxattr), > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index 146667937811..6e37df0465a4 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -1282,6 +1282,33 @@ static int smack_inode_getattr(const struct path *path) > return rc; > } > > +/** > + * smack_inode_xattr_skipcap - Skip the xattr capability checks? > + * @name: name of the xattr > + * > + * Returns 1 to indicate that Smack "owns" the access control rights to xattrs > + * named @name; the LSM layer should avoid enforcing any traditional > + * capability based access controls on this xattr. Returns 0 to indicate that > + * Smack does not "own" the access control rights to xattrs named @name and is > + * deferring to the LSM layer for further access controls, including capability > + * based controls. > + */ > +static int smack_inode_xattr_skipcap(const char *name) > +{ > + if (strncmp(name, XATTR_SMACK_SUFFIX, strlen(XATTR_SMACK_SUFFIX))) > + return 0; > + > + if (strcmp(name, XATTR_NAME_SMACK) == 0 || > + strcmp(name, XATTR_NAME_SMACKIPIN) == 0 || > + strcmp(name, XATTR_NAME_SMACKIPOUT) == 0 || > + strcmp(name, XATTR_NAME_SMACKEXEC) == 0 || > + strcmp(name, XATTR_NAME_SMACKMMAP) == 0 || > + strcmp(name, XATTR_NAME_SMACKTRANSMUTE) == 0) > + return 1; > + > + return 0; > +} > + > /** > * smack_inode_setxattr - Smack check for setting xattrs > * @idmap: idmap of the mount > @@ -1325,8 +1352,7 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap, > 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; > @@ -5050,6 +5076,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = { > LSM_HOOK_INIT(inode_permission, smack_inode_permission), > LSM_HOOK_INIT(inode_setattr, smack_inode_setattr), > LSM_HOOK_INIT(inode_getattr, smack_inode_getattr), > + LSM_HOOK_INIT(inode_xattr_skipcap, smack_inode_xattr_skipcap), > LSM_HOOK_INIT(inode_setxattr, smack_inode_setxattr), > LSM_HOOK_INIT(inode_post_setxattr, smack_inode_post_setxattr), > LSM_HOOK_INIT(inode_getxattr, smack_inode_getxattr),
