[PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

[PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

[Roberto Sassu]

vfs_getxattr() differs from vfs_setxattr() in the way it obtains the xattr
value. The former gives precedence to the LSMs, and if the LSMs don't
provide a value, obtains it from the filesystem handler. The latter does
the opposite, first invokes the filesystem handler, and if the filesystem
does not support xattrs, passes the xattr value to the LSMs.

The problem is that not necessarily the user gets the same xattr value that
he set. For example, if he sets security.selinux with a value not
terminated with '\0', he gets a value terminated with '\0' because SELinux
adds it during the translation from xattr to internal representation
(vfs_setxattr()) and from internal representation to xattr
(vfs_getxattr()).

Normally, this does not have an impact unless the integrity of xattrs is
verified with EVM. The kernel and the user see different values due to the
different functions used to obtain them:

kernel (EVM): uses vfs_getxattr_alloc() which obtains the xattr value from
              the filesystem handler (raw value);

user (ima-evm-utils): uses vfs_getxattr() which obtains the xattr value
                      from the LSMs (normalized value).

Given that the difference between the raw value and the normalized value
should be just the additional '\0' not the rest of the content, this patch
modifies vfs_getxattr() to compare the size of the xattr value obtained
from the LSMs to the size of the raw xattr value. If there is a mismatch
and the filesystem handler does not return an error, vfs_getxattr() returns
the raw value.

This patch should have a minimal impact on existing systems, because if the
SELinux label is written with the appropriate tools such as setfiles or
restorecon, there will not be a mismatch (because the raw value also has
'\0').

In the case where the SELinux label is written directly with setfattr and
without '\0', this patch helps to align EVM and ima-evm-utils in terms of
result provided (due to the fact that they both verify the integrity of
xattrs from raw values).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Tested-by: Mimi Zohar <zohar@linux.ibm.com>
---
 fs/xattr.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/fs/xattr.c b/fs/xattr.c
index 5c8c5175b385..412ec875aa07 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -420,12 +420,27 @@ vfs_getxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
 		int ret = xattr_getsecurity(mnt_userns, inode, suffix, value,
 					    size);
+		int ret_raw;
+
 		/*
 		 * Only overwrite the return value if a security module
 		 * is actually active.
 		 */
 		if (ret == -EOPNOTSUPP)
 			goto nolsm;
+
+		if (ret < 0)
+			return ret;
+
+		/*
+		 * Read raw xattr if the size from the filesystem handler
+		 * differs from that returned by xattr_getsecurity() and is
+		 * equal or greater than zero.
+		 */
+		ret_raw = __vfs_getxattr(dentry, inode, name, NULL, 0);
+		if (ret_raw >= 0 && ret_raw != ret)
+			goto nolsm;
+
 		return ret;
 	}
 nolsm:
-- 
2.25.1

Re: [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

[Stefan Berger]

On 6/16/21 9:22 AM, Roberto Sassu wrote:
> vfs_getxattr() differs from vfs_setxattr() in the way it obtains the xattr
> value. The former gives precedence to the LSMs, and if the LSMs don't
> provide a value, obtains it from the filesystem handler. The latter does
> the opposite, first invokes the filesystem handler, and if the filesystem
> does not support xattrs, passes the xattr value to the LSMs.
>
> The problem is that not necessarily the user gets the same xattr value that
> he set. For example, if he sets security.selinux with a value not
> terminated with '\0', he gets a value terminated with '\0' because SELinux
> adds it during the translation from xattr to internal representation
> (vfs_setxattr()) and from internal representation to xattr
> (vfs_getxattr()).
>
> Normally, this does not have an impact unless the integrity of xattrs is
> verified with EVM. The kernel and the user see different values due to the
> different functions used to obtain them:
>
> kernel (EVM): uses vfs_getxattr_alloc() which obtains the xattr value from
>                the filesystem handler (raw value);
>
> user (ima-evm-utils): uses vfs_getxattr() which obtains the xattr value
>                        from the LSMs (normalized value).

Maybe there should be another implementation similar to 
vfs_getxattr_alloc() (or modify it) to behave like vfs_getxattr() but do 
the memory allocation part so that the kernel sees what user space see 
rather than modifying it with your patch so that user space now sees 
something different than what it has been for years (previous 
NUL-terminated SELinux xattr may not be NUL-terminated anymore)?

     Stefan




>
> Given that the difference between the raw value and the normalized value
> should be just the additional '\0' not the rest of the content, this patch
> modifies vfs_getxattr() to compare the size of the xattr value obtained
> from the LSMs to the size of the raw xattr value. If there is a mismatch
> and the filesystem handler does not return an error, vfs_getxattr() returns
> the raw value.
>
> This patch should have a minimal impact on existing systems, because if the
> SELinux label is written with the appropriate tools such as setfiles or
> restorecon, there will not be a mismatch (because the raw value also has
> '\0').
>
> In the case where the SELinux label is written directly with setfattr and
> without '\0', this patch helps to align EVM and ima-evm-utils in terms of
> result provided (due to the fact that they both verify the integrity of
> xattrs from raw values).
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Tested-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   fs/xattr.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 5c8c5175b385..412ec875aa07 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -420,12 +420,27 @@ vfs_getxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>   		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
>   		int ret = xattr_getsecurity(mnt_userns, inode, suffix, value,
>   					    size);
> +		int ret_raw;
> +
>   		/*
>   		 * Only overwrite the return value if a security module
>   		 * is actually active.
>   		 */
>   		if (ret == -EOPNOTSUPP)
>   			goto nolsm;
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		/*
> +		 * Read raw xattr if the size from the filesystem handler
> +		 * differs from that returned by xattr_getsecurity() and is
> +		 * equal or greater than zero.
> +		 */
> +		ret_raw = __vfs_getxattr(dentry, inode, name, NULL, 0);
> +		if (ret_raw >= 0 && ret_raw != ret)
> +			goto nolsm;
> +
>   		return ret;
>   	}
>   nolsm:

RE: [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

[Roberto Sassu]

> From: Stefan Berger [mailto:stefanb@linux.ibm.com]
> Sent: Wednesday, June 16, 2021 4:40 PM
> On 6/16/21 9:22 AM, Roberto Sassu wrote:
> > vfs_getxattr() differs from vfs_setxattr() in the way it obtains the xattr
> > value. The former gives precedence to the LSMs, and if the LSMs don't
> > provide a value, obtains it from the filesystem handler. The latter does
> > the opposite, first invokes the filesystem handler, and if the filesystem
> > does not support xattrs, passes the xattr value to the LSMs.
> >
> > The problem is that not necessarily the user gets the same xattr value that
> > he set. For example, if he sets security.selinux with a value not
> > terminated with '\0', he gets a value terminated with '\0' because SELinux
> > adds it during the translation from xattr to internal representation
> > (vfs_setxattr()) and from internal representation to xattr
> > (vfs_getxattr()).
> >
> > Normally, this does not have an impact unless the integrity of xattrs is
> > verified with EVM. The kernel and the user see different values due to the
> > different functions used to obtain them:
> >
> > kernel (EVM): uses vfs_getxattr_alloc() which obtains the xattr value from
> >                the filesystem handler (raw value);
> >
> > user (ima-evm-utils): uses vfs_getxattr() which obtains the xattr value
> >                        from the LSMs (normalized value).
> 
> Maybe there should be another implementation similar to
> vfs_getxattr_alloc() (or modify it) to behave like vfs_getxattr() but do
> the memory allocation part so that the kernel sees what user space see
> rather than modifying it with your patch so that user space now sees
> something different than what it has been for years (previous
> NUL-terminated SELinux xattr may not be NUL-terminated anymore)?

I'm concerned that this would break HMACs/digital signatures
calculated with raw values.

An alternative would be to do the EVM verification twice if the
first time didn't succeed (with vfs_getxattr_alloc() and with the
new function that behaves like vfs_getxattr()).

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

>      Stefan
> 
> 
> 
> 
> >
> > Given that the difference between the raw value and the normalized value
> > should be just the additional '\0' not the rest of the content, this patch
> > modifies vfs_getxattr() to compare the size of the xattr value obtained
> > from the LSMs to the size of the raw xattr value. If there is a mismatch
> > and the filesystem handler does not return an error, vfs_getxattr() returns
> > the raw value.
> >
> > This patch should have a minimal impact on existing systems, because if the
> > SELinux label is written with the appropriate tools such as setfiles or
> > restorecon, there will not be a mismatch (because the raw value also has
> > '\0').
> >
> > In the case where the SELinux label is written directly with setfattr and
> > without '\0', this patch helps to align EVM and ima-evm-utils in terms of
> > result provided (due to the fact that they both verify the integrity of
> > xattrs from raw values).
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Tested-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >   fs/xattr.c | 15 +++++++++++++++
> >   1 file changed, 15 insertions(+)
> >
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index 5c8c5175b385..412ec875aa07 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -420,12 +420,27 @@ vfs_getxattr(struct user_namespace *mnt_userns,
> struct dentry *dentry,
> >   		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> >   		int ret = xattr_getsecurity(mnt_userns, inode, suffix, value,
> >   					    size);
> > +		int ret_raw;
> > +
> >   		/*
> >   		 * Only overwrite the return value if a security module
> >   		 * is actually active.
> >   		 */
> >   		if (ret == -EOPNOTSUPP)
> >   			goto nolsm;
> > +
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/*
> > +		 * Read raw xattr if the size from the filesystem handler
> > +		 * differs from that returned by xattr_getsecurity() and is
> > +		 * equal or greater than zero.
> > +		 */
> > +		ret_raw = __vfs_getxattr(dentry, inode, name, NULL, 0);
> > +		if (ret_raw >= 0 && ret_raw != ret)
> > +			goto nolsm;
> > +
> >   		return ret;
> >   	}
> >   nolsm:

Re: [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

[Mimi Zohar]

On Thu, 2021-06-17 at 07:09 +0000, Roberto Sassu wrote:
> > From: Stefan Berger [mailto:stefanb@linux.ibm.com]
> > Sent: Wednesday, June 16, 2021 4:40 PM
> > On 6/16/21 9:22 AM, Roberto Sassu wrote:
> > > vfs_getxattr() differs from vfs_setxattr() in the way it obtains the xattr
> > > value. The former gives precedence to the LSMs, and if the LSMs don't
> > > provide a value, obtains it from the filesystem handler. The latter does
> > > the opposite, first invokes the filesystem handler, and if the filesystem
> > > does not support xattrs, passes the xattr value to the LSMs.
> > >
> > > The problem is that not necessarily the user gets the same xattr value that
> > > he set. For example, if he sets security.selinux with a value not
> > > terminated with '\0', he gets a value terminated with '\0' because SELinux
> > > adds it during the translation from xattr to internal representation
> > > (vfs_setxattr()) and from internal representation to xattr
> > > (vfs_getxattr()).
> > >
> > > Normally, this does not have an impact unless the integrity of xattrs is
> > > verified with EVM. The kernel and the user see different values due to the
> > > different functions used to obtain them:
> > >
> > > kernel (EVM): uses vfs_getxattr_alloc() which obtains the xattr value from
> > >                the filesystem handler (raw value);
> > >
> > > user (ima-evm-utils): uses vfs_getxattr() which obtains the xattr value
> > >                        from the LSMs (normalized value).
> > 
> > Maybe there should be another implementation similar to
> > vfs_getxattr_alloc() (or modify it) to behave like vfs_getxattr() but do
> > the memory allocation part so that the kernel sees what user space see
> > rather than modifying it with your patch so that user space now sees
> > something different than what it has been for years (previous
> > NUL-terminated SELinux xattr may not be NUL-terminated anymore)?
> 
> I'm concerned that this would break HMACs/digital signatures
> calculated with raw values.

Which would happen if the LSM is not enabled (e.g. "lsm=" boot command
line option).

> 
> An alternative would be to do the EVM verification twice if the
> first time didn't succeed (with vfs_getxattr_alloc() and with the
> new function that behaves like vfs_getxattr()).

Unfortunately, I don't see an alternative.

thanks,

Mimi

RE: [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, June 17, 2021 5:28 PM
> On Thu, 2021-06-17 at 07:09 +0000, Roberto Sassu wrote:
> > > From: Stefan Berger [mailto:stefanb@linux.ibm.com]
> > > Sent: Wednesday, June 16, 2021 4:40 PM
> > > On 6/16/21 9:22 AM, Roberto Sassu wrote:
> > > > vfs_getxattr() differs from vfs_setxattr() in the way it obtains the xattr
> > > > value. The former gives precedence to the LSMs, and if the LSMs don't
> > > > provide a value, obtains it from the filesystem handler. The latter does
> > > > the opposite, first invokes the filesystem handler, and if the filesystem
> > > > does not support xattrs, passes the xattr value to the LSMs.
> > > >
> > > > The problem is that not necessarily the user gets the same xattr value
> that
> > > > he set. For example, if he sets security.selinux with a value not
> > > > terminated with '\0', he gets a value terminated with '\0' because
> SELinux
> > > > adds it during the translation from xattr to internal representation
> > > > (vfs_setxattr()) and from internal representation to xattr
> > > > (vfs_getxattr()).
> > > >
> > > > Normally, this does not have an impact unless the integrity of xattrs is
> > > > verified with EVM. The kernel and the user see different values due to
> the
> > > > different functions used to obtain them:
> > > >
> > > > kernel (EVM): uses vfs_getxattr_alloc() which obtains the xattr value
> from
> > > >                the filesystem handler (raw value);
> > > >
> > > > user (ima-evm-utils): uses vfs_getxattr() which obtains the xattr value
> > > >                        from the LSMs (normalized value).
> > >
> > > Maybe there should be another implementation similar to
> > > vfs_getxattr_alloc() (or modify it) to behave like vfs_getxattr() but do
> > > the memory allocation part so that the kernel sees what user space see
> > > rather than modifying it with your patch so that user space now sees
> > > something different than what it has been for years (previous
> > > NUL-terminated SELinux xattr may not be NUL-terminated anymore)?
> >
> > I'm concerned that this would break HMACs/digital signatures
> > calculated with raw values.
> 
> Which would happen if the LSM is not enabled (e.g. "lsm=" boot command
> line option).

For files created after switching to the new behavior, yes, because
EVM could eventually get the label without '\0' from the filesystem
handler.

However, it would happen also for files created before switching to
the new behavior, since the HMAC could have been calculated without
'\0' and after switching it would be calculated with '\0'.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> > An alternative would be to do the EVM verification twice if the
> > first time didn't succeed (with vfs_getxattr_alloc() and with the
> > new function that behaves like vfs_getxattr()).
> 
> Unfortunately, I don't see an alternative.
> 
> thanks,
> 
> Mimi

Re: [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

[Paul Moore]

On Thu, Jun 17, 2021 at 11:28 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Thu, 2021-06-17 at 07:09 +0000, Roberto Sassu wrote:

...

> > An alternative would be to do the EVM verification twice if the
> > first time didn't succeed (with vfs_getxattr_alloc() and with the
> > new function that behaves like vfs_getxattr()).
>
> Unfortunately, I don't see an alternative.

... and while unfortunate, the impact should be non-existant if you
are using the right tools to label files or ensuring that you are
formatting labels properly if doing it by hand.

Handling a corner case is good, but I wouldn't add a lot of code
complexity trying to optimize it.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

[Mimi Zohar]

On Thu, 2021-06-17 at 23:18 -0400, Paul Moore wrote:
> On Thu, Jun 17, 2021 at 11:28 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Thu, 2021-06-17 at 07:09 +0000, Roberto Sassu wrote:
> 
> ...
> 
> > > An alternative would be to do the EVM verification twice if the
> > > first time didn't succeed (with vfs_getxattr_alloc() and with the
> > > new function that behaves like vfs_getxattr()).
> >
> > Unfortunately, I don't see an alternative.
> 
> ... and while unfortunate, the impact should be non-existant if you
> are using the right tools to label files or ensuring that you are
> formatting labels properly if doing it by hand.
> 
> Handling a corner case is good, but I wouldn't add a lot of code
> complexity trying to optimize it.

From userspace it's really difficult to understand the EVM signature
verification failure is due to the missing NULL.

Roberto, I just pushed the "evm: output EVM digest calculation info"
patch to the next-integrity-testing branch, which includes some
debugging.   Instead of this patch, which returns the raw xattr data,
how about adding additional debugging info in evm_calc_hmac_or_hash()
indicating the size discrepancy between the raw xattr and the LSM
returned xattr?

thanks,

Mimi

RE: [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, June 18, 2021 6:04 PM
> On Thu, 2021-06-17 at 23:18 -0400, Paul Moore wrote:
> > On Thu, Jun 17, 2021 at 11:28 AM Mimi Zohar <zohar@linux.ibm.com>
> wrote:
> > > On Thu, 2021-06-17 at 07:09 +0000, Roberto Sassu wrote:
> >
> > ...
> >
> > > > An alternative would be to do the EVM verification twice if the
> > > > first time didn't succeed (with vfs_getxattr_alloc() and with the
> > > > new function that behaves like vfs_getxattr()).
> > >
> > > Unfortunately, I don't see an alternative.
> >
> > ... and while unfortunate, the impact should be non-existant if you
> > are using the right tools to label files or ensuring that you are
> > formatting labels properly if doing it by hand.
> >
> > Handling a corner case is good, but I wouldn't add a lot of code
> > complexity trying to optimize it.
> 
> From userspace it's really difficult to understand the EVM signature
> verification failure is due to the missing NULL.
> 
> Roberto, I just pushed the "evm: output EVM digest calculation info"
> patch to the next-integrity-testing branch, which includes some
> debugging.   Instead of this patch, which returns the raw xattr data,
> how about adding additional debugging info in evm_calc_hmac_or_hash()
> indicating the size discrepancy between the raw xattr and the LSM
> returned xattr?

Good idea. Will do it.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> thanks,
> 
> Mimi

Re: [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

[Paul Moore]

On Fri, Jun 18, 2021 at 12:04 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Thu, 2021-06-17 at 23:18 -0400, Paul Moore wrote:
> > On Thu, Jun 17, 2021 at 11:28 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > On Thu, 2021-06-17 at 07:09 +0000, Roberto Sassu wrote:
> >
> > ...
> >
> > > > An alternative would be to do the EVM verification twice if the
> > > > first time didn't succeed (with vfs_getxattr_alloc() and with the
> > > > new function that behaves like vfs_getxattr()).
> > >
> > > Unfortunately, I don't see an alternative.
> >
> > ... and while unfortunate, the impact should be non-existant if you
> > are using the right tools to label files or ensuring that you are
> > formatting labels properly if doing it by hand.
> >
> > Handling a corner case is good, but I wouldn't add a lot of code
> > complexity trying to optimize it.
>
> From userspace it's really difficult to understand the EVM signature
> verification failure is due to the missing NULL.

I would argue that any signature verification failure, regardless of
the mechanism, is hard to understand.  It either passes or it fails,
and if it fails good luck trying to determine what exactly isn't
matching up; especially if you really don't know the Right Value.

What I mean by the corner case was the fact that the recommended tools
should always do the right thing with respect to '\0' termination,
this should really only be an issue if someone is winging it and doing
it by hand or with their own tools.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

[Mimi Zohar]

On Fri, 2021-06-18 at 12:35 -0400, Paul Moore wrote:
> On Fri, Jun 18, 2021 at 12:04 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Thu, 2021-06-17 at 23:18 -0400, Paul Moore wrote:
> > > On Thu, Jun 17, 2021 at 11:28 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > On Thu, 2021-06-17 at 07:09 +0000, Roberto Sassu wrote:
> > >
> > > ...
> > >
> > > > > An alternative would be to do the EVM verification twice if the
> > > > > first time didn't succeed (with vfs_getxattr_alloc() and with the
> > > > > new function that behaves like vfs_getxattr()).
> > > >
> > > > Unfortunately, I don't see an alternative.
> > >
> > > ... and while unfortunate, the impact should be non-existant if you
> > > are using the right tools to label files or ensuring that you are
> > > formatting labels properly if doing it by hand.
> > >
> > > Handling a corner case is good, but I wouldn't add a lot of code
> > > complexity trying to optimize it.
> >
> > From userspace it's really difficult to understand the EVM signature
> > verification failure is due to the missing NULL.
> 
> I would argue that any signature verification failure, regardless of
> the mechanism, is hard to understand.  It either passes or it fails,
> and if it fails good luck trying to determine what exactly isn't
> matching up; especially if you really don't know the Right Value.

In this case, the discussion is about signing and verifying file meta-
data hashes.  With EVM portable and immutable signatures, the file
meta-data is known.  The userspace tool evmct is able to verify the
file meta-data signature, which the kernel rejects.

> What I mean by the corner case was the fact that the recommended tools
> should always do the right thing with respect to '\0' termination,
> this should really only be an issue if someone is winging it and doing
> it by hand or with their own tools.

I'm not disagreeing with you.  However, it's still annoying, confusing,
and really frustrating.   That's why we're at least including debugging
information.  In addtion, Roberto will provide the reason.

thanks,

Mimi