diff mbox series

[v2,24/25] commoncap: use vfs fscaps interfaces

Message ID 20240221-idmap-fscap-refactor-v2-24-3039364623bd@kernel.org
State New
Headers
Series fs: use type-safe uid representation for filesystem capabilities |

Commit Message

Seth Forshee (DigitalOcean) Feb. 21, 2024, 9:24 p.m. UTC 
  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(-)

Comments

Roberto Sassu March 4, 2024, 10:19 a.m. UTC | #1 
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;
>  
>
Seth Forshee (DigitalOcean) March 4, 2024, 3:31 p.m. UTC | #2 
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?
Roberto Sassu March 4, 2024, 4:17 p.m. UTC | #3 
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
Seth Forshee (DigitalOcean) March 4, 2024, 4:56 p.m. UTC | #4 
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.
diff mbox series

Patch

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;