On Tue, 2024-03-05 at 18:11 +0100, Roberto Sassu wrote:

On Tue, 2024-03-05 at 13:46 +0100, Roberto Sassu wrote:

quoted

On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote:

quoted

On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOcean)
wrote:

quoted

On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote:

quoted

On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote:

quoted

On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote:

quoted

On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean)
wrote:

quoted

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/ (local)

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