Hello
the ima-evm-utils tool discovered an issue doing signature
verification of xattrs.
On kernel side, EVM reads the xattr value with
vfs_getxattr_alloc(), which gets the value directly from the
xattr handler.
On user side, ima-evm-utils reads the value with the
lgetxattr() system call, which gets the value from LSMs.
There is a corner case, where security.selinux is set directly
with setfattr without adding \0 at the end.
In this case, the kernel and the user see different values
due to the fact that the former gets the raw value from the
xattr handler, and the latter gets the value normalized by
SELinux (which adds \0).
I found that originally also lgetxattr() was getting the value
from the xattr handler. This changed with:
commit 4bea58053f206be9a89ca35850f9ad295dac2042
Author: David P. Quigley <[email protected]>
Date: Mon Feb 4 22:29:40 2008 -0800
VFS: Reorder vfs_getxattr to avoid unnecessary calls to the LSM
which directly calls LSMs for security.* xattrs.
Given that this patch is there for a long time, I would ask
if it makes sense to fix this issue. The way I would do it
is to check if the size returned by the xattr handler is the
same of the size returned by LSMs. If not, I would get
the value from the xattr handler.
Although this change does not check the xattr content,
it is sufficient to fix the issue.
Any opinion?
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
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 <[email protected]>
Tested-by: Mimi Zohar <[email protected]>
---
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
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 <[email protected]>
> Tested-by: Mimi Zohar <[email protected]>
> ---
> 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:
> From: Stefan Berger [mailto:[email protected]]
> 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 <[email protected]>
> > Tested-by: Mimi Zohar <[email protected]>
> > ---
> > 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:
On Thu, 2021-06-17 at 07:09 +0000, Roberto Sassu wrote:
> > From: Stefan Berger [mailto:[email protected]]
> > 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
> From: Mimi Zohar [mailto:[email protected]]
> Sent: Thursday, June 17, 2021 5:28 PM
> On Thu, 2021-06-17 at 07:09 +0000, Roberto Sassu wrote:
> > > From: Stefan Berger [mailto:[email protected]]
> > > 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
On Thu, Jun 17, 2021 at 11:28 AM Mimi Zohar <[email protected]> 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
http://www.paul-moore.com
On Fri, Jun 18, 2021 at 12:04 PM Mimi Zohar <[email protected]> wrote:
> On Thu, 2021-06-17 at 23:18 -0400, Paul Moore wrote:
> > On Thu, Jun 17, 2021 at 11:28 AM Mimi Zohar <[email protected]> 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
http://www.paul-moore.com
> From: Mimi Zohar [mailto:[email protected]]
> 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 <[email protected]>
> 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
On Fri, 2021-06-18 at 12:35 -0400, Paul Moore wrote:
> On Fri, Jun 18, 2021 at 12:04 PM Mimi Zohar <[email protected]> wrote:
> > On Thu, 2021-06-17 at 23:18 -0400, Paul Moore wrote:
> > > On Thu, Jun 17, 2021 at 11:28 AM Mimi Zohar <[email protected]> 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
On Thu, 2021-06-17 at 23:18 -0400, Paul Moore wrote:
> On Thu, Jun 17, 2021 at 11:28 AM Mimi Zohar <[email protected]> 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