| Message ID | 20240221-idmap-fscap-refactor-v2-24-3039364623bd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: use type-safe uid representation for filesystem capabilities | expand |
On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > Use the vfs interfaces for fetching file capabilities for killpriv > checks and from get_vfs_caps_from_disk(). While there, update the > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > from vfs_get_fscaps_nosec(). > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > --- > security/commoncap.c | 30 +++++++++++++----------------- > 1 file changed, 13 insertions(+), 17 deletions(-) > > diff --git a/security/commoncap.c b/security/commoncap.c > index a0ff7e6092e0..751bb26a06a6 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > */ > int cap_inode_need_killpriv(struct dentry *dentry) > { > - struct inode *inode = d_backing_inode(dentry); > + struct vfs_caps caps; > int error; > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > - return error > 0; > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > + return error == 0; > } > > /** > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > { > int error; > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > + error = vfs_remove_fscaps_nosec(idmap, dentry); Uhm, I see that the change is logically correct... but the original code was not correct, since the EVM post hook is not called (thus the HMAC is broken, or an xattr change is allowed on a portable signature which should be not). For completeness, the xattr change on a portable signature should not happen in the first place, so cap_inode_killpriv() would not be called. However, since EVM allows same value change, we are here. Here is how I discovered this problem. Example: # ls -l test-file -rw-r-Sr--. 1 3001 3001 5 Mar 4 10:11 test-file # getfattr -m - -d -e hex test-file # file: test-file security.capability=0x0100000202300000023000000000000000000000 security.evm=0x05020498c82b5300663064023052a1aa6200d08b3db60a1c636b97b52658af369ee0bf521cfca6c733671ebf5764b1b122f67030cfc688a111c19a7ed3023039895966cf92217ea55c1405212ced1396c2d830ae55dbdb517c5d199c5a43638f90d430bad48191149dcc7c01f772ac security.ima=0x0404f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 security.selinux=0x756e636f6e66696e65645f753a6f626a6563745f723a756e6c6162656c65645f743a733000 # chown 3001 test-file # ls -l test-file -rw-r-Sr--. 1 3001 3001 5 Mar 4 10:14 test-file # getfattr -m - -d -e hex test-file # file: test-file security.evm=0x05020498c82b5300673065023100cdd772fa7f9c17aa66e654c7f9c124de1ccfd36abbe5b8100b64a296164da45d0025fd2a2dec2e9580d5c82e5a32bfca02305ea3458b74e53d743408f65e748dc6ee52964e3aedac7367a43080248f4e000c655eb8e1f4338becb81797ea37f0bca6 security.ima=0x0404f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 security.selinux=0x756e636f6e66696e65645f753a6f626a6563745f723a756e6c6162656c65645f743a733000 which breaks EVM verification. Roberto > if (error == -EOPNOTSUPP) > error = 0; > return error; > @@ -719,6 +720,10 @@ ssize_t vfs_caps_to_user_xattr(struct mnt_idmap *idmap, > * @cpu_caps: vfs capabilities > * > * Extract the on-exec-apply capability sets for an executable file. > + * For version 3 capabilities xattrs, returns the capabilities only if > + * they are applicable to current_user_ns() (i.e. that the rootid > + * corresponds to an ID which maps to ID 0 in current_user_ns() or an > + * ancestor), and returns -ENODATA otherwise. > * > * If the inode has been found through an idmapped mount the idmap of > * the vfsmount must be passed through @idmap. This function will then > @@ -731,25 +736,16 @@ int get_vfs_caps_from_disk(struct mnt_idmap *idmap, > struct vfs_caps *cpu_caps) > { > struct inode *inode = d_backing_inode(dentry); > - int size, ret; > - struct vfs_ns_cap_data data, *nscaps = &data; > + int ret; > > if (!inode) > return -ENODATA; > > - size = __vfs_getxattr((struct dentry *)dentry, inode, > - XATTR_NAME_CAPS, &data, XATTR_CAPS_SZ); > - if (size == -ENODATA || size == -EOPNOTSUPP) > + ret = vfs_get_fscaps_nosec(idmap, (struct dentry *)dentry, cpu_caps); > + if (ret == -EOPNOTSUPP || ret == -EOVERFLOW) > /* no data, that's ok */ > - return -ENODATA; > + ret = -ENODATA; > > - if (size < 0) > - return size; > - > - ret = vfs_caps_from_xattr(idmap, inode->i_sb->s_user_ns, > - cpu_caps, nscaps, size); > - if (ret == -EOVERFLOW) > - return -ENODATA; > if (ret) > return ret; > >
On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > Use the vfs interfaces for fetching file capabilities for killpriv > > checks and from get_vfs_caps_from_disk(). While there, update the > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > from vfs_get_fscaps_nosec(). > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > --- > > security/commoncap.c | 30 +++++++++++++----------------- > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > index a0ff7e6092e0..751bb26a06a6 100644 > > --- a/security/commoncap.c > > +++ b/security/commoncap.c > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > */ > > int cap_inode_need_killpriv(struct dentry *dentry) > > { > > - struct inode *inode = d_backing_inode(dentry); > > + struct vfs_caps caps; > > int error; > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > - return error > 0; > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > + return error == 0; > > } > > > > /** > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > { > > int error; > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > Uhm, I see that the change is logically correct... but the original > code was not correct, since the EVM post hook is not called (thus the > HMAC is broken, or an xattr change is allowed on a portable signature > which should be not). > > For completeness, the xattr change on a portable signature should not > happen in the first place, so cap_inode_killpriv() would not be called. > However, since EVM allows same value change, we are here. I really don't understand EVM that well and am pretty hesitant to try an change any of the logic around it. But I'll hazard a thought: should EVM have a inode_need_killpriv hook which returns an error in this situation?
On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > > Use the vfs interfaces for fetching file capabilities for killpriv > > > checks and from get_vfs_caps_from_disk(). While there, update the > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > > from vfs_get_fscaps_nosec(). > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > --- > > > security/commoncap.c | 30 +++++++++++++----------------- > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > --- a/security/commoncap.c > > > +++ b/security/commoncap.c > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > */ > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > { > > > - struct inode *inode = d_backing_inode(dentry); > > > + struct vfs_caps caps; > > > int error; > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > > - return error > 0; > > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > > + return error == 0; > > > } > > > > > > /** > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > > { > > > int error; > > > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > Uhm, I see that the change is logically correct... but the original > > code was not correct, since the EVM post hook is not called (thus the > > HMAC is broken, or an xattr change is allowed on a portable signature > > which should be not). > > > > For completeness, the xattr change on a portable signature should not > > happen in the first place, so cap_inode_killpriv() would not be called. > > However, since EVM allows same value change, we are here. > > I really don't understand EVM that well and am pretty hesitant to try an > change any of the logic around it. But I'll hazard a thought: should EVM > have a inode_need_killpriv hook which returns an error in this > situation? Uhm, I think it would not work without modifying security_inode_need_killpriv() and the hook definition. Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would not be invoked. We would need to continue the loop and let EVM know what is the current return value. Then EVM can reject the change. An alternative way would be to detect that actually we are setting the same value for inode metadata, and maybe not returning 1 from cap_inode_need_killpriv(). I would prefer the second, since EVM allows same value change and we would have an exception if there are fscaps. This solves only the case of portable signatures. We would need to change cap_inode_need_killpriv() anyway to update the HMAC for mutable files. Roberto
On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > > > Use the vfs interfaces for fetching file capabilities for killpriv > > > > checks and from get_vfs_caps_from_disk(). While there, update the > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > > > from vfs_get_fscaps_nosec(). > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > --- > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > --- a/security/commoncap.c > > > > +++ b/security/commoncap.c > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > */ > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > { > > > > - struct inode *inode = d_backing_inode(dentry); > > > > + struct vfs_caps caps; > > > > int error; > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > > > - return error > 0; > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > > > + return error == 0; > > > > } > > > > > > > > /** > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > > > { > > > > int error; > > > > > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > Uhm, I see that the change is logically correct... but the original > > > code was not correct, since the EVM post hook is not called (thus the > > > HMAC is broken, or an xattr change is allowed on a portable signature > > > which should be not). > > > > > > For completeness, the xattr change on a portable signature should not > > > happen in the first place, so cap_inode_killpriv() would not be called. > > > However, since EVM allows same value change, we are here. > > > > I really don't understand EVM that well and am pretty hesitant to try an > > change any of the logic around it. But I'll hazard a thought: should EVM > > have a inode_need_killpriv hook which returns an error in this > > situation? > > Uhm, I think it would not work without modifying > security_inode_need_killpriv() and the hook definition. > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would > not be invoked. We would need to continue the loop and let EVM know > what is the current return value. Then EVM can reject the change. > > An alternative way would be to detect that actually we are setting the > same value for inode metadata, and maybe not returning 1 from > cap_inode_need_killpriv(). > > I would prefer the second, since EVM allows same value change and we > would have an exception if there are fscaps. > > This solves only the case of portable signatures. We would need to > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > files. I see. In any case this sounds like a matter for a separate patch series.
On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) wrote: > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > Use the vfs interfaces for fetching file capabilities for killpriv > > > > > checks and from get_vfs_caps_from_disk(). While there, update the > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > > --- > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > --- a/security/commoncap.c > > > > > +++ b/security/commoncap.c > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > */ > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > { > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > + struct vfs_caps caps; > > > > > int error; > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > > > > - return error > 0; > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > > > > + return error == 0; > > > > > } > > > > > > > > > > /** > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > > > > { > > > > > int error; > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > Uhm, I see that the change is logically correct... but the original > > > > code was not correct, since the EVM post hook is not called (thus the > > > > HMAC is broken, or an xattr change is allowed on a portable signature > > > > which should be not). > > > > > > > > For completeness, the xattr change on a portable signature should not > > > > happen in the first place, so cap_inode_killpriv() would not be called. > > > > However, since EVM allows same value change, we are here. > > > > > > I really don't understand EVM that well and am pretty hesitant to try an > > > change any of the logic around it. But I'll hazard a thought: should EVM > > > have a inode_need_killpriv hook which returns an error in this > > > situation? > > > > Uhm, I think it would not work without modifying > > security_inode_need_killpriv() and the hook definition. > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would > > not be invoked. We would need to continue the loop and let EVM know > > what is the current return value. Then EVM can reject the change. > > > > An alternative way would be to detect that actually we are setting the > > same value for inode metadata, and maybe not returning 1 from > > cap_inode_need_killpriv(). > > > > I would prefer the second, since EVM allows same value change and we > > would have an exception if there are fscaps. > > > > This solves only the case of portable signatures. We would need to > > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > > files. > > I see. In any case this sounds like a matter for a separate patch > series. Agreed.
On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote: > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) wrote: > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > Use the vfs interfaces for fetching file capabilities for killpriv > > > > > > checks and from get_vfs_caps_from_disk(). While there, update the > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > > > --- > > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > > --- a/security/commoncap.c > > > > > > +++ b/security/commoncap.c > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > > */ > > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > > { > > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > > + struct vfs_caps caps; > > > > > > int error; > > > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > > > > > - return error > 0; > > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > > > > > + return error == 0; > > > > > > } > > > > > > > > > > > > /** > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > > > > > { > > > > > > int error; > > > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > > > Uhm, I see that the change is logically correct... but the original > > > > > code was not correct, since the EVM post hook is not called (thus the > > > > > HMAC is broken, or an xattr change is allowed on a portable signature > > > > > which should be not). > > > > > > > > > > For completeness, the xattr change on a portable signature should not > > > > > happen in the first place, so cap_inode_killpriv() would not be called. > > > > > However, since EVM allows same value change, we are here. > > > > > > > > I really don't understand EVM that well and am pretty hesitant to try an > > > > change any of the logic around it. But I'll hazard a thought: should EVM > > > > have a inode_need_killpriv hook which returns an error in this > > > > situation? > > > > > > Uhm, I think it would not work without modifying > > > security_inode_need_killpriv() and the hook definition. > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would > > > not be invoked. We would need to continue the loop and let EVM know > > > what is the current return value. Then EVM can reject the change. > > > > > > An alternative way would be to detect that actually we are setting the > > > same value for inode metadata, and maybe not returning 1 from > > > cap_inode_need_killpriv(). > > > > > > I would prefer the second, since EVM allows same value change and we > > > would have an exception if there are fscaps. > > > > > > This solves only the case of portable signatures. We would need to > > > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > > > files. > > > > I see. In any case this sounds like a matter for a separate patch > > series. > > Agreed. Christian, how realistic is that we don't kill priv if we are setting the same owner? Serge, would we be able to replace __vfs_removexattr() (or now vfs_get_fscaps_nosec()) with a security-equivalent alternative? Thanks Roberto
On Tue, Mar 05, 2024 at 01:46:56PM +0100, Roberto Sassu wrote: > On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote: > > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) wrote: > > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > > Use the vfs interfaces for fetching file capabilities for killpriv > > > > > > > checks and from get_vfs_caps_from_disk(). While there, update the > > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > > > > --- > > > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > > > --- a/security/commoncap.c > > > > > > > +++ b/security/commoncap.c > > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > > > */ > > > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > > > { > > > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > > > + struct vfs_caps caps; > > > > > > > int error; > > > > > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > > > > > > - return error > 0; > > > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > > > > > > + return error == 0; > > > > > > > } > > > > > > > > > > > > > > /** > > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > > > > > > { > > > > > > > int error; > > > > > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > > > > > Uhm, I see that the change is logically correct... but the original > > > > > > code was not correct, since the EVM post hook is not called (thus the > > > > > > HMAC is broken, or an xattr change is allowed on a portable signature > > > > > > which should be not). > > > > > > > > > > > > For completeness, the xattr change on a portable signature should not > > > > > > happen in the first place, so cap_inode_killpriv() would not be called. > > > > > > However, since EVM allows same value change, we are here. > > > > > > > > > > I really don't understand EVM that well and am pretty hesitant to try an > > > > > change any of the logic around it. But I'll hazard a thought: should EVM > > > > > have a inode_need_killpriv hook which returns an error in this > > > > > situation? > > > > > > > > Uhm, I think it would not work without modifying > > > > security_inode_need_killpriv() and the hook definition. > > > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would > > > > not be invoked. We would need to continue the loop and let EVM know > > > > what is the current return value. Then EVM can reject the change. > > > > > > > > An alternative way would be to detect that actually we are setting the > > > > same value for inode metadata, and maybe not returning 1 from > > > > cap_inode_need_killpriv(). > > > > > > > > I would prefer the second, since EVM allows same value change and we > > > > would have an exception if there are fscaps. > > > > > > > > This solves only the case of portable signatures. We would need to > > > > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > > > > files. > > > > > > I see. In any case this sounds like a matter for a separate patch > > > series. > > > > Agreed. > > Christian, how realistic is that we don't kill priv if we are setting > the same owner? Uhm, I would need to see the wider context of the proposed change. But iiuc then you would be comparing current and new fscaps and if they are identical you don't kill privs? I think that would work. But again, I would need to see the actual context/change to say something meaningful.
On Tue, 2024-03-05 at 17:26 +0100, Christian Brauner wrote: > On Tue, Mar 05, 2024 at 01:46:56PM +0100, Roberto Sassu wrote: > > On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote: > > > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) wrote: > > > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > > > Use the vfs interfaces for fetching file capabilities for killpriv > > > > > > > > checks and from get_vfs_caps_from_disk(). While there, update the > > > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > > > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > > > > > --- > > > > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > > > > --- a/security/commoncap.c > > > > > > > > +++ b/security/commoncap.c > > > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > > > > */ > > > > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > > > > { > > > > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > > > > + struct vfs_caps caps; > > > > > > > > int error; > > > > > > > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > > > > > > > - return error > 0; > > > > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > > > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > > > > > > > + return error == 0; > > > > > > > > } > > > > > > > > > > > > > > > > /** > > > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > > > > > > > { > > > > > > > > int error; > > > > > > > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > > > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > > > > > > > Uhm, I see that the change is logically correct... but the original > > > > > > > code was not correct, since the EVM post hook is not called (thus the > > > > > > > HMAC is broken, or an xattr change is allowed on a portable signature > > > > > > > which should be not). > > > > > > > > > > > > > > For completeness, the xattr change on a portable signature should not > > > > > > > happen in the first place, so cap_inode_killpriv() would not be called. > > > > > > > However, since EVM allows same value change, we are here. > > > > > > > > > > > > I really don't understand EVM that well and am pretty hesitant to try an > > > > > > change any of the logic around it. But I'll hazard a thought: should EVM > > > > > > have a inode_need_killpriv hook which returns an error in this > > > > > > situation? > > > > > > > > > > Uhm, I think it would not work without modifying > > > > > security_inode_need_killpriv() and the hook definition. > > > > > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would > > > > > not be invoked. We would need to continue the loop and let EVM know > > > > > what is the current return value. Then EVM can reject the change. > > > > > > > > > > An alternative way would be to detect that actually we are setting the > > > > > same value for inode metadata, and maybe not returning 1 from > > > > > cap_inode_need_killpriv(). > > > > > > > > > > I would prefer the second, since EVM allows same value change and we > > > > > would have an exception if there are fscaps. > > > > > > > > > > This solves only the case of portable signatures. We would need to > > > > > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > > > > > files. > > > > > > > > I see. In any case this sounds like a matter for a separate patch > > > > series. > > > > > > Agreed. > > > > Christian, how realistic is that we don't kill priv if we are setting > > the same owner? > > Uhm, I would need to see the wider context of the proposed change. But > iiuc then you would be comparing current and new fscaps and if they are > identical you don't kill privs? I think that would work. But again, I > would need to see the actual context/change to say something meaningful. Ok, basically a software vendor can ship binaries with a signature over file metadata, including UID/GID, etc. A system can verify the signature through the public key of the software vendor. The problem is if someone (or even tar), executes chown on that binary, fscaps are lost. Thus, signature verification will fail from now on. EVM locks file metadata as soon as signature verification succeeds (i.e. metadata are the same of those signed by the software vendor). EVM locking works if someone is trying to set different metadata. But, if I try to chown to the same owner as the one stored in the inode, EVM allows it but the capability LSM removes security.capability, thus invalidating the signature. At least, it would be desirable that security.capability is not removed when setting the same owner. If the owner is different, EVM will handle that. Roberto
On Tue, Mar 05, 2024 at 05:35:11PM +0100, Roberto Sassu wrote: > On Tue, 2024-03-05 at 17:26 +0100, Christian Brauner wrote: > > On Tue, Mar 05, 2024 at 01:46:56PM +0100, Roberto Sassu wrote: > > > On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote: > > > > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) wrote: > > > > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > > > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > > > > Use the vfs interfaces for fetching file capabilities for killpriv > > > > > > > > > checks and from get_vfs_caps_from_disk(). While there, update the > > > > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > > > > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > > > > > > --- > > > > > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > > > > > --- a/security/commoncap.c > > > > > > > > > +++ b/security/commoncap.c > > > > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > > > > > */ > > > > > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > > > > > { > > > > > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > > > > > + struct vfs_caps caps; > > > > > > > > > int error; > > > > > > > > > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > > > > > > > > - return error > 0; > > > > > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > > > > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > > > > > > > > + return error == 0; > > > > > > > > > } > > > > > > > > > > > > > > > > > > /** > > > > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > > > > > > > > { > > > > > > > > > int error; > > > > > > > > > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > > > > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > > > > > > > > > Uhm, I see that the change is logically correct... but the original > > > > > > > > code was not correct, since the EVM post hook is not called (thus the > > > > > > > > HMAC is broken, or an xattr change is allowed on a portable signature > > > > > > > > which should be not). > > > > > > > > > > > > > > > > For completeness, the xattr change on a portable signature should not > > > > > > > > happen in the first place, so cap_inode_killpriv() would not be called. > > > > > > > > However, since EVM allows same value change, we are here. > > > > > > > > > > > > > > I really don't understand EVM that well and am pretty hesitant to try an > > > > > > > change any of the logic around it. But I'll hazard a thought: should EVM > > > > > > > have a inode_need_killpriv hook which returns an error in this > > > > > > > situation? > > > > > > > > > > > > Uhm, I think it would not work without modifying > > > > > > security_inode_need_killpriv() and the hook definition. > > > > > > > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would > > > > > > not be invoked. We would need to continue the loop and let EVM know > > > > > > what is the current return value. Then EVM can reject the change. > > > > > > > > > > > > An alternative way would be to detect that actually we are setting the > > > > > > same value for inode metadata, and maybe not returning 1 from > > > > > > cap_inode_need_killpriv(). > > > > > > > > > > > > I would prefer the second, since EVM allows same value change and we > > > > > > would have an exception if there are fscaps. > > > > > > > > > > > > This solves only the case of portable signatures. We would need to > > > > > > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > > > > > > files. > > > > > > > > > > I see. In any case this sounds like a matter for a separate patch > > > > > series. > > > > > > > > Agreed. > > > > > > Christian, how realistic is that we don't kill priv if we are setting > > > the same owner? > > > > Uhm, I would need to see the wider context of the proposed change. But > > iiuc then you would be comparing current and new fscaps and if they are > > identical you don't kill privs? I think that would work. But again, I > > would need to see the actual context/change to say something meaningful. > > Ok, basically a software vendor can ship binaries with a signature over > file metadata, including UID/GID, etc. > > A system can verify the signature through the public key of the > software vendor. > > The problem is if someone (or even tar), executes chown on that binary, > fscaps are lost. Thus, signature verification will fail from now on. > > EVM locks file metadata as soon as signature verification succeeds > (i.e. metadata are the same of those signed by the software vendor). > > EVM locking works if someone is trying to set different metadata. But, > if I try to chown to the same owner as the one stored in the inode, EVM > allows it but the capability LSM removes security.capability, thus > invalidating the signature. > > At least, it would be desirable that security.capability is not removed > when setting the same owner. If the owner is different, EVM will handle > that. When you say EVM "locks" file metadata, does that mean it prevents modification to file metadata? What about changes to file data? This will also result in removing fscaps xattrs. Does EVM also block changes to file data when signature verification succeeds?
On Tue, 2024-03-05 at 11:03 -0600, Seth Forshee (DigitalOcean) wrote: > On Tue, Mar 05, 2024 at 05:35:11PM +0100, Roberto Sassu wrote: > > On Tue, 2024-03-05 at 17:26 +0100, Christian Brauner wrote: > > > On Tue, Mar 05, 2024 at 01:46:56PM +0100, Roberto Sassu wrote: > > > > On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote: > > > > > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > > > > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > > > > > Use the vfs interfaces for fetching file capabilities for killpriv > > > > > > > > > > checks and from get_vfs_caps_from_disk(). While there, update the > > > > > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > > > > > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > > > > > > > --- > > > > > > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > > > > > > --- a/security/commoncap.c > > > > > > > > > > +++ b/security/commoncap.c > > > > > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > > > > > > */ > > > > > > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > > > > > > { > > > > > > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > > > > > > + struct vfs_caps caps; > > > > > > > > > > int error; > > > > > > > > > > > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > > > > > > > > > - return error > 0; > > > > > > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > > > > > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > > > > > > > > > + return error == 0; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > /** > > > > > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > > > > > > > > > { > > > > > > > > > > int error; > > > > > > > > > > > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > > > > > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > > > > > > > > > > > Uhm, I see that the change is logically correct... but the original > > > > > > > > > code was not correct, since the EVM post hook is not called (thus the > > > > > > > > > HMAC is broken, or an xattr change is allowed on a portable signature > > > > > > > > > which should be not). > > > > > > > > > > > > > > > > > > For completeness, the xattr change on a portable signature should not > > > > > > > > > happen in the first place, so cap_inode_killpriv() would not be called. > > > > > > > > > However, since EVM allows same value change, we are here. > > > > > > > > > > > > > > > > I really don't understand EVM that well and am pretty hesitant to try an > > > > > > > > change any of the logic around it. But I'll hazard a thought: should EVM > > > > > > > > have a inode_need_killpriv hook which returns an error in this > > > > > > > > situation? > > > > > > > > > > > > > > Uhm, I think it would not work without modifying > > > > > > > security_inode_need_killpriv() and the hook definition. > > > > > > > > > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would > > > > > > > not be invoked. We would need to continue the loop and let EVM know > > > > > > > what is the current return value. Then EVM can reject the change. > > > > > > > > > > > > > > An alternative way would be to detect that actually we are setting the > > > > > > > same value for inode metadata, and maybe not returning 1 from > > > > > > > cap_inode_need_killpriv(). > > > > > > > > > > > > > > I would prefer the second, since EVM allows same value change and we > > > > > > > would have an exception if there are fscaps. > > > > > > > > > > > > > > This solves only the case of portable signatures. We would need to > > > > > > > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > > > > > > > files. > > > > > > > > > > > > I see. In any case this sounds like a matter for a separate patch > > > > > > series. > > > > > > > > > > Agreed. > > > > > > > > Christian, how realistic is that we don't kill priv if we are setting > > > > the same owner? > > > > > > Uhm, I would need to see the wider context of the proposed change. But > > > iiuc then you would be comparing current and new fscaps and if they are > > > identical you don't kill privs? I think that would work. But again, I > > > would need to see the actual context/change to say something meaningful. > > > > Ok, basically a software vendor can ship binaries with a signature over > > file metadata, including UID/GID, etc. > > > > A system can verify the signature through the public key of the > > software vendor. > > > > The problem is if someone (or even tar), executes chown on that binary, > > fscaps are lost. Thus, signature verification will fail from now on. > > > > EVM locks file metadata as soon as signature verification succeeds > > (i.e. metadata are the same of those signed by the software vendor). > > > > EVM locking works if someone is trying to set different metadata. But, > > if I try to chown to the same owner as the one stored in the inode, EVM > > allows it but the capability LSM removes security.capability, thus > > invalidating the signature. > > > > At least, it would be desirable that security.capability is not removed > > when setting the same owner. If the owner is different, EVM will handle > > that. > > When you say EVM "locks" file metadata, does that mean it prevents > modification to file metadata? Yes, but only when metadata are in the final state (what the software vendor signed). Otherwise, modifications are still allowed. That was needed to let tar set everything (xattrs and attrs). > What about changes to file data? This will also result in removing > fscaps xattrs. Does EVM also block changes to file data when signature > verification succeeds? That would be the job of IMA. EVM would not be able to detect that. If the file has an EVM immutable and portable signature, IMA denies changes to it (assuming that an IMA appraisal policy was loaded, and that file matches a policy rule). Roberto
On Tue, 2024-03-05 at 13:46 +0100, Roberto Sassu wrote: > On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote: > > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) wrote: > > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > > Use the vfs interfaces for fetching file capabilities for killpriv > > > > > > > checks and from get_vfs_caps_from_disk(). While there, update the > > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > > > > --- > > > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > > > --- a/security/commoncap.c > > > > > > > +++ b/security/commoncap.c > > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > > > */ > > > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > > > { > > > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > > > + struct vfs_caps caps; > > > > > > > int error; > > > > > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > > > > > > - return error > 0; > > > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > > > > > > + return error == 0; > > > > > > > } > > > > > > > > > > > > > > /** > > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > > > > > > { > > > > > > > int error; > > > > > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > > > > > Uhm, I see that the change is logically correct... but the original > > > > > > code was not correct, since the EVM post hook is not called (thus the > > > > > > HMAC is broken, or an xattr change is allowed on a portable signature > > > > > > which should be not). > > > > > > > > > > > > For completeness, the xattr change on a portable signature should not > > > > > > happen in the first place, so cap_inode_killpriv() would not be called. > > > > > > However, since EVM allows same value change, we are here. > > > > > > > > > > I really don't understand EVM that well and am pretty hesitant to try an > > > > > change any of the logic around it. But I'll hazard a thought: should EVM > > > > > have a inode_need_killpriv hook which returns an error in this > > > > > situation? > > > > > > > > Uhm, I think it would not work without modifying > > > > security_inode_need_killpriv() and the hook definition. > > > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would > > > > not be invoked. We would need to continue the loop and let EVM know > > > > what is the current return value. Then EVM can reject the change. > > > > > > > > An alternative way would be to detect that actually we are setting the > > > > same value for inode metadata, and maybe not returning 1 from > > > > cap_inode_need_killpriv(). > > > > > > > > I would prefer the second, since EVM allows same value change and we > > > > would have an exception if there are fscaps. > > > > > > > > This solves only the case of portable signatures. We would need to > > > > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > > > > files. > > > > > > I see. In any case this sounds like a matter for a separate patch > > > series. > > > > Agreed. > > Christian, how realistic is that we don't kill priv if we are setting > the same owner? > > Serge, would we be able to replace __vfs_removexattr() (or now > vfs_get_fscaps_nosec()) with a security-equivalent alternative? It seems it is not necessary. security.capability removal occurs between evm_inode_setattr() and evm_inode_post_setattr(), after the HMAC has been verified and before the new HMAC is recalculated (without security.capability). So, all good. Christian, Seth, I pushed the kernel and the updated tests (all patches are WIP): https://github.com/robertosassu/linux/commits/evm-fscaps-v2/ https://github.com/robertosassu/ima-evm-utils/commits/evm-fscaps-v2/ The tests are passing: https://github.com/robertosassu/ima-evm-utils/actions/runs/8159877004/job/22305521359 Roberto
On Tue, Mar 05, 2024 at 06:11:45PM +0100, Roberto Sassu wrote: > On Tue, 2024-03-05 at 13:46 +0100, Roberto Sassu wrote: > > On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote: > > > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) wrote: > > > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > > > Use the vfs interfaces for fetching file capabilities for killpriv > > > > > > > > checks and from get_vfs_caps_from_disk(). While there, update the > > > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > > > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > > > > > --- > > > > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > > > > --- a/security/commoncap.c > > > > > > > > +++ b/security/commoncap.c > > > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > > > > */ > > > > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > > > > { > > > > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > > > > + struct vfs_caps caps; > > > > > > > > int error; > > > > > > > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > > > > > > > - return error > 0; > > > > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > > > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > > > > > > > + return error == 0; > > > > > > > > } > > > > > > > > > > > > > > > > /** > > > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > > > > > > > { > > > > > > > > int error; > > > > > > > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > > > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > > > > > > > Uhm, I see that the change is logically correct... but the original > > > > > > > code was not correct, since the EVM post hook is not called (thus the > > > > > > > HMAC is broken, or an xattr change is allowed on a portable signature > > > > > > > which should be not). > > > > > > > > > > > > > > For completeness, the xattr change on a portable signature should not > > > > > > > happen in the first place, so cap_inode_killpriv() would not be called. > > > > > > > However, since EVM allows same value change, we are here. > > > > > > > > > > > > I really don't understand EVM that well and am pretty hesitant to try an > > > > > > change any of the logic around it. But I'll hazard a thought: should EVM > > > > > > have a inode_need_killpriv hook which returns an error in this > > > > > > situation? > > > > > > > > > > Uhm, I think it would not work without modifying > > > > > security_inode_need_killpriv() and the hook definition. > > > > > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would > > > > > not be invoked. We would need to continue the loop and let EVM know > > > > > what is the current return value. Then EVM can reject the change. > > > > > > > > > > An alternative way would be to detect that actually we are setting the > > > > > same value for inode metadata, and maybe not returning 1 from > > > > > cap_inode_need_killpriv(). > > > > > > > > > > I would prefer the second, since EVM allows same value change and we > > > > > would have an exception if there are fscaps. > > > > > > > > > > This solves only the case of portable signatures. We would need to > > > > > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > > > > > files. > > > > > > > > I see. In any case this sounds like a matter for a separate patch > > > > series. > > > > > > Agreed. > > > > Christian, how realistic is that we don't kill priv if we are setting > > the same owner? > > > > Serge, would we be able to replace __vfs_removexattr() (or now > > vfs_get_fscaps_nosec()) with a security-equivalent alternative? > > It seems it is not necessary. > > security.capability removal occurs between evm_inode_setattr() and > evm_inode_post_setattr(), after the HMAC has been verified and before > the new HMAC is recalculated (without security.capability). > > So, all good. > > Christian, Seth, I pushed the kernel and the updated tests (all patches > are WIP): > > https://github.com/robertosassu/linux/commits/evm-fscaps-v2/ > > https://github.com/robertosassu/ima-evm-utils/commits/evm-fscaps-v2/ > > > The tests are passing: > > https://github.com/robertosassu/ima-evm-utils/actions/runs/8159877004/job/22305521359 Thanks! I probably won't be able to take them exactly as-is due to other changes for the next version (rebasing onto the changes to make IMA and EVM LSMs, forbidding xattr handlers entirely for fscaps), but they will serve as a good road map for what needs to happen.
On Tue, 2024-03-05 at 18:11 +0100, Roberto Sassu wrote: > On Tue, 2024-03-05 at 13:46 +0100, Roberto Sassu wrote: > > On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote: > > > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) > > > wrote: > > > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) > > > > > > > wrote: > > > > > > > > Use the vfs interfaces for fetching file capabilities for > > > > > > > > killpriv > > > > > > > > checks and from get_vfs_caps_from_disk(). While there, update > > > > > > > > the > > > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is > > > > > > > > different > > > > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > > > > > --- > > > > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > > > > --- a/security/commoncap.c > > > > > > > > +++ b/security/commoncap.c > > > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > > > > */ > > > > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > > > > { > > > > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > > > > + struct vfs_caps caps; > > > > > > > > int error; > > > > > > > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, > > > > > > > > NULL, 0); > > > > > > > > - return error > 0; > > > > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is > > > > > > > > unimportant */ > > > > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, > > > > > > > > &caps); > > > > > > > > + return error == 0; > > > > > > > > } > > > > > > > > > > > > > > > > /** > > > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap > > > > > > > > *idmap, struct dentry *dentry) > > > > > > > > { > > > > > > > > int error; > > > > > > > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, > > > > > > > > XATTR_NAME_CAPS); > > > > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > > > > > > > Uhm, I see that the change is logically correct... but the > > > > > > > original > > > > > > > code was not correct, since the EVM post hook is not called (thus > > > > > > > the > > > > > > > HMAC is broken, or an xattr change is allowed on a portable > > > > > > > signature > > > > > > > which should be not). > > > > > > > > > > > > > > For completeness, the xattr change on a portable signature should > > > > > > > not > > > > > > > happen in the first place, so cap_inode_killpriv() would not be > > > > > > > called. > > > > > > > However, since EVM allows same value change, we are here. > > > > > > > > > > > > I really don't understand EVM that well and am pretty hesitant to > > > > > > try an > > > > > > change any of the logic around it. But I'll hazard a thought: should > > > > > > EVM > > > > > > have a inode_need_killpriv hook which returns an error in this > > > > > > situation? > > > > > > > > > > Uhm, I think it would not work without modifying > > > > > security_inode_need_killpriv() and the hook definition. > > > > > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM > > > > > would > > > > > not be invoked. We would need to continue the loop and let EVM know > > > > > what is the current return value. Then EVM can reject the change. > > > > > > > > > > An alternative way would be to detect that actually we are setting the > > > > > same value for inode metadata, and maybe not returning 1 from > > > > > cap_inode_need_killpriv(). > > > > > > > > > > I would prefer the second, since EVM allows same value change and we > > > > > would have an exception if there are fscaps. > > > > > > > > > > This solves only the case of portable signatures. We would need to > > > > > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > > > > > files. > > > > > > > > I see. In any case this sounds like a matter for a separate patch > > > > series. > > > > > > Agreed. > > > > Christian, how realistic is that we don't kill priv if we are setting > > the same owner? > > > > Serge, would we be able to replace __vfs_removexattr() (or now > > vfs_get_fscaps_nosec()) with a security-equivalent alternative? > > It seems it is not necessary. > > security.capability removal occurs between evm_inode_setattr() and > evm_inode_post_setattr(), after the HMAC has been verified and before > the new HMAC is recalculated (without security.capability). > > So, all good. > > Christian, Seth, I pushed the kernel and the updated tests (all patches > are WIP): > > https://github.com/robertosassu/linux/commits/evm-fscaps-v2/ Resetting the IMA status flag is insufficient. The EVM status needs to be reset as well. Stefan's "ima: re-evaluate file integrity on file metadata change" patch does something similar for overlay. Mimi https://lore.kernel.org/linux-integrity/20240223172513.4049959-8-stefanb@linux.ibm.com/ > > https://github.com/robertosassu/ima-evm-utils/commits/evm-fscaps-v2/ > > > The tests are passing: > > https://github.com/robertosassu/ima-evm-utils/actions/runs/8159877004/job/22305521359 > > Roberto > >
On Tue, 2024-03-05 at 21:17 -0500, Mimi Zohar wrote: > On Tue, 2024-03-05 at 18:11 +0100, Roberto Sassu wrote: > > On Tue, 2024-03-05 at 13:46 +0100, Roberto Sassu wrote: > > > On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote: > > > > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) > > > > wrote: > > > > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > > > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) > > > > > > > > wrote: > > > > > > > > > Use the vfs interfaces for fetching file capabilities for > > > > > > > > > killpriv > > > > > > > > > checks and from get_vfs_caps_from_disk(). While there, update > > > > > > > > > the > > > > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is > > > > > > > > > different > > > > > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > > > > > > --- > > > > > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > > > > > --- a/security/commoncap.c > > > > > > > > > +++ b/security/commoncap.c > > > > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > > > > > */ > > > > > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > > > > > { > > > > > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > > > > > + struct vfs_caps caps; > > > > > > > > > int error; > > > > > > > > > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, > > > > > > > > > NULL, 0); > > > > > > > > > - return error > 0; > > > > > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is > > > > > > > > > unimportant */ > > > > > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, > > > > > > > > > &caps); > > > > > > > > > + return error == 0; > > > > > > > > > } > > > > > > > > > > > > > > > > > > /** > > > > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap > > > > > > > > > *idmap, struct dentry *dentry) > > > > > > > > > { > > > > > > > > > int error; > > > > > > > > > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, > > > > > > > > > XATTR_NAME_CAPS); > > > > > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > > > > > > > > > Uhm, I see that the change is logically correct... but the > > > > > > > > original > > > > > > > > code was not correct, since the EVM post hook is not called (thus > > > > > > > > the > > > > > > > > HMAC is broken, or an xattr change is allowed on a portable > > > > > > > > signature > > > > > > > > which should be not). > > > > > > > > > > > > > > > > For completeness, the xattr change on a portable signature should > > > > > > > > not > > > > > > > > happen in the first place, so cap_inode_killpriv() would not be > > > > > > > > called. > > > > > > > > However, since EVM allows same value change, we are here. > > > > > > > > > > > > > > I really don't understand EVM that well and am pretty hesitant to > > > > > > > try an > > > > > > > change any of the logic around it. But I'll hazard a thought: should > > > > > > > EVM > > > > > > > have a inode_need_killpriv hook which returns an error in this > > > > > > > situation? > > > > > > > > > > > > Uhm, I think it would not work without modifying > > > > > > security_inode_need_killpriv() and the hook definition. > > > > > > > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM > > > > > > would > > > > > > not be invoked. We would need to continue the loop and let EVM know > > > > > > what is the current return value. Then EVM can reject the change. > > > > > > > > > > > > An alternative way would be to detect that actually we are setting the > > > > > > same value for inode metadata, and maybe not returning 1 from > > > > > > cap_inode_need_killpriv(). > > > > > > > > > > > > I would prefer the second, since EVM allows same value change and we > > > > > > would have an exception if there are fscaps. > > > > > > > > > > > > This solves only the case of portable signatures. We would need to > > > > > > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > > > > > > files. > > > > > > > > > > I see. In any case this sounds like a matter for a separate patch > > > > > series. > > > > > > > > Agreed. > > > > > > Christian, how realistic is that we don't kill priv if we are setting > > > the same owner? > > > > > > Serge, would we be able to replace __vfs_removexattr() (or now > > > vfs_get_fscaps_nosec()) with a security-equivalent alternative? > > > > It seems it is not necessary. > > > > security.capability removal occurs between evm_inode_setattr() and > > evm_inode_post_setattr(), after the HMAC has been verified and before > > the new HMAC is recalculated (without security.capability). > > > > So, all good. > > > > Christian, Seth, I pushed the kernel and the updated tests (all patches > > are WIP): > > > > https://github.com/robertosassu/linux/commits/evm-fscaps-v2/ > > Resetting the IMA status flag is insufficient. The EVM status needs to be reset > as well. Stefan's "ima: re-evaluate file integrity on file metadata change" > patch does something similar for overlay. Both the IMA and EVM status are reset. The IMA one is reset based on the evm_revalidate_status() call, similarly to ACLs. Roberto > Mimi > > https://lore.kernel.org/linux-integrity/20240223172513.4049959-8-stefanb@linux.ibm.com/ > > > > > https://github.com/robertosassu/ima-evm-utils/commits/evm-fscaps-v2/ > > > > > > The tests are passing: > > > > https://github.com/robertosassu/ima-evm-utils/actions/runs/8159877004/job/22305521359 > > > > Roberto > > > > >
On Tue, 2024-03-05 at 14:17 -0600, Seth Forshee (DigitalOcean) wrote: > On Tue, Mar 05, 2024 at 06:11:45PM +0100, Roberto Sassu wrote: > > On Tue, 2024-03-05 at 13:46 +0100, Roberto Sassu wrote: > > > On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote: > > > > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) wrote: > > > > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > > > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote: > > > > > > > > > Use the vfs interfaces for fetching file capabilities for killpriv > > > > > > > > > checks and from get_vfs_caps_from_disk(). While there, update the > > > > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different > > > > > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > > > > > > > --- > > > > > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > > > > > --- a/security/commoncap.c > > > > > > > > > +++ b/security/commoncap.c > > > > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > > > > > */ > > > > > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > > > > > { > > > > > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > > > > > + struct vfs_caps caps; > > > > > > > > > int error; > > > > > > > > > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > > > > > > > > > - return error > 0; > > > > > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ > > > > > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); > > > > > > > > > + return error == 0; > > > > > > > > > } > > > > > > > > > > > > > > > > > > /** > > > > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) > > > > > > > > > { > > > > > > > > > int error; > > > > > > > > > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); > > > > > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > > > > > > > > > Uhm, I see that the change is logically correct... but the original > > > > > > > > code was not correct, since the EVM post hook is not called (thus the > > > > > > > > HMAC is broken, or an xattr change is allowed on a portable signature > > > > > > > > which should be not). > > > > > > > > > > > > > > > > For completeness, the xattr change on a portable signature should not > > > > > > > > happen in the first place, so cap_inode_killpriv() would not be called. > > > > > > > > However, since EVM allows same value change, we are here. > > > > > > > > > > > > > > I really don't understand EVM that well and am pretty hesitant to try an > > > > > > > change any of the logic around it. But I'll hazard a thought: should EVM > > > > > > > have a inode_need_killpriv hook which returns an error in this > > > > > > > situation? > > > > > > > > > > > > Uhm, I think it would not work without modifying > > > > > > security_inode_need_killpriv() and the hook definition. > > > > > > > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would > > > > > > not be invoked. We would need to continue the loop and let EVM know > > > > > > what is the current return value. Then EVM can reject the change. > > > > > > > > > > > > An alternative way would be to detect that actually we are setting the > > > > > > same value for inode metadata, and maybe not returning 1 from > > > > > > cap_inode_need_killpriv(). > > > > > > > > > > > > I would prefer the second, since EVM allows same value change and we > > > > > > would have an exception if there are fscaps. > > > > > > > > > > > > This solves only the case of portable signatures. We would need to > > > > > > change cap_inode_need_killpriv() anyway to update the HMAC for mutable > > > > > > files. > > > > > > > > > > I see. In any case this sounds like a matter for a separate patch > > > > > series. > > > > > > > > Agreed. > > > > > > Christian, how realistic is that we don't kill priv if we are setting > > > the same owner? > > > > > > Serge, would we be able to replace __vfs_removexattr() (or now > > > vfs_get_fscaps_nosec()) with a security-equivalent alternative? > > > > It seems it is not necessary. > > > > security.capability removal occurs between evm_inode_setattr() and > > evm_inode_post_setattr(), after the HMAC has been verified and before > > the new HMAC is recalculated (without security.capability). > > > > So, all good. > > > > Christian, Seth, I pushed the kernel and the updated tests (all patches > > are WIP): > > > > https://github.com/robertosassu/linux/commits/evm-fscaps-v2/ > > > > https://github.com/robertosassu/ima-evm-utils/commits/evm-fscaps-v2/ > > > > > > The tests are passing: > > > > https://github.com/robertosassu/ima-evm-utils/actions/runs/8159877004/job/22305521359 > > Thanks! I probably won't be able to take them exactly as-is due to other > changes for the next version (rebasing onto the changes to make IMA and > EVM LSMs, forbidding xattr handlers entirely for fscaps), but they will > serve as a good road map for what needs to happen. Welcome. Yes, both ima_inode_set_fscaps() and ima_inode_remove_fscaps() will be registered as LSM hooks in ima_appraise.c. It will be probably straightforward for you to make those changes, but if you have any question, let me know. Roberto
On Wed, 2024-03-06 at 09:25 +0100, Roberto Sassu wrote: > On Tue, 2024-03-05 at 21:17 -0500, Mimi Zohar wrote: > > On Tue, 2024-03-05 at 18:11 +0100, Roberto Sassu wrote: > > > On Tue, 2024-03-05 at 13:46 +0100, Roberto Sassu wrote: > > > > On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote: > > > > > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean) > > > > > wrote: > > > > > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote: > > > > > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) > > > > > > > wrote: > > > > > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote: > > > > > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) > > > > > > > > > wrote: > > > > > > > > > > Use the vfs interfaces for fetching file capabilities for > > > > > > > > > > killpriv > > > > > > > > > > checks and from get_vfs_caps_from_disk(). While there, > > > > > > > > > > update > > > > > > > > > > the > > > > > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how it is > > > > > > > > > > different > > > > > > > > > > from vfs_get_fscaps_nosec(). > > > > > > > > > > > > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) < > > > > > > > > > > sforshee@kernel.org> > > > > > > > > > > --- > > > > > > > > > > security/commoncap.c | 30 +++++++++++++----------------- > > > > > > > > > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > > > > > index a0ff7e6092e0..751bb26a06a6 100644 > > > > > > > > > > --- a/security/commoncap.c > > > > > > > > > > +++ b/security/commoncap.c > > > > > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, > > > > > > > > > > */ > > > > > > > > > > int cap_inode_need_killpriv(struct dentry *dentry) > > > > > > > > > > { > > > > > > > > > > - struct inode *inode = d_backing_inode(dentry); > > > > > > > > > > + struct vfs_caps caps; > > > > > > > > > > int error; > > > > > > > > > > > > > > > > > > > > - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, > > > > > > > > > > NULL, 0); > > > > > > > > > > - return error > 0; > > > > > > > > > > + /* Use nop_mnt_idmap for no mapping here as mapping is > > > > > > > > > > unimportant */ > > > > > > > > > > + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, > > > > > > > > > > &caps); > > > > > > > > > > + return error == 0; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > /** > > > > > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap > > > > > > > > > > *idmap, struct dentry *dentry) > > > > > > > > > > { > > > > > > > > > > int error; > > > > > > > > > > > > > > > > > > > > - error = __vfs_removexattr(idmap, dentry, > > > > > > > > > > XATTR_NAME_CAPS); > > > > > > > > > > + error = vfs_remove_fscaps_nosec(idmap, dentry); > > > > > > > > > > > > > > > > > > Uhm, I see that the change is logically correct... but the > > > > > > > > > original > > > > > > > > > code was not correct, since the EVM post hook is not called > > > > > > > > > (thus > > > > > > > > > the > > > > > > > > > HMAC is broken, or an xattr change is allowed on a portable > > > > > > > > > signature > > > > > > > > > which should be not). > > > > > > > > > > > > > > > > > > For completeness, the xattr change on a portable signature > > > > > > > > > should > > > > > > > > > not > > > > > > > > > happen in the first place, so cap_inode_killpriv() would not > > > > > > > > > be > > > > > > > > > called. > > > > > > > > > However, since EVM allows same value change, we are here. > > > > > > > > > > > > > > > > I really don't understand EVM that well and am pretty hesitant > > > > > > > > to > > > > > > > > try an > > > > > > > > change any of the logic around it. But I'll hazard a thought: > > > > > > > > should > > > > > > > > EVM > > > > > > > > have a inode_need_killpriv hook which returns an error in this > > > > > > > > situation? > > > > > > > > > > > > > > Uhm, I think it would not work without modifying > > > > > > > security_inode_need_killpriv() and the hook definition. > > > > > > > > > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and EVM > > > > > > > would > > > > > > > not be invoked. We would need to continue the loop and let EVM > > > > > > > know > > > > > > > what is the current return value. Then EVM can reject the change. > > > > > > > > > > > > > > An alternative way would be to detect that actually we are setting > > > > > > > the > > > > > > > same value for inode metadata, and maybe not returning 1 from > > > > > > > cap_inode_need_killpriv(). > > > > > > > > > > > > > > I would prefer the second, since EVM allows same value change and > > > > > > > we > > > > > > > would have an exception if there are fscaps. > > > > > > > > > > > > > > This solves only the case of portable signatures. We would need to > > > > > > > change cap_inode_need_killpriv() anyway to update the HMAC for > > > > > > > mutable > > > > > > > files. > > > > > > > > > > > > I see. In any case this sounds like a matter for a separate patch > > > > > > series. > > > > > > > > > > Agreed. > > > > > > > > Christian, how realistic is that we don't kill priv if we are setting > > > > the same owner? > > > > > > > > Serge, would we be able to replace __vfs_removexattr() (or now > > > > vfs_get_fscaps_nosec()) with a security-equivalent alternative? > > > > > > It seems it is not necessary. > > > > > > security.capability removal occurs between evm_inode_setattr() and > > > evm_inode_post_setattr(), after the HMAC has been verified and before > > > the new HMAC is recalculated (without security.capability). > > > > > > So, all good. > > > > > > Christian, Seth, I pushed the kernel and the updated tests (all patches > > > are WIP): > > > > > > https://github.com/robertosassu/linux/commits/evm-fscaps-v2/ > > > > Resetting the IMA status flag is insufficient. The EVM status needs to be > > reset > > as well. Stefan's "ima: re-evaluate file integrity on file metadata change" > > patch does something similar for overlay. > > Both the IMA and EVM status are reset. The IMA one is reset based on > the evm_revalidate_status() call, similarly to ACLs. Agreed. Oh, evm_status is being reset in evm_inode_post_set_fscaps(). > > > > https://lore.kernel.org/linux-integrity/20240223172513.4049959-8-stefanb@linux.ibm.com/ > > > > > https://github.com/robertosassu/ima-evm-utils/commits/evm-fscaps-v2/ > > > > > > > > > The tests are passing: > > > > > > https://github.com/robertosassu/ima-evm-utils/actions/runs/8159877004/job/22305521359 > > > > > > Roberto > > > > > > > >
diff --git a/security/commoncap.c b/security/commoncap.c index a0ff7e6092e0..751bb26a06a6 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -296,11 +296,12 @@ int cap_capset(struct cred *new, */ int cap_inode_need_killpriv(struct dentry *dentry) { - struct inode *inode = d_backing_inode(dentry); + struct vfs_caps caps; int error; - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); - return error > 0; + /* Use nop_mnt_idmap for no mapping here as mapping is unimportant */ + error = vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps); + return error == 0; } /** @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry) { int error; - error = __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS); + error = vfs_remove_fscaps_nosec(idmap, dentry); if (error == -EOPNOTSUPP) error = 0; return error; @@ -719,6 +720,10 @@ ssize_t vfs_caps_to_user_xattr(struct mnt_idmap *idmap, * @cpu_caps: vfs capabilities * * Extract the on-exec-apply capability sets for an executable file. + * For version 3 capabilities xattrs, returns the capabilities only if + * they are applicable to current_user_ns() (i.e. that the rootid + * corresponds to an ID which maps to ID 0 in current_user_ns() or an + * ancestor), and returns -ENODATA otherwise. * * If the inode has been found through an idmapped mount the idmap of * the vfsmount must be passed through @idmap. This function will then @@ -731,25 +736,16 @@ int get_vfs_caps_from_disk(struct mnt_idmap *idmap, struct vfs_caps *cpu_caps) { struct inode *inode = d_backing_inode(dentry); - int size, ret; - struct vfs_ns_cap_data data, *nscaps = &data; + int ret; if (!inode) return -ENODATA; - size = __vfs_getxattr((struct dentry *)dentry, inode, - XATTR_NAME_CAPS, &data, XATTR_CAPS_SZ); - if (size == -ENODATA || size == -EOPNOTSUPP) + ret = vfs_get_fscaps_nosec(idmap, (struct dentry *)dentry, cpu_caps); + if (ret == -EOPNOTSUPP || ret == -EOVERFLOW) /* no data, that's ok */ - return -ENODATA; + ret = -ENODATA; - if (size < 0) - return size; - - ret = vfs_caps_from_xattr(idmap, inode->i_sb->s_user_ns, - cpu_caps, nscaps, size); - if (ret == -EOVERFLOW) - return -ENODATA; if (ret) return ret;
Use the vfs interfaces for fetching file capabilities for killpriv checks and from get_vfs_caps_from_disk(). While there, update the kerneldoc for get_vfs_caps_from_disk() to explain how it is different from vfs_get_fscaps_nosec(). Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> --- security/commoncap.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)