| Message ID | 20210616132227.999256-1-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | fs: Return raw xattr for security.* if there is size disagreement with LSMs | expand |
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:
> 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:
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
> 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
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.
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
> 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
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.
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
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:
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(+)