| Message ID | 20200429073935.11913-1-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/3] evm: Move hooks outside LSM infrastructure | expand |
On Wed, Apr 29, 2020 at 10:45 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > EVM is a module for the protection of the integrity of file metadata. It > protects security-relevant extended attributes, and some file attributes > such as the UID and the GID. It protects their integrity with an HMAC or > with a signature. > > What makes EVM different from other LSMs is that it makes a security > decision depending on multiple pieces of information, which cannot be > managed atomically by the system. > > Example: cp -a file.orig file.dest > > If security.selinux, security.ima and security.evm must be preserved, cp > will invoke setxattr() for each xattr, and EVM performs a verification > during each operation. The problem is that copying security.evm from > file.orig to file.dest will likely break the following EVM verifications if > some metadata still have to be copied. EVM has no visibility on the > metadata of the source file, so it cannot determine when the copy can be > considered complete. > > On the other hand, EVM has to check metadata during every operation to > ensure that there is no transition from corrupted metadata, e.g. after an > offline attack, to valid ones after the operation. An HMAC update would > prevent the corruption to be detected, as the HMAC on the new values would > be correct. Thus, to avoid this issue, EVM has to return an error to the > system call so that its execution will be interrupted. > > A solution that would satisfy both requirements, not breaking user space > applications and detecting corrupted metadata is to let metadata operations > be completed successfully and to pass the result of the EVM verification > from the pre hooks to the post hooks. In this way, the HMAC update can be > avoided if the verification wasn't successful. > > This approach will bring another important benefit: it is no longer > required that every file has a valid HMAC or signature. Instead of always > enforcing metadata integrity, even when it is not relevant for IMA, EVM > will let IMA decide for files selected with the appraisal policy, > depending on the result of the requested verification. > > The main problem is that the result of the verification currently cannot be > passed from the pre hooks to the post hooks, due to how the LSM API is > defined. A possible solution would be to use integrity_iint_cache for this > purpose, but it will increase the memory pressure, as new structures will > be allocated also for metadata operations, not only for measurement, > appraisal and audit. Another solution would be to extend the LSM API, but > it seems not worthwhile as EVM would be the only module getting a benefit > from this change. > > Given that pre and post hooks are called from the same system call, a more > efficient solution seems to move the hooks outside the LSM infrastructure, > so that the return value of the pre hooks can be passed to the post hooks. > A predefined error (-EAGAIN) will be used to signal to the system call to > continue the execution. Otherwise, if the pre hooks return -EPERM, the > system calls will behave as before and will immediately return before > metadata are changed. > > Overview of the changes: > > evm_inode_init_security() LSM (no change) > evm_inode_setxattr() LSM -> vfs_setxattr() > evm_inode_post_setxattr() LSM -> vfs_setxattr() > evm_inode_removexattr() LSM -> vfs_removexattr() > evm_inode_post_removexattr() vfs_removexattr() (no change) > evm_inode_setattr() LSM -> vfs_setattr() > evm_inode_post_setattr() vfs_setattr() (no change) > evm_verifyxattr() outside LSM (no change) > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > fs/attr.c | 5 ++++- > fs/xattr.c | 17 +++++++++++++++-- > security/security.c | 18 +++--------------- > 3 files changed, 22 insertions(+), 18 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index b4bbdbd4c8ca..8f26d7d2e3b4 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -224,7 +224,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de > { > struct inode *inode = dentry->d_inode; > umode_t mode = inode->i_mode; > - int error; > + int error, evm_error; > struct timespec64 now; > unsigned int ia_valid = attr->ia_valid; > > @@ -328,6 +328,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de > error = security_inode_setattr(dentry, attr); > if (error) > return error; > + evm_error = evm_inode_setattr(dentry, attr); > + if (evm_error) > + return evm_error; > error = try_break_deleg(inode, delegated_inode); > if (error) > return error; > diff --git a/fs/xattr.c b/fs/xattr.c > index e13265e65871..3b323b75b741 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -183,6 +183,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name, > fsnotify_xattr(dentry); > security_inode_post_setxattr(dentry, name, value, > size, flags); > + evm_inode_post_setxattr(dentry, name, value, size); > } > } else { > if (unlikely(is_bad_inode(inode))) > @@ -210,7 +211,7 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value, > size_t size, int flags) > { > struct inode *inode = dentry->d_inode; > - int error; > + int error, evm_error; > > error = xattr_permission(inode, name, MAY_WRITE); > if (error) > @@ -221,6 +222,12 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value, > if (error) > goto out; > > + evm_error = evm_inode_setxattr(dentry, name, value, size); > + if (evm_error) { > + error = evm_error; > + goto out; > + } > + > error = __vfs_setxattr_noperm(dentry, name, value, size, flags); > > out: > @@ -382,7 +389,7 @@ int > vfs_removexattr(struct dentry *dentry, const char *name) > { > struct inode *inode = dentry->d_inode; > - int error; > + int error, evm_error; > > error = xattr_permission(inode, name, MAY_WRITE); > if (error) > @@ -393,6 +400,12 @@ vfs_removexattr(struct dentry *dentry, const char *name) > if (error) > goto out; > > + evm_error = evm_inode_removexattr(dentry, name); > + if (evm_error) { > + error = evm_error; > + goto out; > + } > + > error = __vfs_removexattr(dentry, name); > > if (!error) { > diff --git a/security/security.c b/security/security.c > index 7fed24b9d57e..e1368ab34cee 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1255,14 +1255,9 @@ int security_inode_permission(struct inode *inode, int mask) > > int security_inode_setattr(struct dentry *dentry, struct iattr *attr) > { > - int ret; > - > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return 0; > - ret = call_int_hook(inode_setattr, 0, dentry, attr); > - if (ret) > - return ret; > - return evm_inode_setattr(dentry, attr); > + return call_int_hook(inode_setattr, 0, dentry, attr); > } > EXPORT_SYMBOL_GPL(security_inode_setattr); > > @@ -1291,10 +1286,7 @@ int security_inode_setxattr(struct dentry *dentry, const char *name, > ret = cap_inode_setxattr(dentry, name, value, size, flags); > if (ret) > return ret; > - ret = ima_inode_setxattr(dentry, name, value, size); > - if (ret) > - return ret; > - return evm_inode_setxattr(dentry, name, value, size); > + return ima_inode_setxattr(dentry, name, value, size); > } > > void security_inode_post_setxattr(struct dentry *dentry, const char *name, > @@ -1303,7 +1295,6 @@ void security_inode_post_setxattr(struct dentry *dentry, const char *name, > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return; > call_void_hook(inode_post_setxattr, dentry, name, value, size, flags); > - evm_inode_post_setxattr(dentry, name, value, size); > } > > int security_inode_getxattr(struct dentry *dentry, const char *name) > @@ -1335,10 +1326,7 @@ int security_inode_removexattr(struct dentry *dentry, const char *name) > ret = cap_inode_removexattr(dentry, name); > if (ret) > return ret; > - ret = ima_inode_removexattr(dentry, name); > - if (ret) > - return ret; > - return evm_inode_removexattr(dentry, name); > + return ima_inode_removexattr(dentry, name); > } > > int security_inode_need_killpriv(struct dentry *dentry) > -- > 2.17.1 > Hi Roberto, I apologize that due to my relatively small experience I may not understand completely your patch. Please be patient. To my understanding, EVM knows(configuration) which security attributes must be taken into HMAC, security.selinux, security.smack, system.acl, security.ima, etc. So until HMAC calculation should fail until cp command will not finish copying all security attributes. Is it the desired behavior of HMAC? If the number of security attributes varies between files under EVM, can we just remove evm_inode_setxattr() from secutity_inode_setxattr() because it is the job of evmctl utility to compute security.hmac value? Actually I do not see the use case why HMAC should be computed inline. I come from embedded word and will never modify extended attributes because the signing key will never be placed on the target device. I think this argument stands also for production servers and for package management as well. Regards, Lev
> -----Original Message----- > From: Lev R. Oshvang . [mailto:levonshe@gmail.com] > Sent: Thursday, April 30, 2020 6:12 PM > To: Roberto Sassu <roberto.sassu@huawei.com>; linux- > integrity@vger.kernel.org > Subject: Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure > > On Wed, Apr 29, 2020 at 10:45 AM Roberto Sassu > <roberto.sassu@huawei.com> wrote: > > > > EVM is a module for the protection of the integrity of file metadata. It > > protects security-relevant extended attributes, and some file attributes > > such as the UID and the GID. It protects their integrity with an HMAC or > > with a signature. > > > > What makes EVM different from other LSMs is that it makes a security > > decision depending on multiple pieces of information, which cannot be > > managed atomically by the system. > > > > Example: cp -a file.orig file.dest > > > > If security.selinux, security.ima and security.evm must be preserved, cp > > will invoke setxattr() for each xattr, and EVM performs a verification > > during each operation. The problem is that copying security.evm from > > file.orig to file.dest will likely break the following EVM verifications if > > some metadata still have to be copied. EVM has no visibility on the > > metadata of the source file, so it cannot determine when the copy can be > > considered complete. > > > > On the other hand, EVM has to check metadata during every operation to > > ensure that there is no transition from corrupted metadata, e.g. after an > > offline attack, to valid ones after the operation. An HMAC update would > > prevent the corruption to be detected, as the HMAC on the new values > would > > be correct. Thus, to avoid this issue, EVM has to return an error to the > > system call so that its execution will be interrupted. > > > > A solution that would satisfy both requirements, not breaking user space > > applications and detecting corrupted metadata is to let metadata > operations > > be completed successfully and to pass the result of the EVM verification > > from the pre hooks to the post hooks. In this way, the HMAC update can > be > > avoided if the verification wasn't successful. > > > > This approach will bring another important benefit: it is no longer > > required that every file has a valid HMAC or signature. Instead of always > > enforcing metadata integrity, even when it is not relevant for IMA, EVM > > will let IMA decide for files selected with the appraisal policy, > > depending on the result of the requested verification. > > > > The main problem is that the result of the verification currently cannot be > > passed from the pre hooks to the post hooks, due to how the LSM API is > > defined. A possible solution would be to use integrity_iint_cache for this > > purpose, but it will increase the memory pressure, as new structures will > > be allocated also for metadata operations, not only for measurement, > > appraisal and audit. Another solution would be to extend the LSM API, but > > it seems not worthwhile as EVM would be the only module getting a > benefit > > from this change. > > > > Given that pre and post hooks are called from the same system call, a > more > > efficient solution seems to move the hooks outside the LSM infrastructure, > > so that the return value of the pre hooks can be passed to the post hooks. > > A predefined error (-EAGAIN) will be used to signal to the system call to > > continue the execution. Otherwise, if the pre hooks return -EPERM, the > > system calls will behave as before and will immediately return before > > metadata are changed. > > > > Overview of the changes: > > > > evm_inode_init_security() LSM (no change) > > evm_inode_setxattr() LSM -> vfs_setxattr() > > evm_inode_post_setxattr() LSM -> vfs_setxattr() > > evm_inode_removexattr() LSM -> vfs_removexattr() > > evm_inode_post_removexattr() vfs_removexattr() (no change) > > evm_inode_setattr() LSM -> vfs_setattr() > > evm_inode_post_setattr() vfs_setattr() (no change) > > evm_verifyxattr() outside LSM (no change) > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > --- > > fs/attr.c | 5 ++++- > > fs/xattr.c | 17 +++++++++++++++-- > > security/security.c | 18 +++--------------- > > 3 files changed, 22 insertions(+), 18 deletions(-) > > > > diff --git a/fs/attr.c b/fs/attr.c > > index b4bbdbd4c8ca..8f26d7d2e3b4 100644 > > --- a/fs/attr.c > > +++ b/fs/attr.c > > @@ -224,7 +224,7 @@ int notify_change(struct dentry * dentry, struct > iattr * attr, struct inode **de > > { > > struct inode *inode = dentry->d_inode; > > umode_t mode = inode->i_mode; > > - int error; > > + int error, evm_error; > > struct timespec64 now; > > unsigned int ia_valid = attr->ia_valid; > > > > @@ -328,6 +328,9 @@ int notify_change(struct dentry * dentry, struct > iattr * attr, struct inode **de > > error = security_inode_setattr(dentry, attr); > > if (error) > > return error; > > + evm_error = evm_inode_setattr(dentry, attr); > > + if (evm_error) > > + return evm_error; > > error = try_break_deleg(inode, delegated_inode); > > if (error) > > return error; > > diff --git a/fs/xattr.c b/fs/xattr.c > > index e13265e65871..3b323b75b741 100644 > > --- a/fs/xattr.c > > +++ b/fs/xattr.c > > @@ -183,6 +183,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, > const char *name, > > fsnotify_xattr(dentry); > > security_inode_post_setxattr(dentry, name, value, > > size, flags); > > + evm_inode_post_setxattr(dentry, name, value, size); > > } > > } else { > > if (unlikely(is_bad_inode(inode))) > > @@ -210,7 +211,7 @@ vfs_setxattr(struct dentry *dentry, const char > *name, const void *value, > > size_t size, int flags) > > { > > struct inode *inode = dentry->d_inode; > > - int error; > > + int error, evm_error; > > > > error = xattr_permission(inode, name, MAY_WRITE); > > if (error) > > @@ -221,6 +222,12 @@ vfs_setxattr(struct dentry *dentry, const char > *name, const void *value, > > if (error) > > goto out; > > > > + evm_error = evm_inode_setxattr(dentry, name, value, size); > > + if (evm_error) { > > + error = evm_error; > > + goto out; > > + } > > + > > error = __vfs_setxattr_noperm(dentry, name, value, size, flags); > > > > out: > > @@ -382,7 +389,7 @@ int > > vfs_removexattr(struct dentry *dentry, const char *name) > > { > > struct inode *inode = dentry->d_inode; > > - int error; > > + int error, evm_error; > > > > error = xattr_permission(inode, name, MAY_WRITE); > > if (error) > > @@ -393,6 +400,12 @@ vfs_removexattr(struct dentry *dentry, const > char *name) > > if (error) > > goto out; > > > > + evm_error = evm_inode_removexattr(dentry, name); > > + if (evm_error) { > > + error = evm_error; > > + goto out; > > + } > > + > > error = __vfs_removexattr(dentry, name); > > > > if (!error) { > > diff --git a/security/security.c b/security/security.c > > index 7fed24b9d57e..e1368ab34cee 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -1255,14 +1255,9 @@ int security_inode_permission(struct inode > *inode, int mask) > > > > int security_inode_setattr(struct dentry *dentry, struct iattr *attr) > > { > > - int ret; > > - > > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > > return 0; > > - ret = call_int_hook(inode_setattr, 0, dentry, attr); > > - if (ret) > > - return ret; > > - return evm_inode_setattr(dentry, attr); > > + return call_int_hook(inode_setattr, 0, dentry, attr); > > } > > EXPORT_SYMBOL_GPL(security_inode_setattr); > > > > @@ -1291,10 +1286,7 @@ int security_inode_setxattr(struct dentry > *dentry, const char *name, > > ret = cap_inode_setxattr(dentry, name, value, size, flags); > > if (ret) > > return ret; > > - ret = ima_inode_setxattr(dentry, name, value, size); > > - if (ret) > > - return ret; > > - return evm_inode_setxattr(dentry, name, value, size); > > + return ima_inode_setxattr(dentry, name, value, size); > > } > > > > void security_inode_post_setxattr(struct dentry *dentry, const char > *name, > > @@ -1303,7 +1295,6 @@ void security_inode_post_setxattr(struct dentry > *dentry, const char *name, > > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > > return; > > call_void_hook(inode_post_setxattr, dentry, name, value, size, flags); > > - evm_inode_post_setxattr(dentry, name, value, size); > > } > > > > int security_inode_getxattr(struct dentry *dentry, const char *name) > > @@ -1335,10 +1326,7 @@ int security_inode_removexattr(struct dentry > *dentry, const char *name) > > ret = cap_inode_removexattr(dentry, name); > > if (ret) > > return ret; > > - ret = ima_inode_removexattr(dentry, name); > > - if (ret) > > - return ret; > > - return evm_inode_removexattr(dentry, name); > > + return ima_inode_removexattr(dentry, name); > > } > > > > int security_inode_need_killpriv(struct dentry *dentry) > > -- > > 2.17.1 > > > > Hi Roberto, > > I apologize that due to my relatively small experience I may not > understand completely your patch. > Please be patient. > To my understanding, EVM knows(configuration) which security > attributes must be taken into HMAC, security.selinux, security.smack, > system.acl, security.ima, etc. > So until HMAC calculation should fail until cp command will not finish > copying all security attributes. > Is it the desired behavior of HMAC? Hi Lev HMAC is not the main focus of this patch. In /etc/xattr.conf there is a line to ignore security.evm. so cp will copy one xattr at time and the HMAC will never be invalid. The problem arises with signatures. In this case you cannot ignore security.evm and you have to copy it to the destination. But when you copy it, verification of the destination is not yet possible because other xattrs are still not copied. My idea is that it is not necessary to block xattr operations to guarantee xattr integrity. The same can be achieved by letting the operations be performed and not updating the HMAC if xattrs are corrupted. This will ensure much better compatibility with user space. > If the number of security attributes varies between files under EVM, > can we just remove > evm_inode_setxattr() from secutity_inode_setxattr() because it is the > job of evmctl utility to compute security.hmac value? > > Actually I do not see the use case why HMAC should be computed inline. > I come from embedded word and will never modify extended attributes > because the signing key will never be placed on the target device. I > think this argument stands also for production servers and for package > management as well. HMAC is meant to be used to protect mutable files. Since security.ima changes after every modification, as it includes the digest of the file, signatures cannot be used. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
> -----Original Message----- > From: Roberto Sassu > Sent: Wednesday, April 29, 2020 9:40 AM > To: zohar@linux.ibm.com; david.safford@gmail.com; > viro@zeniv.linux.org.uk; jmorris@namei.org > Cc: linux-fsdevel@vger.kernel.org; linux-integrity@vger.kernel.org; linux- > security-module@vger.kernel.org; linux-kernel@vger.kernel.org; Silviu > Vlasceanu <Silviu.Vlasceanu@huawei.com>; Roberto Sassu > <roberto.sassu@huawei.com> > Subject: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Any thought on this? The implementation can be discussed later. I just wanted a feedback on the approach, if this is the right direction to solve the problem. Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > EVM is a module for the protection of the integrity of file metadata. It > protects security-relevant extended attributes, and some file attributes > such as the UID and the GID. It protects their integrity with an HMAC or > with a signature. > > What makes EVM different from other LSMs is that it makes a security > decision depending on multiple pieces of information, which cannot be > managed atomically by the system. > > Example: cp -a file.orig file.dest > > If security.selinux, security.ima and security.evm must be preserved, cp > will invoke setxattr() for each xattr, and EVM performs a verification > during each operation. The problem is that copying security.evm from > file.orig to file.dest will likely break the following EVM verifications if > some metadata still have to be copied. EVM has no visibility on the > metadata of the source file, so it cannot determine when the copy can be > considered complete. > > On the other hand, EVM has to check metadata during every operation to > ensure that there is no transition from corrupted metadata, e.g. after an > offline attack, to valid ones after the operation. An HMAC update would > prevent the corruption to be detected, as the HMAC on the new values > would > be correct. Thus, to avoid this issue, EVM has to return an error to the > system call so that its execution will be interrupted. > > A solution that would satisfy both requirements, not breaking user space > applications and detecting corrupted metadata is to let metadata operations > be completed successfully and to pass the result of the EVM verification > from the pre hooks to the post hooks. In this way, the HMAC update can be > avoided if the verification wasn't successful. > > This approach will bring another important benefit: it is no longer > required that every file has a valid HMAC or signature. Instead of always > enforcing metadata integrity, even when it is not relevant for IMA, EVM > will let IMA decide for files selected with the appraisal policy, > depending on the result of the requested verification. > > The main problem is that the result of the verification currently cannot be > passed from the pre hooks to the post hooks, due to how the LSM API is > defined. A possible solution would be to use integrity_iint_cache for this > purpose, but it will increase the memory pressure, as new structures will > be allocated also for metadata operations, not only for measurement, > appraisal and audit. Another solution would be to extend the LSM API, but > it seems not worthwhile as EVM would be the only module getting a benefit > from this change. > > Given that pre and post hooks are called from the same system call, a more > efficient solution seems to move the hooks outside the LSM infrastructure, > so that the return value of the pre hooks can be passed to the post hooks. > A predefined error (-EAGAIN) will be used to signal to the system call to > continue the execution. Otherwise, if the pre hooks return -EPERM, the > system calls will behave as before and will immediately return before > metadata are changed. > > Overview of the changes: > > evm_inode_init_security() LSM (no change) > evm_inode_setxattr() LSM -> vfs_setxattr() > evm_inode_post_setxattr() LSM -> vfs_setxattr() > evm_inode_removexattr() LSM -> vfs_removexattr() > evm_inode_post_removexattr() vfs_removexattr() (no change) > evm_inode_setattr() LSM -> vfs_setattr() > evm_inode_post_setattr() vfs_setattr() (no change) > evm_verifyxattr() outside LSM (no change) > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > fs/attr.c | 5 ++++- > fs/xattr.c | 17 +++++++++++++++-- > security/security.c | 18 +++--------------- > 3 files changed, 22 insertions(+), 18 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index b4bbdbd4c8ca..8f26d7d2e3b4 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -224,7 +224,7 @@ int notify_change(struct dentry * dentry, struct iattr > * attr, struct inode **de > { > struct inode *inode = dentry->d_inode; > umode_t mode = inode->i_mode; > - int error; > + int error, evm_error; > struct timespec64 now; > unsigned int ia_valid = attr->ia_valid; > > @@ -328,6 +328,9 @@ int notify_change(struct dentry * dentry, struct iattr > * attr, struct inode **de > error = security_inode_setattr(dentry, attr); > if (error) > return error; > + evm_error = evm_inode_setattr(dentry, attr); > + if (evm_error) > + return evm_error; > error = try_break_deleg(inode, delegated_inode); > if (error) > return error; > diff --git a/fs/xattr.c b/fs/xattr.c > index e13265e65871..3b323b75b741 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -183,6 +183,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, > const char *name, > fsnotify_xattr(dentry); > security_inode_post_setxattr(dentry, name, value, > size, flags); > + evm_inode_post_setxattr(dentry, name, value, > size); > } > } else { > if (unlikely(is_bad_inode(inode))) > @@ -210,7 +211,7 @@ vfs_setxattr(struct dentry *dentry, const char > *name, const void *value, > size_t size, int flags) > { > struct inode *inode = dentry->d_inode; > - int error; > + int error, evm_error; > > error = xattr_permission(inode, name, MAY_WRITE); > if (error) > @@ -221,6 +222,12 @@ vfs_setxattr(struct dentry *dentry, const char > *name, const void *value, > if (error) > goto out; > > + evm_error = evm_inode_setxattr(dentry, name, value, size); > + if (evm_error) { > + error = evm_error; > + goto out; > + } > + > error = __vfs_setxattr_noperm(dentry, name, value, size, flags); > > out: > @@ -382,7 +389,7 @@ int > vfs_removexattr(struct dentry *dentry, const char *name) > { > struct inode *inode = dentry->d_inode; > - int error; > + int error, evm_error; > > error = xattr_permission(inode, name, MAY_WRITE); > if (error) > @@ -393,6 +400,12 @@ vfs_removexattr(struct dentry *dentry, const char > *name) > if (error) > goto out; > > + evm_error = evm_inode_removexattr(dentry, name); > + if (evm_error) { > + error = evm_error; > + goto out; > + } > + > error = __vfs_removexattr(dentry, name); > > if (!error) { > diff --git a/security/security.c b/security/security.c > index 7fed24b9d57e..e1368ab34cee 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1255,14 +1255,9 @@ int security_inode_permission(struct inode > *inode, int mask) > > int security_inode_setattr(struct dentry *dentry, struct iattr *attr) > { > - int ret; > - > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return 0; > - ret = call_int_hook(inode_setattr, 0, dentry, attr); > - if (ret) > - return ret; > - return evm_inode_setattr(dentry, attr); > + return call_int_hook(inode_setattr, 0, dentry, attr); > } > EXPORT_SYMBOL_GPL(security_inode_setattr); > > @@ -1291,10 +1286,7 @@ int security_inode_setxattr(struct dentry *dentry, > const char *name, > ret = cap_inode_setxattr(dentry, name, value, size, flags); > if (ret) > return ret; > - ret = ima_inode_setxattr(dentry, name, value, size); > - if (ret) > - return ret; > - return evm_inode_setxattr(dentry, name, value, size); > + return ima_inode_setxattr(dentry, name, value, size); > } > > void security_inode_post_setxattr(struct dentry *dentry, const char > *name, > @@ -1303,7 +1295,6 @@ void security_inode_post_setxattr(struct dentry > *dentry, const char *name, > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return; > call_void_hook(inode_post_setxattr, dentry, name, value, size, > flags); > - evm_inode_post_setxattr(dentry, name, value, size); > } > > int security_inode_getxattr(struct dentry *dentry, const char *name) > @@ -1335,10 +1326,7 @@ int security_inode_removexattr(struct dentry > *dentry, const char *name) > ret = cap_inode_removexattr(dentry, name); > if (ret) > return ret; > - ret = ima_inode_removexattr(dentry, name); > - if (ret) > - return ret; > - return evm_inode_removexattr(dentry, name); > + return ima_inode_removexattr(dentry, name); > } > > int security_inode_need_killpriv(struct dentry *dentry) > -- > 2.17.1
[Cc: John Johansen] On Wed, 2020-04-29 at 09:39 +0200, Roberto Sassu wrote: > EVM is a module for the protection of the integrity of file metadata. It > protects security-relevant extended attributes, and some file attributes > such as the UID and the GID. It protects their integrity with an HMAC or > with a signature. > > What makes EVM different from other LSMs is that it makes a security > decision depending on multiple pieces of information, which cannot be > managed atomically by the system. > > Example: cp -a file.orig file.dest > > If security.selinux, security.ima and security.evm must be preserved, cp > will invoke setxattr() for each xattr, and EVM performs a verification > during each operation. The problem is that copying security.evm from > file.orig to file.dest will likely break the following EVM verifications if > some metadata still have to be copied. EVM has no visibility on the > metadata of the source file, so it cannot determine when the copy can be > considered complete. I remember having a similar discussion in the past. At the time, there wasn't EVM portable and immutable signature support, just the HMAC and original signature types. Neither of these EVM xattrs types should be copied. Calling evm_verifyxattr() is not limited to IMA, but may be called by other LSMs/subsystems as well. At some point, there was some discussion about AppArmor calling it directly. Not sure if that is still being discussed. Since copying the EVM HMAC or original signature isn't applicable, I would prefer exploring an EVM portable and immutable signature only solution. Mimi
On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote: > Since copying the EVM HMAC or original signature isn't applicable, I > would prefer exploring an EVM portable and immutable signature only > solution. To prevent copying the EVM xattr, we added "security.evm" to /etc/xattr.conf. To support copying just the EVM portable and immutable signatures will require a different solution. Mimi
> -----Original Message----- > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Wednesday, May 6, 2020 11:10 PM > To: Roberto Sassu <roberto.sassu@huawei.com>; david.safford@gmail.com; > viro@zeniv.linux.org.uk; jmorris@namei.org; John Johansen > <john.johansen@canonical.com> > Cc: linux-fsdevel@vger.kernel.org; linux-integrity@vger.kernel.org; linux- > security-module@vger.kernel.org; linux-kernel@vger.kernel.org; Silviu > Vlasceanu <Silviu.Vlasceanu@huawei.com> > Subject: Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure > > On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote: > > Since copying the EVM HMAC or original signature isn't applicable, I > > would prefer exploring an EVM portable and immutable signature only > > solution. > > To prevent copying the EVM xattr, we added "security.evm" to > /etc/xattr.conf. To support copying just the EVM portable and > immutable signatures will require a different solution. This patch set removes the need for ignoring security.evm. It can be always copied, even if it is an HMAC. EVM will update it only when verification in the pre hook is successful. Combined with the ability of protecting a subset of files without introducing an EVM policy, these advantages seem to outweigh the effort necessary to make the switch. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Thu, 2020-05-07 at 07:53 +0000, Roberto Sassu wrote: > > -----Original Message----- > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > Sent: Wednesday, May 6, 2020 11:10 PM > > To: Roberto Sassu <roberto.sassu@huawei.com>; david.safford@gmail.com; > > viro@zeniv.linux.org.uk; jmorris@namei.org; John Johansen > > <john.johansen@canonical.com> > > Cc: linux-fsdevel@vger.kernel.org; linux-integrity@vger.kernel.org; linux- > > security-module@vger.kernel.org; linux-kernel@vger.kernel.org; Silviu > > Vlasceanu <Silviu.Vlasceanu@huawei.com> > > Subject: Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Roberto, please fix your mailer or at least manually remove this sort of info from the email. > > > > On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote: > > > Since copying the EVM HMAC or original signature isn't applicable, I > > > would prefer exploring an EVM portable and immutable signature only > > > solution. > > > > To prevent copying the EVM xattr, we added "security.evm" to > > /etc/xattr.conf. To support copying just the EVM portable and > > immutable signatures will require a different solution. > > This patch set removes the need for ignoring security.evm. It can be always > copied, even if it is an HMAC. EVM will update it only when verification in > the pre hook is successful. Combined with the ability of protecting a subset > of files without introducing an EVM policy, these advantages seem to > outweigh the effort necessary to make the switch. As the EVM file HMAC and original signature contain inode specific information (eg. i_version, i_generation), these xattrs cannot ever be copied. The proposed change is in order to support just the new EVM signatures. At least IMA file hashes should always be used in conjunction with EVM. EVM xattrs should always require a security.ima xattr to bind the file metadata to the file data. The IMA and EVM policies really need to be in sync. Mimi
> -----Original Message----- > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > On Thu, 2020-05-07 at 07:53 +0000, Roberto Sassu wrote: > > > -----Original Message----- > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > Sent: Wednesday, May 6, 2020 11:10 PM > > > To: Roberto Sassu <roberto.sassu@huawei.com>; > david.safford@gmail.com; > > > viro@zeniv.linux.org.uk; jmorris@namei.org; John Johansen > > > <john.johansen@canonical.com> > > > Cc: linux-fsdevel@vger.kernel.org; linux-integrity@vger.kernel.org; > linux- > > > security-module@vger.kernel.org; linux-kernel@vger.kernel.org; Silviu > > > Vlasceanu <Silviu.Vlasceanu@huawei.com> > > > Subject: Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM > infrastructure > > Roberto, please fix your mailer or at least manually remove this sort > of info from the email. > > > > > > > On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote: > > > > Since copying the EVM HMAC or original signature isn't applicable, I > > > > would prefer exploring an EVM portable and immutable signature only > > > > solution. > > > > > > To prevent copying the EVM xattr, we added "security.evm" to > > > /etc/xattr.conf. To support copying just the EVM portable and > > > immutable signatures will require a different solution. > > > > This patch set removes the need for ignoring security.evm. It can be > always > > copied, even if it is an HMAC. EVM will update it only when verification in > > the pre hook is successful. Combined with the ability of protecting a > subset > > of files without introducing an EVM policy, these advantages seem to > > outweigh the effort necessary to make the switch. > > As the EVM file HMAC and original signature contain inode specific > information (eg. i_version, i_generation), these xattrs cannot ever be > copied. The proposed change is in order to support just the new EVM > signatures. Right, I didn't consider it. Would it make sense instead to introduce an alias like security.evm_immutable so that this xattr can be copied? > At least IMA file hashes should always be used in conjunction with > EVM. EVM xattrs should always require a security.ima xattr to bind I proposed to enforce this restriction some time ago: https://patchwork.kernel.org/patch/10979351/ Is it ok to enforce it globally? > the file metadata to the file data. The IMA and EVM policies really > need to be in sync. It would be nice, but at the moment EVM considers also files that are not selected by the IMA policy. An example of why this is a problem is the audit service that fails to start when it tries to adjust the permissions of the log files. Those files don't have security.evm because they are not appraised by IMA, but EVM denies the operation. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote: > > > > On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote: > > > > > Since copying the EVM HMAC or original signature isn't applicable, I > > > > > would prefer exploring an EVM portable and immutable signature only > > > > > solution. > > > > > > > > To prevent copying the EVM xattr, we added "security.evm" to > > > > /etc/xattr.conf. To support copying just the EVM portable and > > > > immutable signatures will require a different solution. > > > > > > This patch set removes the need for ignoring security.evm. It can be > > always > > > copied, even if it is an HMAC. EVM will update it only when verification in > > > the pre hook is successful. Combined with the ability of protecting a > > subset > > > of files without introducing an EVM policy, these advantages seem to > > > outweigh the effort necessary to make the switch. > > > > As the EVM file HMAC and original signature contain inode specific > > information (eg. i_version, i_generation), these xattrs cannot ever be > > copied. The proposed change is in order to support just the new EVM > > signatures. > > Right, I didn't consider it. > > Would it make sense instead to introduce an alias like security.evm_immutable > so that this xattr can be copied? Being portable, not the attribute of being immutable, allows copying the EVM xattr. Your original problem - the order in which the xattrs are copied - might still be an issue. We need to look at "cp" closer to understand what it is doing. For example, are the xattrs written while the target file is tagged as a new file? There have been similar problems in the past. For example, tar calls mknodat to create the file, but doesn't write the file data. The solution there was to tag the file as a new file. We need to understand the problem better, before deciding how to resolve it. > > > At least IMA file hashes should always be used in conjunction with > > EVM. EVM xattrs should always require a security.ima xattr to bind > > I proposed to enforce this restriction some time ago: > > https://patchwork.kernel.org/patch/10979351/ > > Is it ok to enforce it globally? Doing this would then be dependent on upstreaming the initramfs xattr patches first, wouldn't it? :) > > > the file metadata to the file data. The IMA and EVM policies really > > need to be in sync. > > It would be nice, but at the moment EVM considers also files that are > not selected by the IMA policy. An example of why this is a problem is > the audit service that fails to start when it tries to adjust the permissions > of the log files. Those files don't have security.evm because they are > not appraised by IMA, but EVM denies the operation. No, this is a timing issue as to whether or not the builtin policy or a custom policy has been loaded. A custom policy could exclude the log files based on LSM labels, but they are included in the builtin policy. Mimi
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote: > > > > > On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote: > > > > > > Since copying the EVM HMAC or original signature isn't applicable, I > > > > > > would prefer exploring an EVM portable and immutable signature > only > > > > > > solution. > > > > > > > > > > To prevent copying the EVM xattr, we added "security.evm" to > > > > > /etc/xattr.conf. To support copying just the EVM portable and > > > > > immutable signatures will require a different solution. > > > > > > > > This patch set removes the need for ignoring security.evm. It can be > > > always > > > > copied, even if it is an HMAC. EVM will update it only when verification > in > > > > the pre hook is successful. Combined with the ability of protecting a > > > subset > > > > of files without introducing an EVM policy, these advantages seem to > > > > outweigh the effort necessary to make the switch. > > > > > > As the EVM file HMAC and original signature contain inode specific > > > information (eg. i_version, i_generation), these xattrs cannot ever be > > > copied. The proposed change is in order to support just the new EVM > > > signatures. > > > > Right, I didn't consider it. > > > > Would it make sense instead to introduce an alias like > security.evm_immutable > > so that this xattr can be copied? > > Being portable, not the attribute of being immutable, allows copying > the EVM xattr. Your original problem - the order in which the xattrs > are copied - might still be an issue. We need to look at "cp" closer > to understand what it is doing. For example, are the xattrs written > while the target file is tagged as a new file? Yes: openat(AT_FDCWD, "/bin/cat", O_RDONLY|O_NOFOLLOW) = 3 openat(AT_FDCWD, "cat", O_WRONLY|O_CREAT|O_EXCL, 0700) = 4 read(3, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\3601\0\0\0\0\0\0"..., 131072) = 85520 write(4, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\3601\0\0\0\0\0\0"..., 85520) = 85520 read(3, "", 131072) = 0 flistxattr(3, NULL, 0) = 43 flistxattr(3, "security.selinux\0security.evm\0se"..., 43) = 43 fgetxattr(3, "security.evm", NULL, 0) = 521 fgetxattr(3, "security.evm", "\5\2\4\256\316\302\206\2\09\226L52=\233^n\376:\363+\231\272\213\t\247\312\324\263\222N"..., 521) = 521 fsetxattr(4, "security.evm", "\5\2\4\256\316\302\206\2\09\226L52=\233^n\376:\363+\231\272\213\t\247\312\324\263\222N"..., 521, 0) = 0 fgetxattr(3, "security.ima", NULL, 0) = 34 fgetxattr(3, "security.ima", "\4\4\323\327\215\202I1~\325\0V\354}\4\3328$\210\363ja'\364\351\26\27\222\331\177\23\341"..., 34) = 34 fsetxattr(4, "security.ima", "\4\4\323\327\215\202I1~\325\0V\354}\4\3328$\210\363ja'\364\351\26\27\222\331\177\23\341"..., 34, 0) = 0 fgetxattr(3, "system.posix_acl_access", 0x7ffdf6929310, 132) = -1 ENODATA (No data available) fstat(3, {st_mode=S_IFREG|0755, st_size=85520, ...}) = 0 fsetxattr(4, "system.posix_acl_access", "\2\0\0\0\1\0\7\0\377\377\377\377\4\0\5\0\377\377\377\377 \0\5\0\377\377\377\377", 28, 0) = 0 close(4) = 0 close(3) = 0 This will fail. After security.evm is added to the target, the status when security.ima is added is INTEGRITY_FAIL. openat(AT_FDCWD, "test-archive.tar", O_RDONLY) = 3 mknodat(AT_FDCWD, "cat", 0755) = 0 setxattr("cat", "security.selinux", "system_u:object_r:bin_t:s0", 27, 0) = 0 setxattr("cat", "security.evm", "\5\2\4\256\316\302\206\2\09\226L52=\233^n\376:\363+\231\272\213\t\247\312\324\263\222N"..., 521, 0) = 0 setxattr("cat", "security.ima", "\4\4\323\327\215\202I1~\325\0V\354}\4\3328$\210\363ja'\364\351\26\27\222\331\177\23\341"..., 34, 0) = 0 openat(AT_FDCWD, "cat", O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK|O_CLOEXEC, 0700) = 4 write(4, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\3601\0\0\0\0\0\0"..., 8192) = 8192 read(3, "\363\17\36\372H\203\354\10H\213\5\301\217\0\0H\205\300t\2\377\320H\203\304\10\303\0\0\0\0\0"..., 10240) = 10240 write(4, "\363\17\36\372H\203\354\10H\213\5\301\217\0\0H\205\300t\2\377\320H\203\304\10\303\0\0\0\0\0"..., 10240) = 10240 read(3, "\300E\204\311t\16M9\346v\5C\306\4'\\I\203\304\1H\203\303\1H9\313\17\203M\1\0"..., 10240) = 10240 write(4, "\300E\204\311t\16M9\346v\5C\306\4'\\I\203\304\1H\203\303\1H9\313\17\203M\1\0"..., 10240) = 10240 read(3, "\1\0\2\0write error\0cat\0[\0test invoc"..., 10240) = 10240 write(4, "\1\0\2\0write error\0cat\0[\0test invoc"..., 10240) = 10240 read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 10240) = 10240 write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 10240) = 10240 read(3, "\0\0\0\0\0\1\0\0GA*\6\22\0\0\0\21\0\f\0\21\0\0\0\0\0\0\0\0\1\0\0"..., 10240) = 10240 write(4, "\0\0\0\0\0\1\0\0GA*\6\22\0\0\0\21\0\f\0\21\0\0\0\0\0\0\0\0\1\0\0"..., 10240) = 10240 read(3, "\0\0\0\0\27\0\0\0\0\0\0\0\0\1\0\0GA$\5gcc 9.0.1 20"..., 10240) = 10240 write(4, "\0\0\0\0\27\0\0\0\0\0\0\0\0\1\0\0GA$\5gcc 9.0.1 20"..., 10240) = 10240 read(3, "\0\0\0\0\0\1\0\0GA+stack_clash\0\0\23\0\0\0\0\0\0\0"..., 10240) = 10240 write(4, "\0\0\0\0\0\1\0\0GA+stack_clash\0\0\23\0\0\0\0\0\0\0"..., 10240) = 10240 read(3, "\0\0\0\0\0\1\0\0GA+GLIBCXX_ASSERTIONS\0\0\0"..., 10240) = 10240 write(4, "\0\0\0\0\0\1\0\0GA+GLIBCXX_ASSERTIONS\0\0\0"..., 5648) = 5648 fchown(4, 0, 0) = 0 fchmod(4, 0755) = 0 close(4) = 0 close(3) = 0 Also this will fail, but for a different reason. After security.selinux is written, the status when security.evm is added is INTEGRITY_NOLABEL (unless the HMAC key is loaded, otherwise it will fail when tar sets security.ima). Even with my patches to ignore verification errors, there is still another problem. The signature becomes valid when all xattrs are set, but tar tries anyway to adjust the owner even if it is already correct. Since metadata are immutable, fchown() fails (this status is not ignored). After we solve the problem of copying xattrs, I will send a patch to determine whether setxattr()/setattr() change metadata. If not, the operation is allowed even if metadata are immutable. > There have been similar problems in the past. For example, tar calls > mknodat to create the file, but doesn't write the file data. The > solution there was to tag the file as a new file. > > We need to understand the problem better, before deciding how to > resolve it. > > > > > > At least IMA file hashes should always be used in conjunction with > > > EVM. EVM xattrs should always require a security.ima xattr to bind > > > > I proposed to enforce this restriction some time ago: > > > > https://patchwork.kernel.org/patch/10979351/ > > > > Is it ok to enforce it globally? > > Doing this would then be dependent on upstreaming the initramfs xattr > patches first, wouldn't it? :) Well, this patch set and the metadata change detection are a dependency of the initramfs xattr patch set. Without them, we cannot create a ram disk with files with portable signatures, if enforcement is enabled. The new appraisal modes solve a general security issue that would occur also when appraisal is done in the root filesystem (it is possible to gain access to all files for which the IMA policy does not require a signature by removing the EVM keys and adding to those files a valid security.ima with the file digest). > > > the file metadata to the file data. The IMA and EVM policies really > > > need to be in sync. > > > > It would be nice, but at the moment EVM considers also files that are > > not selected by the IMA policy. An example of why this is a problem is > > the audit service that fails to start when it tries to adjust the permissions > > of the log files. Those files don't have security.evm because they are > > not appraised by IMA, but EVM denies the operation. > > No, this is a timing issue as to whether or not the builtin policy or > a custom policy has been loaded. A custom policy could exclude the > log files based on LSM labels, but they are included in the builtin > policy. Yes, I was referring to a custom policy. In this case, EVM will not adapt to the custom policy but still verifies all files. If access control is done exclusively by IMA at the time evm_verifyxattr() is called, we wouldn't need to add security.evm to all files. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Fri, 2020-05-08 at 10:20 +0000, Roberto Sassu wrote: > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote: <snip> > > > > the file metadata to the file data. The IMA and EVM policies really > > > > need to be in sync. > > > > > > It would be nice, but at the moment EVM considers also files that are > > > not selected by the IMA policy. An example of why this is a problem is > > > the audit service that fails to start when it tries to adjust the permissions > > > of the log files. Those files don't have security.evm because they are > > > not appraised by IMA, but EVM denies the operation. > > > > No, this is a timing issue as to whether or not the builtin policy or > > a custom policy has been loaded. A custom policy could exclude the > > log files based on LSM labels, but they are included in the builtin > > policy. > > Yes, I was referring to a custom policy. In this case, EVM will not adapt > to the custom policy but still verifies all files. If access control is done > exclusively by IMA at the time evm_verifyxattr() is called, we wouldn't > need to add security.evm to all files. Roberto, EVM is only triggered by IMA, unless you've modified the kernel to do otherwise. I'm not interested in a complicated solution, just one that addresses the new EVM immutable and portable signature. It might require EVM HMAC, IMA differentiating between a new file and an existing file, or it might require writing the new EVM signature last, after all the other xattrs or metadata are updated. Please nothing that changes existing expectations. Mimi
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Friday, May 8, 2020 7:08 PM > On Fri, 2020-05-08 at 10:20 +0000, Roberto Sassu wrote: > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote: > > <snip> > > > > > > the file metadata to the file data. The IMA and EVM policies really > > > > > need to be in sync. > > > > > > > > It would be nice, but at the moment EVM considers also files that are > > > > not selected by the IMA policy. An example of why this is a problem is > > > > the audit service that fails to start when it tries to adjust the > permissions > > > > of the log files. Those files don't have security.evm because they are > > > > not appraised by IMA, but EVM denies the operation. > > > > > > No, this is a timing issue as to whether or not the builtin policy or > > > a custom policy has been loaded. A custom policy could exclude the > > > log files based on LSM labels, but they are included in the builtin > > > policy. > > > > Yes, I was referring to a custom policy. In this case, EVM will not adapt > > to the custom policy but still verifies all files. If access control is done > > exclusively by IMA at the time evm_verifyxattr() is called, we wouldn't > > need to add security.evm to all files. > > Roberto, EVM is only triggered by IMA, unless you've modified the > kernel to do otherwise. EVM would deny xattr/attr operations even if IMA is disabled in the kernel configuration. For example, evm_setxattr() returns the value from evm_protect_xattr(). IMA is not involved there. > I'm not interested in a complicated solution, just one that addresses > the new EVM immutable and portable signature. It might require EVM > HMAC, IMA differentiating between a new file and an existing file, or > it might require writing the new EVM signature last, after all the > other xattrs or metadata are updated. Please nothing that changes > existing expectations. Ok. Introducing the new status INTEGRITY_FAIL_IMMUTABLE, as I mentioned in '[PATCH] ima: Allow imasig requirement to be satisfied by EVM portable signatures' seems to have an additional benefit. We could introduce an additional exception in evm_protect_xattr(), other than INTEGRITY_NOXATTRS, as we know that xattr/attr update won't cause HMAC update. However, it won't work unless the IMA policy says that the file should be appraised when the mknod() system call is executed. Otherwise, integrity_iint_cache is not created for the file and the IMA_NEW_FILE flag is not set. Granting an exception for INTEGRITY_FAIL_IMMUTABLE solves the case where security.evm is the first xattr set. If a protected xattr is the first to be added, then we also have to handle the INTEGRITY_NOLABEL error. It should be fine to add an exception for this error if the HMAC key is not loaded. This still does not solve all problems. INTEGRITY_NOLABEL cannot be ignored if the HMAC key is loaded, which means that all files need to be protected by EVM to avoid issues like the one I described (auditd). Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Mon, 2020-05-11 at 14:13 +0000, Roberto Sassu wrote: > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > Sent: Friday, May 8, 2020 7:08 PM > > On Fri, 2020-05-08 at 10:20 +0000, Roberto Sassu wrote: > > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > > On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote: > > > > <snip> > > > > > > > > the file metadata to the file data. The IMA and EVM policies really > > > > > > need to be in sync. > > > > > > > > > > It would be nice, but at the moment EVM considers also files that are > > > > > not selected by the IMA policy. An example of why this is a problem is > > > > > the audit service that fails to start when it tries to adjust the > > permissions > > > > > of the log files. Those files don't have security.evm because they are > > > > > not appraised by IMA, but EVM denies the operation. > > > > > > > > No, this is a timing issue as to whether or not the builtin policy or > > > > a custom policy has been loaded. A custom policy could exclude the > > > > log files based on LSM labels, but they are included in the builtin > > > > policy. > > > > > > Yes, I was referring to a custom policy. In this case, EVM will not adapt > > > to the custom policy but still verifies all files. If access control is done > > > exclusively by IMA at the time evm_verifyxattr() is called, we wouldn't > > > need to add security.evm to all files. > > > > Roberto, EVM is only triggered by IMA, unless you've modified the > > kernel to do otherwise. > > EVM would deny xattr/attr operations even if IMA is disabled in the > kernel configuration. For example, evm_setxattr() returns the value > from evm_protect_xattr(). IMA is not involved there. Commit ae1ba1676b88 ("EVM: Allow userland to permit modification of EVM-protected metadata") introduced EVM_ALLOW_METADATA_WRITES to allow writing the EVM portable and immutable file signatures. > > > I'm not interested in a complicated solution, just one that addresses > > the new EVM immutable and portable signature. It might require EVM > > HMAC, IMA differentiating between a new file and an existing file, or:q > > it might require writing the new EVM signature last, after all the > > other xattrs or metadata are updated. Please nothing that changes > > existing expectations. > > Ok. Introducing the new status INTEGRITY_FAIL_IMMUTABLE, as I > mentioned in '[PATCH] ima: Allow imasig requirement to be satisfied by > EVM portable signatures' seems to have an additional benefit. We > could introduce an additional exception in evm_protect_xattr(), other > than INTEGRITY_NOXATTRS, as we know that xattr/attr update won't > cause HMAC update. Refer to Documentation/ABI/testing/evm describes on how to permit writing the security.evm signatures. > > However, it won't work unless the IMA policy says that the file should > be appraised when the mknod() system call is executed. Otherwise, > integrity_iint_cache is not created for the file and the IMA_NEW_FILE > flag is not set. > > Granting an exception for INTEGRITY_FAIL_IMMUTABLE solves the case > where security.evm is the first xattr set. If a protected xattr is the first to > be added, then we also have to handle the INTEGRITY_NOLABEL error. > It should be fine to add an exception for this error if the HMAC key is not > loaded. > > This still does not solve all problems. INTEGRITY_NOLABEL cannot be > ignored if the HMAC key is loaded, which means that all files need to be > protected by EVM to avoid issues like the one I described (auditd). The application still needs to defer writing the EVM portable and immutable file signatures until after all the other xattrs are written otherwise it won't validate. Mimi
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Monday, May 11, 2020 11:37 PM > On Mon, 2020-05-11 at 14:13 +0000, Roberto Sassu wrote: > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > Sent: Friday, May 8, 2020 7:08 PM > > > On Fri, 2020-05-08 at 10:20 +0000, Roberto Sassu wrote: > > > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > > > On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote: > > > > > > <snip> > > > > > > > > > > the file metadata to the file data. The IMA and EVM policies > really > > > > > > > need to be in sync. > > > > > > > > > > > > It would be nice, but at the moment EVM considers also files that > are > > > > > > not selected by the IMA policy. An example of why this is a > problem is > > > > > > the audit service that fails to start when it tries to adjust the > > > permissions > > > > > > of the log files. Those files don't have security.evm because they > are > > > > > > not appraised by IMA, but EVM denies the operation. > > > > > > > > > > No, this is a timing issue as to whether or not the builtin policy or > > > > > a custom policy has been loaded. A custom policy could exclude the > > > > > log files based on LSM labels, but they are included in the builtin > > > > > policy. > > > > > > > > Yes, I was referring to a custom policy. In this case, EVM will not adapt > > > > to the custom policy but still verifies all files. If access control is done > > > > exclusively by IMA at the time evm_verifyxattr() is called, we wouldn't > > > > need to add security.evm to all files. > > > > > > Roberto, EVM is only triggered by IMA, unless you've modified the > > > kernel to do otherwise. > > > > EVM would deny xattr/attr operations even if IMA is disabled in the > > kernel configuration. For example, evm_setxattr() returns the value > > from evm_protect_xattr(). IMA is not involved there. > > Commit ae1ba1676b88 ("EVM: Allow userland to permit modification of > EVM-protected metadata") introduced EVM_ALLOW_METADATA_WRITES > to allow > writing the EVM portable and immutable file signatures. According to Documentation/ABI/testing/evm: Note that once a key has been loaded, it will no longer be possible to enable metadata modification. We load the EVM key from /etc/keys/x509_evm.der, during kernel initialization, which excludes the possibility of using that flag. Deferring the loading of the key is not possible if that key is used for appraisal and enforcement is enabled from the beginning. Also, once the flag is cleared portable signatures cannot be installed until reboot. This seems a big limitation especially when software updates are installed on a running system. > > > I'm not interested in a complicated solution, just one that addresses > > > the new EVM immutable and portable signature. It might require EVM > > > HMAC, IMA differentiating between a new file and an existing file, or:q > > > > it might require writing the new EVM signature last, after all the > > > other xattrs or metadata are updated. Please nothing that changes > > > existing expectations. > > > > Ok. Introducing the new status INTEGRITY_FAIL_IMMUTABLE, as I > > mentioned in '[PATCH] ima: Allow imasig requirement to be satisfied by > > EVM portable signatures' seems to have an additional benefit. We > > could introduce an additional exception in evm_protect_xattr(), other > > than INTEGRITY_NOXATTRS, as we know that xattr/attr update won't > > cause HMAC update. > > Refer to Documentation/ABI/testing/evm describes on how to permit > writing the security.evm signatures. It doesn't seem there is a solution for adding portable signatures at run-time. > > However, it won't work unless the IMA policy says that the file should > > be appraised when the mknod() system call is executed. Otherwise, > > integrity_iint_cache is not created for the file and the IMA_NEW_FILE > > flag is not set. > > > > Granting an exception for INTEGRITY_FAIL_IMMUTABLE solves the case > > where security.evm is the first xattr set. If a protected xattr is the first to > > be added, then we also have to handle the INTEGRITY_NOLABEL error. > > It should be fine to add an exception for this error if the HMAC key is not > > loaded. > > > > This still does not solve all problems. INTEGRITY_NOLABEL cannot be > > ignored if the HMAC key is loaded, which means that all files need to be > > protected by EVM to avoid issues like the one I described (auditd). > > The application still needs to defer writing the EVM portable and > immutable file signatures until after all the other xattrs are written > otherwise it won't validate. EVM won't allow that because at the second setxattr() it will find that security.evm is missing. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Tue, 2020-05-12 at 07:54 +0000, Roberto Sassu wrote: > > > > Roberto, EVM is only triggered by IMA, unless you've modified the > > > > kernel to do otherwise. > > > > > > EVM would deny xattr/attr operations even if IMA is disabled in the > > > kernel configuration. For example, evm_setxattr() returns the value > > > from evm_protect_xattr(). IMA is not involved there. > > > > Commit ae1ba1676b88 ("EVM: Allow userland to permit modification of > > EVM-protected metadata") introduced EVM_ALLOW_METADATA_WRITES > > to allow writing the EVM portable and immutable file signatures. > > According to Documentation/ABI/testing/evm: > > Note that once a key has been loaded, it will no longer be > possible to enable metadata modification. Not any key, but the HMAC key. 2 Permit modification of EVM-protected metadata at runtime. Not supported if HMAC validation and creation is enabled. Each time the EVM protected file metadata is updated, the EVM HMAC is updated, assuming the existing EVM HMAC is valid. Userspace should not have access to the HMAC key, so we only allow writing EVM signatures. The only difference between writing the original EVM signature and the new portable and immutable signature is the security.ima xattr requirement. Since the new EVM signature does not include the filesystem specific data, something else needs to bind the file metadata to the file data. Thus the IMA xattr requirement. Assuming that the new EVM signature is written last, as long as there is an IMA xattr, there shouldn't be a problem writing the new EVM signature. Mimi
> From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux- > security-module@vger.kernel.org] On Behalf Of Mimi Zohar > Sent: Tuesday, May 12, 2020 4:17 PM > On Tue, 2020-05-12 at 07:54 +0000, Roberto Sassu wrote: > > > > > Roberto, EVM is only triggered by IMA, unless you've modified the > > > > > kernel to do otherwise. > > > > > > > > EVM would deny xattr/attr operations even if IMA is disabled in the > > > > kernel configuration. For example, evm_setxattr() returns the value > > > > from evm_protect_xattr(). IMA is not involved there. > > > > > > Commit ae1ba1676b88 ("EVM: Allow userland to permit modification of > > > EVM-protected metadata") > introduced EVM_ALLOW_METADATA_WRITES > > > to allow writing the EVM portable and immutable file signatures. > > > > According to Documentation/ABI/testing/evm: > > > > Note that once a key has been loaded, it will no longer be > > possible to enable metadata modification. > > Not any key, but the HMAC key. > > 2 Permit modification of EVM-protected metadata at > runtime. Not supported if HMAC validation and > creation is enabled. #ifdef CONFIG_EVM_LOAD_X509 void __init evm_load_x509(void) { [...] rc = integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); if (!rc) evm_initialized |= EVM_INIT_X509; static ssize_t evm_write_key(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { [...] /* Don't allow a request to freshly enable metadata writes if * keys are loaded. */ if ((i & EVM_ALLOW_METADATA_WRITES) && ((evm_initialized & EVM_KEY_MASK) != 0) && !(evm_initialized & EVM_ALLOW_METADATA_WRITES)) return -EPERM; Should have been: if ((i & EVM_ALLOW_METADATA_WRITES) && ((evm_initialized & EVM_INIT_HMAC) != 0) && !(evm_initialized & EVM_ALLOW_METADATA_WRITES)) return -EPERM; > Each time the EVM protected file metadata is updated, the EVM HMAC is > updated, assuming the existing EVM HMAC is valid. Userspace should > not have access to the HMAC key, so we only allow writing EVM > signatures. > > The only difference between writing the original EVM signature and the > new portable and immutable signature is the security.ima xattr > requirement. Since the new EVM signature does not include the > filesystem specific data, something else needs to bind the file > metadata to the file data. Thus the IMA xattr requirement. > > Assuming that the new EVM signature is written last, as long as there > is an IMA xattr, there shouldn't be a problem writing the new EVM > signature. /* first need to know the sig type */ rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char **)&xattr_data, 0, GFP_NOFS); if (rc <= 0) { evm_status = INTEGRITY_FAIL; if (rc == -ENODATA) { rc = evm_find_protected_xattrs(dentry); if (rc > 0) evm_status = INTEGRITY_NOLABEL; else if (rc == 0) evm_status = INTEGRITY_NOXATTRS; /* new file */ If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr can be written (status INTEGRITY_NOXATTRS is ok). After, evm_find_protected_xattrs() returns rc > 0, so the status is INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr(). Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Tue, 2020-05-12 at 15:31 +0000, Roberto Sassu wrote: > > From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux- > > security-module@vger.kernel.org] On Behalf Of Mimi Zohar > > Sent: Tuesday, May 12, 2020 4:17 PM > > On Tue, 2020-05-12 at 07:54 +0000, Roberto Sassu wrote: > > > > > > Roberto, EVM is only triggered by IMA, unless you've modified the > > > > > > kernel to do otherwise. > > > > > > > > > > EVM would deny xattr/attr operations even if IMA is disabled in the > > > > > kernel configuration. For example, evm_setxattr() returns the value > > > > > from evm_protect_xattr(). IMA is not involved there. > > > > > > > > Commit ae1ba1676b88 ("EVM: Allow userland to permit modification of > > > > EVM-protected metadata") > > introduced EVM_ALLOW_METADATA_WRITES > > > > to allow writing the EVM portable and immutable file signatures. > > > > > > According to Documentation/ABI/testing/evm: > > > > > > Note that once a key has been loaded, it will no longer be > > > possible to enable metadata modification. > > > > Not any key, but the HMAC key. > > > > 2 Permit modification of EVM-protected metadata at > > runtime. Not supported if HMAC validation and > > creation is enabled. > > #ifdef CONFIG_EVM_LOAD_X509 > void __init evm_load_x509(void) > { > [...] > rc = integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); > if (!rc) > evm_initialized |= EVM_INIT_X509; > > > static ssize_t evm_write_key(struct file *file, const char __user *buf, > size_t count, loff_t *ppos) > { > [...] > /* Don't allow a request to freshly enable metadata writes if > * keys are loaded. > */ > if ((i & EVM_ALLOW_METADATA_WRITES) && > ((evm_initialized & EVM_KEY_MASK) != 0) && > !(evm_initialized & EVM_ALLOW_METADATA_WRITES)) > return -EPERM; > > Should have been: > > if ((i & EVM_ALLOW_METADATA_WRITES) && > ((evm_initialized & EVM_INIT_HMAC) != 0) && > !(evm_initialized & EVM_ALLOW_METADATA_WRITES)) > return -EPERM; Ok > > > Each time the EVM protected file metadata is updated, the EVM HMAC is > > updated, assuming the existing EVM HMAC is valid. Userspace should > > not have access to the HMAC key, so we only allow writing EVM > > signatures. > > > > The only difference between writing the original EVM signature and the > > new portable and immutable signature is the security.ima xattr > > requirement. Since the new EVM signature does not include the > > filesystem specific data, something else needs to bind the file > > metadata to the file data. Thus the IMA xattr requirement. > > > > Assuming that the new EVM signature is written last, as long as there > > is an IMA xattr, there shouldn't be a problem writing the new EVM > > signature. > > /* first need to know the sig type */ > rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char **)&xattr_data, 0, > GFP_NOFS); > if (rc <= 0) { > evm_status = INTEGRITY_FAIL; > if (rc == -ENODATA) { > rc = evm_find_protected_xattrs(dentry); > if (rc > 0) > evm_status = INTEGRITY_NOLABEL; > else if (rc == 0) > evm_status = INTEGRITY_NOXATTRS; /* new file */ > > If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr > can be written (status INTEGRITY_NOXATTRS is ok). After, > evm_find_protected_xattrs() returns rc > 0, so the status is > INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr(). With EVM HMAC enabled, as a result of writing the first protected xattr, an EVM HMAC should be calculated and written in evm_inode_post_setxattr(). Mimi
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Tuesday, May 12, 2020 5:50 PM > On Tue, 2020-05-12 at 15:31 +0000, Roberto Sassu wrote: > > > From: owner-linux-security-module@vger.kernel.org [mailto:owner- > linux- > > > security-module@vger.kernel.org] On Behalf Of Mimi Zohar > > > Sent: Tuesday, May 12, 2020 4:17 PM > > > On Tue, 2020-05-12 at 07:54 +0000, Roberto Sassu wrote: > > > > > > > Roberto, EVM is only triggered by IMA, unless you've modified > the > > > > > > > kernel to do otherwise. > > > > > > > > > > > > EVM would deny xattr/attr operations even if IMA is disabled in > the > > > > > > kernel configuration. For example, evm_setxattr() returns the > value > > > > > > from evm_protect_xattr(). IMA is not involved there. > > > > > > > > > > Commit ae1ba1676b88 ("EVM: Allow userland to permit modification > of > > > > > EVM-protected metadata") > > > introduced EVM_ALLOW_METADATA_WRITES > > > > > to allow writing the EVM portable and immutable file signatures. > > > > > > > > According to Documentation/ABI/testing/evm: > > > > > > > > Note that once a key has been loaded, it will no longer be > > > > possible to enable metadata modification. > > > > > > Not any key, but the HMAC key. > > > > > > 2 Permit modification of EVM-protected metadata at > > > runtime. Not supported if HMAC validation and > > > creation is enabled. > > > > #ifdef CONFIG_EVM_LOAD_X509 > > void __init evm_load_x509(void) > > { > > [...] > > rc = integrity_load_x509(INTEGRITY_KEYRING_EVM, > CONFIG_EVM_X509_PATH); > > if (!rc) > > evm_initialized |= EVM_INIT_X509; > > > > > > static ssize_t evm_write_key(struct file *file, const char __user *buf, > > size_t count, loff_t *ppos) > > { > > [...] > > /* Don't allow a request to freshly enable metadata writes if > > * keys are loaded. > > */ > > if ((i & EVM_ALLOW_METADATA_WRITES) && > > ((evm_initialized & EVM_KEY_MASK) != 0) && > > !(evm_initialized & EVM_ALLOW_METADATA_WRITES)) > > return -EPERM; > > > > Should have been: > > > > if ((i & EVM_ALLOW_METADATA_WRITES) && > > ((evm_initialized & EVM_INIT_HMAC) != 0) && > > !(evm_initialized & EVM_ALLOW_METADATA_WRITES)) > > return -EPERM; > > Ok > > > > > > Each time the EVM protected file metadata is updated, the EVM HMAC > is > > > updated, assuming the existing EVM HMAC is valid. Userspace should > > > not have access to the HMAC key, so we only allow writing EVM > > > signatures. > > > > > > The only difference between writing the original EVM signature and the > > > new portable and immutable signature is the security.ima xattr > > > requirement. Since the new EVM signature does not include the > > > filesystem specific data, something else needs to bind the file > > > metadata to the file data. Thus the IMA xattr requirement. > > > > > > Assuming that the new EVM signature is written last, as long as there > > > is an IMA xattr, there shouldn't be a problem writing the new EVM > > > signature. > > > > /* first need to know the sig type */ > > rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char > **)&xattr_data, 0, > > GFP_NOFS); > > if (rc <= 0) { > > evm_status = INTEGRITY_FAIL; > > if (rc == -ENODATA) { > > rc = evm_find_protected_xattrs(dentry); > > if (rc > 0) > > evm_status = INTEGRITY_NOLABEL; > > else if (rc == 0) > > evm_status = INTEGRITY_NOXATTRS; /* new file */ > > > > If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr > > can be written (status INTEGRITY_NOXATTRS is ok). After, > > evm_find_protected_xattrs() returns rc > 0, so the status is > > INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr(). > > With EVM HMAC enabled, as a result of writing the first protected > xattr, an EVM HMAC should be calculated and written in > evm_inode_post_setxattr(). To solve the ordering issue, wouldn't allowing setxattr() on a file with portable signature that does not yet pass verification be safe? evm_update_evmxattr() checks if the signature is portable and if yes, does not calculate the HMAC. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Tue, 2020-05-12 at 16:31 +0000, Roberto Sassu wrote: > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > > Each time the EVM protected file metadata is updated, the EVM HMAC > > is > > > > updated, assuming the existing EVM HMAC is valid. Userspace should > > > > not have access to the HMAC key, so we only allow writing EVM > > > > signatures. > > > > > > > > The only difference between writing the original EVM signature and the > > > > new portable and immutable signature is the security.ima xattr > > > > requirement. Since the new EVM signature does not include the > > > > filesystem specific data, something else needs to bind the file > > > > metadata to the file data. Thus the IMA xattr requirement. > > > > > > > > Assuming that the new EVM signature is written last, as long as there > > > > is an IMA xattr, there shouldn't be a problem writing the new EVM > > > > signature. > > > > > > /* first need to know the sig type */ > > > rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char > > **)&xattr_data, 0, > > > GFP_NOFS); > > > if (rc <= 0) { > > > evm_status = INTEGRITY_FAIL; > > > if (rc == -ENODATA) { > > > rc = evm_find_protected_xattrs(dentry); > > > if (rc > 0) > > > evm_status = INTEGRITY_NOLABEL; > > > else if (rc == 0) > > > evm_status = INTEGRITY_NOXATTRS; /* new file */ > > > > > > If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr > > > can be written (status INTEGRITY_NOXATTRS is ok). After, > > > evm_find_protected_xattrs() returns rc > 0, so the status is > > > INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr(). > > > > With EVM HMAC enabled, as a result of writing the first protected > > xattr, an EVM HMAC should be calculated and written in > > evm_inode_post_setxattr(). > > To solve the ordering issue, wouldn't allowing setxattr() on a file > with portable signature that does not yet pass verification be safe? > evm_update_evmxattr() checks if the signature is portable and > if yes, does not calculate the HMAC. Before agreeing to allowing the protected xattrs to be written on a file with a portable signature that does not yet pass verification are safe, would we be introducing any new types of attacks? For example, would we differentiate between portable signatures that don't pass verification and ones that do? If we don't differentiate, could it be used for DoS? Should it be limited to new files? Mimi
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Tuesday, May 12, 2020 9:38 PM > On Tue, 2020-05-12 at 16:31 +0000, Roberto Sassu wrote: > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > > > > Each time the EVM protected file metadata is updated, the EVM > HMAC > > > is > > > > > updated, assuming the existing EVM HMAC is valid. Userspace > should > > > > > not have access to the HMAC key, so we only allow writing EVM > > > > > signatures. > > > > > > > > > > The only difference between writing the original EVM signature and > the > > > > > new portable and immutable signature is the security.ima xattr > > > > > requirement. Since the new EVM signature does not include the > > > > > filesystem specific data, something else needs to bind the file > > > > > metadata to the file data. Thus the IMA xattr requirement. > > > > > > > > > > Assuming that the new EVM signature is written last, as long as there > > > > > is an IMA xattr, there shouldn't be a problem writing the new EVM > > > > > signature. > > > > > > > > /* first need to know the sig type */ > > > > rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char > > > **)&xattr_data, 0, > > > > GFP_NOFS); > > > > if (rc <= 0) { > > > > evm_status = INTEGRITY_FAIL; > > > > if (rc == -ENODATA) { > > > > rc = evm_find_protected_xattrs(dentry); > > > > if (rc > 0) > > > > evm_status = INTEGRITY_NOLABEL; > > > > else if (rc == 0) > > > > evm_status = INTEGRITY_NOXATTRS; /* new file */ > > > > > > > > If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr > > > > can be written (status INTEGRITY_NOXATTRS is ok). After, > > > > evm_find_protected_xattrs() returns rc > 0, so the status is > > > > INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr(). > > > > > > With EVM HMAC enabled, as a result of writing the first protected > > > xattr, an EVM HMAC should be calculated and written in > > > evm_inode_post_setxattr(). > > > > To solve the ordering issue, wouldn't allowing setxattr() on a file > > with portable signature that does not yet pass verification be safe? > > evm_update_evmxattr() checks if the signature is portable and > > if yes, does not calculate the HMAC. > > Before agreeing to allowing the protected xattrs to be written on a > file with a portable signature that does not yet pass verification are > safe, would we be introducing any new types of attacks? Allowing xattr/attr update means that someone can do: setxattr(path, "security.evm", ...); with type=5 all subsequent setxattr()/setattr() succeed until the correct combination is set. At that point, any xattr/attr operation fails, even if one tries to set an xattr with the same value. If we still deny the operation when the verification succeeds, we have to fix that. It is common that the signature passes verification before user space tools finish to set xattrs/attrs. For example, if a file is created with mode 644 and this was the mode at the time of signing, any attempt by tar for example to set again the same mode fails. If allowing a change of xattrs/attrs for portable signatures is safe or not, I would say yes. Portable signatures cannot be modified even if __vfs_setxattr_noperm() is called directly. > For example, would we differentiate between portable signatures that > don't pass verification and ones that do? If we don't differentiate, > could it be used for DoS? Should it be limited to new files? I would prefer to lock files when signatures pass the verification to avoid accidental changes. Unless we find a better way to identify new file, without depending on the appraisal policy, I would allow the operation even for existing files. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Wed, 2020-05-13 at 07:21 +0000, Roberto Sassu wrote: > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > Sent: Tuesday, May 12, 2020 9:38 PM > > On Tue, 2020-05-12 at 16:31 +0000, Roberto Sassu wrote: > > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > > > > > > Each time the EVM protected file metadata is updated, the EVM > > HMAC > > > > is > > > > > > updated, assuming the existing EVM HMAC is valid. Userspace > > should > > > > > > not have access to the HMAC key, so we only allow writing EVM > > > > > > signatures. > > > > > > > > > > > > The only difference between writing the original EVM signature and > > the > > > > > > new portable and immutable signature is the security.ima xattr > > > > > > requirement. Since the new EVM signature does not include the > > > > > > filesystem specific data, something else needs to bind the file > > > > > > metadata to the file data. Thus the IMA xattr requirement. > > > > > > > > > > > > Assuming that the new EVM signature is written last, as long as there > > > > > > is an IMA xattr, there shouldn't be a problem writing the new EVM > > > > > > signature. > > > > > > > > > > /* first need to know the sig type */ > > > > > rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char > > > > **)&xattr_data, 0, > > > > > GFP_NOFS); > > > > > if (rc <= 0) { > > > > > evm_status = INTEGRITY_FAIL; > > > > > if (rc == -ENODATA) { > > > > > rc = evm_find_protected_xattrs(dentry); > > > > > if (rc > 0) > > > > > evm_status = INTEGRITY_NOLABEL; > > > > > else if (rc == 0) > > > > > evm_status = INTEGRITY_NOXATTRS; /* new file */ > > > > > > > > > > If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr > > > > > can be written (status INTEGRITY_NOXATTRS is ok). After, > > > > > evm_find_protected_xattrs() returns rc > 0, so the status is > > > > > INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr(). > > > > > > > > With EVM HMAC enabled, as a result of writing the first protected > > > > xattr, an EVM HMAC should be calculated and written in > > > > evm_inode_post_setxattr(). > > > > > > To solve the ordering issue, wouldn't allowing setxattr() on a file > > > with portable signature that does not yet pass verification be safe? > > > evm_update_evmxattr() checks if the signature is portable and > > > if yes, does not calculate the HMAC. > > > > Before agreeing to allowing the protected xattrs to be written on a > > file with a portable signature that does not yet pass verification are > > safe, would we be introducing any new types of attacks? > > Allowing xattr/attr update means that someone can do: > > setxattr(path, "security.evm", ...); with type=5 > > all subsequent setxattr()/setattr() succeed until the correct > combination is set. > > At that point, any xattr/attr operation fails, even if one tries to set > an xattr with the same value. If we still deny the operation when the > verification succeeds, we have to fix that. > > It is common that the signature passes verification before user space > tools finish to set xattrs/attrs. For example, if a file is created with > mode 644 and this was the mode at the time of signing, any attempt > by tar for example to set again the same mode fails. We have a couple of options: always fail the write, differentiate the reason for the failure, or check the value before failing the write. The easiest obviously would be to always fail the write once it is valid. Whatever you decide, please keep it simple. > > If allowing a change of xattrs/attrs for portable signatures is safe or > not, I would say yes. Portable signatures cannot be modified even > if __vfs_setxattr_noperm() is called directly. > > > For example, would we differentiate between portable signatures that > > don't pass verification and ones that do? If we don't differentiate, > > could it be used for DoS? Should it be limited to new files? > > I would prefer to lock files when signatures pass the verification to > avoid accidental changes. > > Unless we find a better way to identify new file, without depending > on the appraisal policy, I would allow the operation even for existing > files. The existing code verifies the EVM xattr before allowing the xattr/attr change. It sounds like for EVM_XATTR_PORTABLE_DIGSIG we need to do exactly the reverse. Mimi
diff --git a/fs/attr.c b/fs/attr.c index b4bbdbd4c8ca..8f26d7d2e3b4 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -224,7 +224,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de { struct inode *inode = dentry->d_inode; umode_t mode = inode->i_mode; - int error; + int error, evm_error; struct timespec64 now; unsigned int ia_valid = attr->ia_valid; @@ -328,6 +328,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de error = security_inode_setattr(dentry, attr); if (error) return error; + evm_error = evm_inode_setattr(dentry, attr); + if (evm_error) + return evm_error; error = try_break_deleg(inode, delegated_inode); if (error) return error; diff --git a/fs/xattr.c b/fs/xattr.c index e13265e65871..3b323b75b741 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -183,6 +183,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name, fsnotify_xattr(dentry); security_inode_post_setxattr(dentry, name, value, size, flags); + evm_inode_post_setxattr(dentry, name, value, size); } } else { if (unlikely(is_bad_inode(inode))) @@ -210,7 +211,7 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size, int flags) { struct inode *inode = dentry->d_inode; - int error; + int error, evm_error; error = xattr_permission(inode, name, MAY_WRITE); if (error) @@ -221,6 +222,12 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value, if (error) goto out; + evm_error = evm_inode_setxattr(dentry, name, value, size); + if (evm_error) { + error = evm_error; + goto out; + } + error = __vfs_setxattr_noperm(dentry, name, value, size, flags); out: @@ -382,7 +389,7 @@ int vfs_removexattr(struct dentry *dentry, const char *name) { struct inode *inode = dentry->d_inode; - int error; + int error, evm_error; error = xattr_permission(inode, name, MAY_WRITE); if (error) @@ -393,6 +400,12 @@ vfs_removexattr(struct dentry *dentry, const char *name) if (error) goto out; + evm_error = evm_inode_removexattr(dentry, name); + if (evm_error) { + error = evm_error; + goto out; + } + error = __vfs_removexattr(dentry, name); if (!error) { diff --git a/security/security.c b/security/security.c index 7fed24b9d57e..e1368ab34cee 100644 --- a/security/security.c +++ b/security/security.c @@ -1255,14 +1255,9 @@ int security_inode_permission(struct inode *inode, int mask) int security_inode_setattr(struct dentry *dentry, struct iattr *attr) { - int ret; - if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) return 0; - ret = call_int_hook(inode_setattr, 0, dentry, attr); - if (ret) - return ret; - return evm_inode_setattr(dentry, attr); + return call_int_hook(inode_setattr, 0, dentry, attr); } EXPORT_SYMBOL_GPL(security_inode_setattr); @@ -1291,10 +1286,7 @@ int security_inode_setxattr(struct dentry *dentry, const char *name, ret = cap_inode_setxattr(dentry, name, value, size, flags); if (ret) return ret; - ret = ima_inode_setxattr(dentry, name, value, size); - if (ret) - return ret; - return evm_inode_setxattr(dentry, name, value, size); + return ima_inode_setxattr(dentry, name, value, size); } void security_inode_post_setxattr(struct dentry *dentry, const char *name, @@ -1303,7 +1295,6 @@ void security_inode_post_setxattr(struct dentry *dentry, const char *name, if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) return; call_void_hook(inode_post_setxattr, dentry, name, value, size, flags); - evm_inode_post_setxattr(dentry, name, value, size); } int security_inode_getxattr(struct dentry *dentry, const char *name) @@ -1335,10 +1326,7 @@ int security_inode_removexattr(struct dentry *dentry, const char *name) ret = cap_inode_removexattr(dentry, name); if (ret) return ret; - ret = ima_inode_removexattr(dentry, name); - if (ret) - return ret; - return evm_inode_removexattr(dentry, name); + return ima_inode_removexattr(dentry, name); } int security_inode_need_killpriv(struct dentry *dentry)
EVM is a module for the protection of the integrity of file metadata. It protects security-relevant extended attributes, and some file attributes such as the UID and the GID. It protects their integrity with an HMAC or with a signature. What makes EVM different from other LSMs is that it makes a security decision depending on multiple pieces of information, which cannot be managed atomically by the system. Example: cp -a file.orig file.dest If security.selinux, security.ima and security.evm must be preserved, cp will invoke setxattr() for each xattr, and EVM performs a verification during each operation. The problem is that copying security.evm from file.orig to file.dest will likely break the following EVM verifications if some metadata still have to be copied. EVM has no visibility on the metadata of the source file, so it cannot determine when the copy can be considered complete. On the other hand, EVM has to check metadata during every operation to ensure that there is no transition from corrupted metadata, e.g. after an offline attack, to valid ones after the operation. An HMAC update would prevent the corruption to be detected, as the HMAC on the new values would be correct. Thus, to avoid this issue, EVM has to return an error to the system call so that its execution will be interrupted. A solution that would satisfy both requirements, not breaking user space applications and detecting corrupted metadata is to let metadata operations be completed successfully and to pass the result of the EVM verification from the pre hooks to the post hooks. In this way, the HMAC update can be avoided if the verification wasn't successful. This approach will bring another important benefit: it is no longer required that every file has a valid HMAC or signature. Instead of always enforcing metadata integrity, even when it is not relevant for IMA, EVM will let IMA decide for files selected with the appraisal policy, depending on the result of the requested verification. The main problem is that the result of the verification currently cannot be passed from the pre hooks to the post hooks, due to how the LSM API is defined. A possible solution would be to use integrity_iint_cache for this purpose, but it will increase the memory pressure, as new structures will be allocated also for metadata operations, not only for measurement, appraisal and audit. Another solution would be to extend the LSM API, but it seems not worthwhile as EVM would be the only module getting a benefit from this change. Given that pre and post hooks are called from the same system call, a more efficient solution seems to move the hooks outside the LSM infrastructure, so that the return value of the pre hooks can be passed to the post hooks. A predefined error (-EAGAIN) will be used to signal to the system call to continue the execution. Otherwise, if the pre hooks return -EPERM, the system calls will behave as before and will immediately return before metadata are changed. Overview of the changes: evm_inode_init_security() LSM (no change) evm_inode_setxattr() LSM -> vfs_setxattr() evm_inode_post_setxattr() LSM -> vfs_setxattr() evm_inode_removexattr() LSM -> vfs_removexattr() evm_inode_post_removexattr() vfs_removexattr() (no change) evm_inode_setattr() LSM -> vfs_setattr() evm_inode_post_setattr() vfs_setattr() (no change) evm_verifyxattr() outside LSM (no change) Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- fs/attr.c | 5 ++++- fs/xattr.c | 17 +++++++++++++++-- security/security.c | 18 +++--------------- 3 files changed, 22 insertions(+), 18 deletions(-)