* [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded @ 2020-06-18 16:01 Roberto Sassu 2020-06-18 16:01 ` [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal Roberto Sassu ` (4 more replies) 0 siblings, 5 replies; 17+ messages in thread From: Roberto Sassu @ 2020-06-18 16:01 UTC (permalink / raw) To: zohar, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu, stable evm_inode_init_security() requires the HMAC key to calculate the HMAC on initial xattrs provided by LSMs. Unfortunately, with the evm_key_loaded() check, the function continues even if the HMAC key is not loaded (evm_key_loaded() returns true also if EVM has been initialized only with a public key). If the HMAC key is not loaded, evm_inode_init_security() returns an error later when it calls evm_init_hmac(). Thus, this patch replaces the evm_key_loaded() check with a check of the EVM_INIT_HMAC flag in evm_initialized, so that evm_inode_init_security() returns 0 instead of an error. Cc: stable@vger.kernel.org # 4.5.x Fixes: 26ddabfe96b ("evm: enable EVM when X509 certificate is loaded") Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/evm/evm_main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 0d36259b690d..744c105b48d1 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -521,7 +521,8 @@ int evm_inode_init_security(struct inode *inode, struct evm_xattr *xattr_data; int rc; - if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name)) + if (!(evm_initialized & EVM_INIT_HMAC) || + !evm_protected_xattr(lsm_xattr->name)) return 0; xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS); -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal 2020-06-18 16:01 [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded Roberto Sassu @ 2020-06-18 16:01 ` Roberto Sassu 2020-08-21 18:45 ` Mimi Zohar 2020-06-18 16:01 ` [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded Roberto Sassu ` (3 subsequent siblings) 4 siblings, 1 reply; 17+ messages in thread From: Roberto Sassu @ 2020-06-18 16:01 UTC (permalink / raw) To: zohar, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu Public keys do not need to be appraised by IMA as the restriction on the IMA/EVM keyrings ensures that a key is loaded only if it is signed with a key in the primary or secondary keyring. However, when evm_load_x509() is loaded, appraisal is already enabled and a valid IMA signature must be added to the EVM key to pass verification. Since the restriction is applied on both IMA and EVM keyrings, it is safe to disable appraisal also when the EVM key is loaded. This patch calls evm_load_x509() inside ima_load_x509() if CONFIG_IMA_LOAD_X509 is defined. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/iint.c | 2 ++ security/integrity/ima/ima_init.c | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/security/integrity/iint.c b/security/integrity/iint.c index e12c4900510f..4765a266ba96 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -212,7 +212,9 @@ int integrity_kernel_read(struct file *file, loff_t offset, void __init integrity_load_keys(void) { ima_load_x509(); +#ifndef CONFIG_IMA_LOAD_X509 evm_load_x509(); +#endif } static int __init integrity_fs_init(void) diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 4902fe7bd570..9d29a1680da8 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -106,6 +106,10 @@ void __init ima_load_x509(void) ima_policy_flag &= ~unset_flags; integrity_load_x509(INTEGRITY_KEYRING_IMA, CONFIG_IMA_X509_PATH); + + /* load also EVM key to avoid appraisal */ + evm_load_x509(); + ima_policy_flag |= unset_flags; } #endif -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal 2020-06-18 16:01 ` [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal Roberto Sassu @ 2020-08-21 18:45 ` Mimi Zohar 2020-08-31 9:44 ` Roberto Sassu 0 siblings, 1 reply; 17+ messages in thread From: Mimi Zohar @ 2020-08-21 18:45 UTC (permalink / raw) To: Roberto Sassu, mjg59; +Cc: linux-integrity, linux-security-module, linux-kernel On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote: > Public keys do not need to be appraised by IMA as the restriction on the > IMA/EVM keyrings ensures that a key is loaded only if it is signed with a > key in the primary or secondary keyring. > > However, when evm_load_x509() is loaded, appraisal is already enabled and > a valid IMA signature must be added to the EVM key to pass verification. > > Since the restriction is applied on both IMA and EVM keyrings, it is safe > to disable appraisal also when the EVM key is loaded. This patch calls > evm_load_x509() inside ima_load_x509() if CONFIG_IMA_LOAD_X509 is defined. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > security/integrity/iint.c | 2 ++ > security/integrity/ima/ima_init.c | 4 ++++ > 2 files changed, 6 insertions(+) > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c > index e12c4900510f..4765a266ba96 100644 > --- a/security/integrity/iint.c > +++ b/security/integrity/iint.c > @@ -212,7 +212,9 @@ int integrity_kernel_read(struct file *file, loff_t offset, > void __init integrity_load_keys(void) > { > ima_load_x509(); > +#ifndef CONFIG_IMA_LOAD_X509 > evm_load_x509(); > +#endif > } > > static int __init integrity_fs_init(void) > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c > index 4902fe7bd570..9d29a1680da8 100644 > --- a/security/integrity/ima/ima_init.c > +++ b/security/integrity/ima/ima_init.c > @@ -106,6 +106,10 @@ void __init ima_load_x509(void) > > ima_policy_flag &= ~unset_flags; > integrity_load_x509(INTEGRITY_KEYRING_IMA, CONFIG_IMA_X509_PATH); > + > + /* load also EVM key to avoid appraisal */ > + evm_load_x509(); > + > ima_policy_flag |= unset_flags; > } > #endif As much as possible IMA and EVM should remain independent of each other. Modifying integrity_load_x509() doesn't help. This looks like a good reason for calling another EVM function from within IMA. Mimi ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal 2020-08-21 18:45 ` Mimi Zohar @ 2020-08-31 9:44 ` Roberto Sassu 2020-08-31 19:26 ` Mimi Zohar 0 siblings, 1 reply; 17+ messages in thread From: Roberto Sassu @ 2020-08-31 9:44 UTC (permalink / raw) To: Mimi Zohar, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, Silviu Vlasceanu > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Friday, August 21, 2020 8:45 PM > On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote: > > Public keys do not need to be appraised by IMA as the restriction on the > > IMA/EVM keyrings ensures that a key is loaded only if it is signed with a > > key in the primary or secondary keyring. > > > > However, when evm_load_x509() is loaded, appraisal is already enabled > and > > a valid IMA signature must be added to the EVM key to pass verification. > > > > Since the restriction is applied on both IMA and EVM keyrings, it is safe > > to disable appraisal also when the EVM key is loaded. This patch calls > > evm_load_x509() inside ima_load_x509() if CONFIG_IMA_LOAD_X509 is > defined. > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > --- > > security/integrity/iint.c | 2 ++ > > security/integrity/ima/ima_init.c | 4 ++++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c > > index e12c4900510f..4765a266ba96 100644 > > --- a/security/integrity/iint.c > > +++ b/security/integrity/iint.c > > @@ -212,7 +212,9 @@ int integrity_kernel_read(struct file *file, loff_t > offset, > > void __init integrity_load_keys(void) > > { > > ima_load_x509(); > > +#ifndef CONFIG_IMA_LOAD_X509 > > evm_load_x509(); > > +#endif > > } > > > > static int __init integrity_fs_init(void) > > diff --git a/security/integrity/ima/ima_init.c > b/security/integrity/ima/ima_init.c > > index 4902fe7bd570..9d29a1680da8 100644 > > --- a/security/integrity/ima/ima_init.c > > +++ b/security/integrity/ima/ima_init.c > > @@ -106,6 +106,10 @@ void __init ima_load_x509(void) > > > > ima_policy_flag &= ~unset_flags; > > integrity_load_x509(INTEGRITY_KEYRING_IMA, > CONFIG_IMA_X509_PATH); > > + > > + /* load also EVM key to avoid appraisal */ > > + evm_load_x509(); > > + > > ima_policy_flag |= unset_flags; > > } > > #endif > > As much as possible IMA and EVM should remain independent of each > other. Modifying integrity_load_x509() doesn't help. This looks like > a good reason for calling another EVM function from within IMA. Can I add your Reviewed-by? Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal 2020-08-31 9:44 ` Roberto Sassu @ 2020-08-31 19:26 ` Mimi Zohar 0 siblings, 0 replies; 17+ messages in thread From: Mimi Zohar @ 2020-08-31 19:26 UTC (permalink / raw) To: Roberto Sassu, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, Silviu Vlasceanu On Mon, 2020-08-31 at 09:44 +0000, Roberto Sassu wrote: > > > As much as possible IMA and EVM should remain independent of each > > other. Modifying integrity_load_x509() doesn't help. This looks like > > a good reason for calling another EVM function from within IMA. > > Can I add your Reviewed-by? Yes, that's fine. Mimi ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded 2020-06-18 16:01 [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded Roberto Sassu 2020-06-18 16:01 ` [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal Roberto Sassu @ 2020-06-18 16:01 ` Roberto Sassu 2020-08-21 20:14 ` Mimi Zohar 2020-06-18 16:01 ` [PATCH 04/11] evm: Check size of security.evm before using it Roberto Sassu ` (2 subsequent siblings) 4 siblings, 1 reply; 17+ messages in thread From: Roberto Sassu @ 2020-06-18 16:01 UTC (permalink / raw) To: zohar, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu, stable Granting metadata write is safe if the HMAC key is not loaded, as it won't let an attacker obtain a valid HMAC from corrupted xattrs. evm_write_key() however does not allow it if any key is loaded, including a public key, which should not be a problem. This patch allows setting EVM_ALLOW_METADATA_WRITES if the EVM_INIT_HMAC flag is not set. Cc: stable@vger.kernel.org # 4.16.x Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata") Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/evm/evm_secfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c index cfc3075769bb..92fe26ace797 100644 --- a/security/integrity/evm/evm_secfs.c +++ b/security/integrity/evm/evm_secfs.c @@ -84,7 +84,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf, * keys are loaded. */ if ((i & EVM_ALLOW_METADATA_WRITES) && - ((evm_initialized & EVM_KEY_MASK) != 0) && + ((evm_initialized & EVM_INIT_HMAC) != 0) && !(evm_initialized & EVM_ALLOW_METADATA_WRITES)) return -EPERM; -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded 2020-06-18 16:01 ` [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded Roberto Sassu @ 2020-08-21 20:14 ` Mimi Zohar 2020-08-31 8:24 ` Roberto Sassu 0 siblings, 1 reply; 17+ messages in thread From: Mimi Zohar @ 2020-08-21 20:14 UTC (permalink / raw) To: Roberto Sassu, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, stable Hi Roberto, On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote: > Granting metadata write is safe if the HMAC key is not loaded, as it won't > let an attacker obtain a valid HMAC from corrupted xattrs. evm_write_key() > however does not allow it if any key is loaded, including a public key, > which should not be a problem. > Why is the existing hebavior a problem? What is the problem being solved? > This patch allows setting EVM_ALLOW_METADATA_WRITES if the EVM_INIT_HMAC > flag is not set. > > Cc: stable@vger.kernel.org # 4.16.x > Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata") > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > security/integrity/evm/evm_secfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c > index cfc3075769bb..92fe26ace797 100644 > --- a/security/integrity/evm/evm_secfs.c > +++ b/security/integrity/evm/evm_secfs.c > @@ -84,7 +84,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf, > * keys are loaded. > */ > if ((i & EVM_ALLOW_METADATA_WRITES) && > - ((evm_initialized & EVM_KEY_MASK) != 0) && > + ((evm_initialized & EVM_INIT_HMAC) != 0) && > !(evm_initialized & EVM_ALLOW_METADATA_WRITES)) > return -EPERM; > Documentation/ABI/testing/evm needs to be updated as well. thanks, Mimi ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded 2020-08-21 20:14 ` Mimi Zohar @ 2020-08-31 8:24 ` Roberto Sassu 2020-08-31 21:31 ` Mimi Zohar 0 siblings, 1 reply; 17+ messages in thread From: Roberto Sassu @ 2020-08-31 8:24 UTC (permalink / raw) To: Mimi Zohar, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, stable, Silviu Vlasceanu > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Friday, August 21, 2020 10:15 PM > Hi Roberto, > > On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote: > > Granting metadata write is safe if the HMAC key is not loaded, as it won't > > let an attacker obtain a valid HMAC from corrupted xattrs. > evm_write_key() > > however does not allow it if any key is loaded, including a public key, > > which should not be a problem. > > > > Why is the existing hebavior a problem? What is the problem being > solved? Hi Mimi currently it is not possible to set EVM_ALLOW_METADATA_WRITES when only a public key is loaded and the HMAC key is not. The patch removes this limitation. > > This patch allows setting EVM_ALLOW_METADATA_WRITES if the > EVM_INIT_HMAC > > flag is not set. > > > > Cc: stable@vger.kernel.org # 4.16.x > > Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of > EVM-protected metadata") > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > --- > > security/integrity/evm/evm_secfs.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/security/integrity/evm/evm_secfs.c > b/security/integrity/evm/evm_secfs.c > > index cfc3075769bb..92fe26ace797 100644 > > --- a/security/integrity/evm/evm_secfs.c > > +++ b/security/integrity/evm/evm_secfs.c > > @@ -84,7 +84,7 @@ static ssize_t evm_write_key(struct file *file, const > char __user *buf, > > * keys are loaded. > > */ > > if ((i & EVM_ALLOW_METADATA_WRITES) && > > - ((evm_initialized & EVM_KEY_MASK) != 0) && > > + ((evm_initialized & EVM_INIT_HMAC) != 0) && > > !(evm_initialized & EVM_ALLOW_METADATA_WRITES)) > > return -EPERM; > > > > > Documentation/ABI/testing/evm needs to be updated as well. Ok. Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > thanks, > > Mimi > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded 2020-08-31 8:24 ` Roberto Sassu @ 2020-08-31 21:31 ` Mimi Zohar 0 siblings, 0 replies; 17+ messages in thread From: Mimi Zohar @ 2020-08-31 21:31 UTC (permalink / raw) To: Roberto Sassu, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, stable, Silviu Vlasceanu On Mon, 2020-08-31 at 08:24 +0000, Roberto Sassu wrote: > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > Sent: Friday, August 21, 2020 10:15 PM > > Hi Roberto, > > > > On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote: > > > Granting metadata write is safe if the HMAC key is not loaded, as it won't > > > let an attacker obtain a valid HMAC from corrupted xattrs. > > evm_write_key() > > > however does not allow it if any key is loaded, including a public key, > > > which should not be a problem. > > > > > > > Why is the existing hebavior a problem? What is the problem being > > solved? > > Hi Mimi > > currently it is not possible to set EVM_ALLOW_METADATA_WRITES when > only a public key is loaded and the HMAC key is not. The patch removes > this limitation. Yes, I understand. You're describing "what" the problem is, not "why" this is a problem. Support for loading EVM HMAC and x509 certificates isn't new. Please add a line or two prior to this paragraph providing the context for why this is now a problem. Is the problem related to previoulsy not beginning EVM verification until after the EVM HMAC key was loaded? Or perhaps EVM signatures were not that common since they weren't portable. Now, with portable and immutable signatures loading x509 certificates is more common. thanks, Mimi ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 04/11] evm: Check size of security.evm before using it 2020-06-18 16:01 [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded Roberto Sassu 2020-06-18 16:01 ` [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal Roberto Sassu 2020-06-18 16:01 ` [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded Roberto Sassu @ 2020-06-18 16:01 ` Roberto Sassu 2020-08-24 12:14 ` Mimi Zohar 2020-06-18 16:01 ` [PATCH 05/11] evm: Allow xattr/attr operations for portable signatures if check fails Roberto Sassu 2020-08-21 18:30 ` [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded Mimi Zohar 4 siblings, 1 reply; 17+ messages in thread From: Roberto Sassu @ 2020-06-18 16:01 UTC (permalink / raw) To: zohar, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu, stable This patch checks the size for the EVM_IMA_XATTR_DIGSIG and EVM_XATTR_PORTABLE_DIGSIG types to ensure that the algorithm is read from the buffer returned by vfs_getxattr_alloc(). Cc: stable@vger.kernel.org # 4.19.x Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures") Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/evm/evm_main.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 744c105b48d1..4e9f5e8b21d5 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -181,6 +181,12 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, break; case EVM_IMA_XATTR_DIGSIG: case EVM_XATTR_PORTABLE_DIGSIG: + /* accept xattr with non-empty signature field */ + if (xattr_len <= sizeof(struct signature_v2_hdr)) { + evm_status = INTEGRITY_FAIL; + goto out; + } + hdr = (struct signature_v2_hdr *)xattr_data; digest.hdr.algo = hdr->hash_algo; rc = evm_calc_hash(dentry, xattr_name, xattr_value, -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 04/11] evm: Check size of security.evm before using it 2020-06-18 16:01 ` [PATCH 04/11] evm: Check size of security.evm before using it Roberto Sassu @ 2020-08-24 12:14 ` Mimi Zohar 0 siblings, 0 replies; 17+ messages in thread From: Mimi Zohar @ 2020-08-24 12:14 UTC (permalink / raw) To: Roberto Sassu, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, stable On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote: > This patch checks the size for the EVM_IMA_XATTR_DIGSIG and > EVM_XATTR_PORTABLE_DIGSIG types to ensure that the algorithm is read from > the buffer returned by vfs_getxattr_alloc(). > > Cc: stable@vger.kernel.org # 4.19.x > Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures") > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 05/11] evm: Allow xattr/attr operations for portable signatures if check fails 2020-06-18 16:01 [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded Roberto Sassu ` (2 preceding siblings ...) 2020-06-18 16:01 ` [PATCH 04/11] evm: Check size of security.evm before using it Roberto Sassu @ 2020-06-18 16:01 ` Roberto Sassu 2020-08-24 12:16 ` Mimi Zohar 2020-08-21 18:30 ` [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded Mimi Zohar 4 siblings, 1 reply; 17+ messages in thread From: Roberto Sassu @ 2020-06-18 16:01 UTC (permalink / raw) To: zohar, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu If files with portable signatures are copied from one location to another or are extracted from an archive, verification can temporarily fail until all xattrs/attrs are set in the destination. Portable signatures are the only ones that can be moved to different files, as they don't depend on system-specific information such as the inode generation. Unlike other security.evm types, portable signatures can never be replaced even if an xattr/attr operation is granted, as once evm_update_evmxattr() detects this type, it returns without updating the HMAC. Thus, it wouldn't be a problem to allow those operations so that verification passes on the destination after all xattrs/attrs are copied. This patch first introduces a new integrity status called INTEGRITY_FAIL_IMMUTABLE, that allows callers of evm_verify_current_integrity() to detect that a portable signature didn't pass verification and then adds an exception in evm_protect_xattr() and evm_inode_setattr() for this status and returns 0 instead of -EPERM. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- include/linux/integrity.h | 1 + security/integrity/evm/evm_main.c | 25 ++++++++++++++++++++----- security/integrity/ima/ima_appraise.c | 1 + 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/include/linux/integrity.h b/include/linux/integrity.h index 2271939c5c31..2ea0f2f65ab6 100644 --- a/include/linux/integrity.h +++ b/include/linux/integrity.h @@ -13,6 +13,7 @@ enum integrity_status { INTEGRITY_PASS = 0, INTEGRITY_PASS_IMMUTABLE, INTEGRITY_FAIL, + INTEGRITY_FAIL_IMMUTABLE, INTEGRITY_NOLABEL, INTEGRITY_NOXATTRS, INTEGRITY_UNKNOWN, diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 4e9f5e8b21d5..30072030f05d 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -27,7 +27,8 @@ int evm_initialized; static const char * const integrity_status_msg[] = { - "pass", "pass_immutable", "fail", "no_label", "no_xattrs", "unknown" + "pass", "pass_immutable", "fail", "fail_immutable", "no_label", + "no_xattrs", "unknown" }; int evm_hmac_attrs; @@ -134,7 +135,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, enum integrity_status evm_status = INTEGRITY_PASS; struct evm_digest digest; struct inode *inode; - int rc, xattr_len; + int rc, xattr_len, evm_immutable = 0; if (iint && (iint->evm_status == INTEGRITY_PASS || iint->evm_status == INTEGRITY_PASS_IMMUTABLE)) @@ -179,8 +180,10 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, if (rc) rc = -EINVAL; break; - case EVM_IMA_XATTR_DIGSIG: case EVM_XATTR_PORTABLE_DIGSIG: + evm_immutable = 1; + fallthrough; + case EVM_IMA_XATTR_DIGSIG: /* accept xattr with non-empty signature field */ if (xattr_len <= sizeof(struct signature_v2_hdr)) { evm_status = INTEGRITY_FAIL; @@ -219,7 +222,8 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, if (rc) evm_status = (rc == -ENODATA) ? - INTEGRITY_NOXATTRS : INTEGRITY_FAIL; + INTEGRITY_NOXATTRS : evm_immutable ? + INTEGRITY_FAIL_IMMUTABLE : INTEGRITY_FAIL; out: if (iint) iint->evm_status = evm_status; @@ -351,6 +355,12 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name, -EPERM, 0); } out: + /* It is safe to allow fail_immutable, portable signatures can never be + * updated + */ + if (evm_status == INTEGRITY_FAIL_IMMUTABLE) + return 0; + if (evm_status != INTEGRITY_PASS) integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), dentry->d_name.name, "appraise_metadata", @@ -488,9 +498,14 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr) if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))) return 0; evm_status = evm_verify_current_integrity(dentry); + /* It is safe to allow fail_immutable, portable signatures can never + * be updated + */ if ((evm_status == INTEGRITY_PASS) || - (evm_status == INTEGRITY_NOXATTRS)) + (evm_status == INTEGRITY_NOXATTRS) || + (evm_status == INTEGRITY_FAIL_IMMUTABLE)) return 0; + integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), dentry->d_name.name, "appraise_metadata", integrity_status_msg[evm_status], -EPERM, 0); diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index a9649b04b9f1..21bda264fc30 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -393,6 +393,7 @@ int ima_appraise_measurement(enum ima_hooks func, case INTEGRITY_NOLABEL: /* No security.evm xattr. */ cause = "missing-HMAC"; goto out; + case INTEGRITY_FAIL_IMMUTABLE: case INTEGRITY_FAIL: /* Invalid HMAC/signature. */ cause = "invalid-HMAC"; goto out; -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 05/11] evm: Allow xattr/attr operations for portable signatures if check fails 2020-06-18 16:01 ` [PATCH 05/11] evm: Allow xattr/attr operations for portable signatures if check fails Roberto Sassu @ 2020-08-24 12:16 ` Mimi Zohar 0 siblings, 0 replies; 17+ messages in thread From: Mimi Zohar @ 2020-08-24 12:16 UTC (permalink / raw) To: Roberto Sassu, mjg59; +Cc: linux-integrity, linux-security-module, linux-kernel On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote: > If files with portable signatures are copied from one location to another > or are extracted from an archive, verification can temporarily fail until > all xattrs/attrs are set in the destination. Portable signatures are the > only ones that can be moved to different files, as they don't depend on > system-specific information such as the inode generation. ^Only portable signatures may be moved or copied from one file to another, as they ... Instead portable signatures must include "security.ima". > Unlike other security.evm types, portable signatures ^, EVM portable signatures are also immutable. They > can never be replaced > even if an xattr/attr operation is granted, as once evm_update_evmxattr() > detects this type, it returns without updating the HMAC. Thus, it wouldn't > be a problem to allow those operations so that verification passes on the > destination after all xattrs/attrs are copied. This needs to be reworded a bit. > > This patch first introduces a new integrity status called > INTEGRITY_FAIL_IMMUTABLE, that allows callers of > evm_verify_current_integrity() to detect that a portable signature didn't > pass verification and then adds an exception in evm_protect_xattr() and > evm_inode_setattr() for this status and returns 0 instead of -EPERM. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> After this patch, nothing prevents modifying the xattrs after all of them are in place and the signature verification would be successful. (Ok, that is being addressed in subsequent patches.) > --- > include/linux/integrity.h | 1 + > security/integrity/evm/evm_main.c | 25 ++++++++++++++++++++----- > security/integrity/ima/ima_appraise.c | 1 + > 3 files changed, 22 insertions(+), 5 deletions(-) > > diff --git a/include/linux/integrity.h b/include/linux/integrity.h > index 2271939c5c31..2ea0f2f65ab6 100644 > --- a/include/linux/integrity.h > +++ b/include/linux/integrity.h > @@ -13,6 +13,7 @@ enum integrity_status { > INTEGRITY_PASS = 0, > INTEGRITY_PASS_IMMUTABLE, > INTEGRITY_FAIL, > + INTEGRITY_FAIL_IMMUTABLE, > INTEGRITY_NOLABEL, > INTEGRITY_NOXATTRS, > INTEGRITY_UNKNOWN, > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c > index 4e9f5e8b21d5..30072030f05d 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -27,7 +27,8 @@ > int evm_initialized; > > static const char * const integrity_status_msg[] = { > - "pass", "pass_immutable", "fail", "no_label", "no_xattrs", "unknown" > + "pass", "pass_immutable", "fail", "fail_immutable", "no_label", > + "no_xattrs", "unknown" > }; > int evm_hmac_attrs; > > @@ -134,7 +135,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, > enum integrity_status evm_status = INTEGRITY_PASS; > struct evm_digest digest; > struct inode *inode; > - int rc, xattr_len; > + int rc, xattr_len, evm_immutable = 0; > > if (iint && (iint->evm_status == INTEGRITY_PASS || > iint->evm_status == INTEGRITY_PASS_IMMUTABLE)) > @@ -179,8 +180,10 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, > if (rc) > rc = -EINVAL; > break; > - case EVM_IMA_XATTR_DIGSIG: > case EVM_XATTR_PORTABLE_DIGSIG: > + evm_immutable = 1; > + fallthrough; > + case EVM_IMA_XATTR_DIGSIG: > /* accept xattr with non-empty signature field */ > if (xattr_len <= sizeof(struct signature_v2_hdr)) { > evm_status = INTEGRITY_FAIL; > @@ -219,7 +222,8 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, > > if (rc) > evm_status = (rc == -ENODATA) ? > - INTEGRITY_NOXATTRS : INTEGRITY_FAIL; > + INTEGRITY_NOXATTRS : evm_immutable ? > + INTEGRITY_FAIL_IMMUTABLE : INTEGRITY_FAIL; Embedded ternary operator should be replaced with normal C syntax. > out: > if (iint) > iint->evm_status = evm_status; > @@ -351,6 +355,12 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name, > -EPERM, 0); > } > out: > + /* It is safe to allow fail_immutable, portable signatures can never be > + * updated > + */ Replace "It" with "Writing other xattrs". Writing other xattrs is safe for portable signatures, as portable signatures are immutable and ...." > + if (evm_status == INTEGRITY_FAIL_IMMUTABLE) > + return 0; > + > if (evm_status != INTEGRITY_PASS) > integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), > dentry->d_name.name, "appraise_metadata", > @@ -488,9 +498,14 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr) > if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))) > return 0; > evm_status = evm_verify_current_integrity(dentry); > + /* It is safe to allow fail_immutable, portable signatures can never > + * be updated > + */ Replace "It" with what is safe. Mimi > if ((evm_status == INTEGRITY_PASS) || > - (evm_status == INTEGRITY_NOXATTRS)) > + (evm_status == INTEGRITY_NOXATTRS) || > + (evm_status == INTEGRITY_FAIL_IMMUTABLE)) > return 0; > + > integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), > dentry->d_name.name, "appraise_metadata", > integrity_status_msg[evm_status], -EPERM, 0); > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index a9649b04b9f1..21bda264fc30 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -393,6 +393,7 @@ int ima_appraise_measurement(enum ima_hooks func, > case INTEGRITY_NOLABEL: /* No security.evm xattr. */ > cause = "missing-HMAC"; > goto out; > + case INTEGRITY_FAIL_IMMUTABLE: > case INTEGRITY_FAIL: /* Invalid HMAC/signature. */ > cause = "invalid-HMAC"; > goto out; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded 2020-06-18 16:01 [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded Roberto Sassu ` (3 preceding siblings ...) 2020-06-18 16:01 ` [PATCH 05/11] evm: Allow xattr/attr operations for portable signatures if check fails Roberto Sassu @ 2020-08-21 18:30 ` Mimi Zohar 2020-08-24 17:45 ` Mimi Zohar 4 siblings, 1 reply; 17+ messages in thread From: Mimi Zohar @ 2020-08-21 18:30 UTC (permalink / raw) To: Roberto Sassu, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, stable Hi Roberto, Sorry for the delay in reviewing these patches. Missing from this patch set is a cover letter with an explanation for grouping these patches into a patch set, other than for convenience. In this case, it would be along the lines that the original use case for EVM portable and immutable keys support was for a few critical files, not combined with an EVM encrypted key type. This patch set more fully integrates the initial EVM portable and immutable signature support. On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote: > evm_inode_init_security() requires the HMAC key to calculate the HMAC on > initial xattrs provided by LSMs. Unfortunately, with the evm_key_loaded() > check, the function continues even if the HMAC key is not loaded > (evm_key_loaded() returns true also if EVM has been initialized only with a > public key). If the HMAC key is not loaded, evm_inode_init_security() > returns an error later when it calls evm_init_hmac(). > > Thus, this patch replaces the evm_key_loaded() check with a check of the > EVM_INIT_HMAC flag in evm_initialized, so that evm_inode_init_security() > returns 0 instead of an error. > > Cc: stable@vger.kernel.org # 4.5.x > Fixes: 26ddabfe96b ("evm: enable EVM when X509 certificate is loaded") > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > --- > security/integrity/evm/evm_main.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c > index 0d36259b690d..744c105b48d1 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -521,7 +521,8 @@ int evm_inode_init_security(struct inode *inode, > struct evm_xattr *xattr_data; > int rc; > > - if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name)) > + if (!(evm_initialized & EVM_INIT_HMAC) || > + !evm_protected_xattr(lsm_xattr->name)) > return 0; > > xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS); ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded 2020-08-21 18:30 ` [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded Mimi Zohar @ 2020-08-24 17:45 ` Mimi Zohar 2020-09-02 11:42 ` Roberto Sassu 0 siblings, 1 reply; 17+ messages in thread From: Mimi Zohar @ 2020-08-24 17:45 UTC (permalink / raw) To: Roberto Sassu, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, stable Hi Roberto, On Fri, 2020-08-21 at 14:30 -0400, Mimi Zohar wrote: > Sorry for the delay in reviewing these patches. Missing from this > patch set is a cover letter with an explanation for grouping these > patches into a patch set, other than for convenience. In this case, it > would be along the lines that the original use case for EVM portable > and immutable keys support was for a few critical files, not combined > with an EVM encrypted key type. This patch set more fully integrates > the initial EVM portable and immutable signature support. Thank you for more fully integrating the EVM portable signatures into IMA. " [PATCH 08/11] ima: Allow imasig requirement to be satisfied by EVM portable signatures" equates an IMA signature to having a portable and immutable EVM signature. That is true in terms of signature verification, but from an attestation perspective the "ima-sig" template will not contain a signature. If not the EVM signature, then at least some other indication should be included in the measurement list. Are you planning on posting the associated IMA/EVM regression tests? Mimi ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded 2020-08-24 17:45 ` Mimi Zohar @ 2020-09-02 11:42 ` Roberto Sassu 2020-09-02 13:40 ` Mimi Zohar 0 siblings, 1 reply; 17+ messages in thread From: Roberto Sassu @ 2020-09-02 11:42 UTC (permalink / raw) To: Mimi Zohar, mjg59 Cc: linux-integrity, linux-security-module, linux-kernel, stable, Silviu Vlasceanu > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Monday, August 24, 2020 7:45 PM > Hi Roberto, > > On Fri, 2020-08-21 at 14:30 -0400, Mimi Zohar wrote: > > Sorry for the delay in reviewing these patches. Missing from this > > patch set is a cover letter with an explanation for grouping these > > patches into a patch set, other than for convenience. In this case, it > > would be along the lines that the original use case for EVM portable > > and immutable keys support was for a few critical files, not combined > > with an EVM encrypted key type. This patch set more fully integrates > > the initial EVM portable and immutable signature support. > > Thank you for more fully integrating the EVM portable signatures into > IMA. > > " [PATCH 08/11] ima: Allow imasig requirement to be satisfied by EVM > portable signatures" equates an IMA signature to having a portable and > immutable EVM signature. That is true in terms of signature > verification, but from an attestation perspective the "ima-sig" > template will not contain a signature. If not the EVM signature, then > at least some other indication should be included in the measurement > list. Would it be ok to print the EVM portable signature in the sig field if the IMA signature is not found? Later we can introduce the new template evm-sig to include all metadata necessary to verify the EVM portable signature. > Are you planning on posting the associated IMA/EVM regression tests? I didn't have a look yet at the code. I will try to write some later. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded 2020-09-02 11:42 ` Roberto Sassu @ 2020-09-02 13:40 ` Mimi Zohar 0 siblings, 0 replies; 17+ messages in thread From: Mimi Zohar @ 2020-09-02 13:40 UTC (permalink / raw) To: Roberto Sassu, mjg59, Petr Vorel Cc: linux-integrity, linux-security-module, linux-kernel, stable, Silviu Vlasceanu On Wed, 2020-09-02 at 11:42 +0000, Roberto Sassu wrote: > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > Sent: Monday, August 24, 2020 7:45 PM > > Hi Roberto, > > > > On Fri, 2020-08-21 at 14:30 -0400, Mimi Zohar wrote: > > > Sorry for the delay in reviewing these patches. Missing from this > > > patch set is a cover letter with an explanation for grouping these > > > patches into a patch set, other than for convenience. In this case, it > > > would be along the lines that the original use case for EVM portable > > > and immutable keys support was for a few critical files, not combined > > > with an EVM encrypted key type. This patch set more fully integrates > > > the initial EVM portable and immutable signature support. > > > > Thank you for more fully integrating the EVM portable signatures into > > IMA. > > > > " [PATCH 08/11] ima: Allow imasig requirement to be satisfied by EVM > > portable signatures" equates an IMA signature to having a portable and > > immutable EVM signature. That is true in terms of signature > > verification, but from an attestation perspective the "ima-sig" > > template will not contain a signature. If not the EVM signature, then > > at least some other indication should be included in the measurement > > list. > > Would it be ok to print the EVM portable signature in the sig field if the IMA > signature is not found? Later we can introduce the new template evm-sig > to include all metadata necessary to verify the EVM portable signature. As long as the attestation server can differentiate between the signature types, including the EVM signature should be fine. > > > Are you planning on posting the associated IMA/EVM regression tests? > > I didn't have a look yet at the code. I will try to write some later. It looks like ima_verify_signature() in ima-evm-utils could be extended to support the EVM portable signature or at least not to fail the signature verification. Mimi ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-09-02 13:46 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-18 16:01 [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded Roberto Sassu 2020-06-18 16:01 ` [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal Roberto Sassu 2020-08-21 18:45 ` Mimi Zohar 2020-08-31 9:44 ` Roberto Sassu 2020-08-31 19:26 ` Mimi Zohar 2020-06-18 16:01 ` [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded Roberto Sassu 2020-08-21 20:14 ` Mimi Zohar 2020-08-31 8:24 ` Roberto Sassu 2020-08-31 21:31 ` Mimi Zohar 2020-06-18 16:01 ` [PATCH 04/11] evm: Check size of security.evm before using it Roberto Sassu 2020-08-24 12:14 ` Mimi Zohar 2020-06-18 16:01 ` [PATCH 05/11] evm: Allow xattr/attr operations for portable signatures if check fails Roberto Sassu 2020-08-24 12:16 ` Mimi Zohar 2020-08-21 18:30 ` [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded Mimi Zohar 2020-08-24 17:45 ` Mimi Zohar 2020-09-02 11:42 ` Roberto Sassu 2020-09-02 13:40 ` Mimi Zohar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).