| Message ID | 20240221-idmap-fscap-refactor-v2-12-3039364623bd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | fs: use type-safe uid representation for filesystem capabilities | expand |
On Wed, Feb 21, 2024 at 4:25 PM Seth Forshee (DigitalOcean) <sforshee@kernel.org> wrote: > > Add hooks for set/get/remove fscaps operations which perform the same > checks as the xattr hooks would have done for XATTR_NAME_CAPS. > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > --- > security/selinux/hooks.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index a6bf90ace84c..da129a387b34 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3367,6 +3367,29 @@ static int selinux_inode_removexattr(struct mnt_idmap *idmap, > return -EACCES; > } > > +static int selinux_inode_set_fscaps(struct mnt_idmap *idmap, > + struct dentry *dentry, > + const struct vfs_caps *caps, int flags) > +{ > + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); > +} The selinux_inode_setxattr() code also has a cap_inode_setxattr() check which is missing here. Unless you are handling this somewhere else, I would expect the function above to look similar to selinux_inode_remove_fscaps(), but obviously tweaked for setting the fscaps and not removing them. > +static int selinux_inode_get_fscaps(struct mnt_idmap *idmap, > + struct dentry *dentry) > +{ > + return dentry_has_perm(current_cred(), dentry, FILE__GETATTR); > +} > + > +static int selinux_inode_remove_fscaps(struct mnt_idmap *idmap, > + struct dentry *dentry) > +{ > + int rc = cap_inode_removexattr(idmap, dentry, XATTR_NAME_CAPS); > + if (rc) > + return rc; > + > + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); > +} > + > static int selinux_path_notify(const struct path *path, u64 mask, > unsigned int obj_type) > { > @@ -7165,6 +7188,9 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = { > LSM_HOOK_INIT(inode_set_acl, selinux_inode_set_acl), > LSM_HOOK_INIT(inode_get_acl, selinux_inode_get_acl), > LSM_HOOK_INIT(inode_remove_acl, selinux_inode_remove_acl), > + LSM_HOOK_INIT(inode_set_fscaps, selinux_inode_set_fscaps), > + LSM_HOOK_INIT(inode_get_fscaps, selinux_inode_get_fscaps), > + LSM_HOOK_INIT(inode_remove_fscaps, selinux_inode_remove_fscaps), > LSM_HOOK_INIT(inode_getsecurity, selinux_inode_getsecurity), > LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity), > LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity), > > -- > 2.43.0
On Wed, Feb 21, 2024 at 06:38:33PM -0500, Paul Moore wrote: > On Wed, Feb 21, 2024 at 4:25 PM Seth Forshee (DigitalOcean) > <sforshee@kernel.org> wrote: > > > > Add hooks for set/get/remove fscaps operations which perform the same > > checks as the xattr hooks would have done for XATTR_NAME_CAPS. > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > --- > > security/selinux/hooks.c | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index a6bf90ace84c..da129a387b34 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -3367,6 +3367,29 @@ static int selinux_inode_removexattr(struct mnt_idmap *idmap, > > return -EACCES; > > } > > > > +static int selinux_inode_set_fscaps(struct mnt_idmap *idmap, > > + struct dentry *dentry, > > + const struct vfs_caps *caps, int flags) > > +{ > > + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); > > +} > > The selinux_inode_setxattr() code also has a cap_inode_setxattr() > check which is missing here. Unless you are handling this somewhere > else, I would expect the function above to look similar to > selinux_inode_remove_fscaps(), but obviously tweaked for setting the > fscaps and not removing them. Right, but cap_inode_setxattr() doesn't do anything for fscaps, so I omitted the call. Unless you think the call should be included in case cap_inode_setxattr() changes in the future, which is a reasonable position. Thanks, Seth
On Wed, Feb 21, 2024 at 7:10 PM Seth Forshee (DigitalOcean) <sforshee@kernel.org> wrote: > On Wed, Feb 21, 2024 at 06:38:33PM -0500, Paul Moore wrote: > > On Wed, Feb 21, 2024 at 4:25 PM Seth Forshee (DigitalOcean) > > <sforshee@kernel.org> wrote: > > > > > > Add hooks for set/get/remove fscaps operations which perform the same > > > checks as the xattr hooks would have done for XATTR_NAME_CAPS. > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > --- > > > security/selinux/hooks.c | 26 ++++++++++++++++++++++++++ > > > 1 file changed, 26 insertions(+) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index a6bf90ace84c..da129a387b34 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -3367,6 +3367,29 @@ static int selinux_inode_removexattr(struct mnt_idmap *idmap, > > > return -EACCES; > > > } > > > > > > +static int selinux_inode_set_fscaps(struct mnt_idmap *idmap, > > > + struct dentry *dentry, > > > + const struct vfs_caps *caps, int flags) > > > +{ > > > + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); > > > +} > > > > The selinux_inode_setxattr() code also has a cap_inode_setxattr() > > check which is missing here. Unless you are handling this somewhere > > else, I would expect the function above to look similar to > > selinux_inode_remove_fscaps(), but obviously tweaked for setting the > > fscaps and not removing them. > > Right, but cap_inode_setxattr() doesn't do anything for fscaps, so I > omitted the call. Unless you think the call should be included in case > cap_inode_setxattr() changes in the future, which is a reasonable > position. Fair enough, but I'd be a lot happier if you included the call in case something changes in the future. I worry that omitting the call would make it easier for us to forget about this if/when things change and suddenly we have a security issue. If you are morally opposed to that, at the very least put a comment in selinux_inode_set_fscaps() about this so we know who to yell at in the future ;)
On Wed, Feb 21, 2024 at 07:19:07PM -0500, Paul Moore wrote: > On Wed, Feb 21, 2024 at 7:10 PM Seth Forshee (DigitalOcean) > <sforshee@kernel.org> wrote: > > On Wed, Feb 21, 2024 at 06:38:33PM -0500, Paul Moore wrote: > > > On Wed, Feb 21, 2024 at 4:25 PM Seth Forshee (DigitalOcean) > > > <sforshee@kernel.org> wrote: > > > > > > > > Add hooks for set/get/remove fscaps operations which perform the same > > > > checks as the xattr hooks would have done for XATTR_NAME_CAPS. > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > --- > > > > security/selinux/hooks.c | 26 ++++++++++++++++++++++++++ > > > > 1 file changed, 26 insertions(+) > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > index a6bf90ace84c..da129a387b34 100644 > > > > --- a/security/selinux/hooks.c > > > > +++ b/security/selinux/hooks.c > > > > @@ -3367,6 +3367,29 @@ static int selinux_inode_removexattr(struct mnt_idmap *idmap, > > > > return -EACCES; > > > > } > > > > > > > > +static int selinux_inode_set_fscaps(struct mnt_idmap *idmap, > > > > + struct dentry *dentry, > > > > + const struct vfs_caps *caps, int flags) > > > > +{ > > > > + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); > > > > +} > > > > > > The selinux_inode_setxattr() code also has a cap_inode_setxattr() > > > check which is missing here. Unless you are handling this somewhere > > > else, I would expect the function above to look similar to > > > selinux_inode_remove_fscaps(), but obviously tweaked for setting the > > > fscaps and not removing them. > > > > Right, but cap_inode_setxattr() doesn't do anything for fscaps, so I > > omitted the call. Unless you think the call should be included in case > > cap_inode_setxattr() changes in the future, which is a reasonable > > position. > > Fair enough, but I'd be a lot happier if you included the call in case > something changes in the future. I worry that omitting the call would > make it easier for us to forget about this if/when things change and > suddenly we have a security issue. If you are morally opposed to > that, at the very least put a comment in selinux_inode_set_fscaps() > about this so we know who to yell at in the future ;) Makes sense, no objection from me. I'll add it in for v3.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index a6bf90ace84c..da129a387b34 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3367,6 +3367,29 @@ static int selinux_inode_removexattr(struct mnt_idmap *idmap, return -EACCES; } +static int selinux_inode_set_fscaps(struct mnt_idmap *idmap, + struct dentry *dentry, + const struct vfs_caps *caps, int flags) +{ + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); +} + +static int selinux_inode_get_fscaps(struct mnt_idmap *idmap, + struct dentry *dentry) +{ + return dentry_has_perm(current_cred(), dentry, FILE__GETATTR); +} + +static int selinux_inode_remove_fscaps(struct mnt_idmap *idmap, + struct dentry *dentry) +{ + int rc = cap_inode_removexattr(idmap, dentry, XATTR_NAME_CAPS); + if (rc) + return rc; + + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); +} + static int selinux_path_notify(const struct path *path, u64 mask, unsigned int obj_type) { @@ -7165,6 +7188,9 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = { LSM_HOOK_INIT(inode_set_acl, selinux_inode_set_acl), LSM_HOOK_INIT(inode_get_acl, selinux_inode_get_acl), LSM_HOOK_INIT(inode_remove_acl, selinux_inode_remove_acl), + LSM_HOOK_INIT(inode_set_fscaps, selinux_inode_set_fscaps), + LSM_HOOK_INIT(inode_get_fscaps, selinux_inode_get_fscaps), + LSM_HOOK_INIT(inode_remove_fscaps, selinux_inode_remove_fscaps), LSM_HOOK_INIT(inode_getsecurity, selinux_inode_getsecurity), LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity), LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
Add hooks for set/get/remove fscaps operations which perform the same checks as the xattr hooks would have done for XATTR_NAME_CAPS. Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> --- security/selinux/hooks.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)