* [PATCH v3 0/2] ima/evm fixes for v5.2 @ 2019-06-06 11:26 Roberto Sassu 2019-06-06 11:26 ` [PATCH v3 1/2] evm: add option to set a random HMAC key at early boot Roberto Sassu ` (3 more replies) 0 siblings, 4 replies; 21+ messages in thread From: Roberto Sassu @ 2019-06-06 11:26 UTC (permalink / raw) To: zohar, dmitry.kasatkin, mjg59 Cc: linux-integrity, linux-security-module, linux-doc, stable, linux-kernel, silviu.vlasceanu, Roberto Sassu Previous versions included the patch 'ima: don't ignore INTEGRITY_UNKNOWN EVM status'. However, I realized that this patch cannot be accepted alone because IMA-Appraisal would deny access to new files created during the boot. With the current behavior, those files are accessible because they have a valid security.ima (not protected by EVM) created after the first write. A solution for this problem is to initialize EVM very early with a random key. Access to created files will be granted, even with the strict appraisal, because after the first write those files will have both security.ima and security.evm (HMAC calculated with the random key). Strict appraisal will work only if it is done with signatures until the persistent HMAC key is loaded. Roberto Sassu (2): evm: add option to set a random HMAC key at early boot ima: add enforce-evm and log-evm modes to strictly check EVM status .../admin-guide/kernel-parameters.txt | 11 ++-- security/integrity/evm/evm.h | 10 +++- security/integrity/evm/evm_crypto.c | 57 ++++++++++++++++--- security/integrity/evm/evm_main.c | 41 ++++++++++--- security/integrity/ima/ima_appraise.c | 8 +++ security/integrity/integrity.h | 1 + 6 files changed, 106 insertions(+), 22 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 1/2] evm: add option to set a random HMAC key at early boot 2019-06-06 11:26 [PATCH v3 0/2] ima/evm fixes for v5.2 Roberto Sassu @ 2019-06-06 11:26 ` Roberto Sassu 2019-06-06 11:26 ` [PATCH v3 2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status Roberto Sassu ` (2 subsequent siblings) 3 siblings, 0 replies; 21+ messages in thread From: Roberto Sassu @ 2019-06-06 11:26 UTC (permalink / raw) To: zohar, dmitry.kasatkin, mjg59 Cc: linux-integrity, linux-security-module, linux-doc, stable, linux-kernel, silviu.vlasceanu, Roberto Sassu Mutable files can be created before the HMAC key is unsealed, for example the dracut state and the systemd journal. Next accesses to those files will be denied if the new appraisal mode enforce-evm is selected (INTEGRITY_UNKNOWN returned by EVM is considered as an error). This patch solves this problem by initializing EVM at early boot with a randomly generated key. This key is used to calculate and verify the HMAC for new files in a tmpfs filesystem, until the persistent key is loaded. The new xattr type EVM_XATTR_HMAC_RND_KEY has been introduced to determine which key should be used to verify the HMAC. This type is used for new files and file updates (unless security.evm exists with a different type), until the persistent key is loaded. Afterwards, existing HMACs calculated with the random key are replaced with HMACs calculated with the persistent key. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- .../admin-guide/kernel-parameters.txt | 8 ++- security/integrity/evm/evm.h | 10 +++- security/integrity/evm/evm_crypto.c | 57 ++++++++++++++++--- security/integrity/evm/evm_main.c | 41 ++++++++++--- security/integrity/integrity.h | 1 + 5 files changed, 96 insertions(+), 21 deletions(-) diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 138f6664b2e2..fe5cde58c11b 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1239,9 +1239,11 @@ has equivalent usage. See its documentation for details. evm= [EVM] - Format: { "fix" } - Permit 'security.evm' to be updated regardless of - current integrity status. + Format: { "fix" | "random" } + Specify "fix" to permit 'security.evm' to be updated + regardless of current integrity status. Specify "random" + to initialize EVM with a random key to be used for new + files until the persistent HMAC key is loaded. failslab= fail_page_alloc= diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h index c3f437f5db10..0ca4490b7e40 100644 --- a/security/integrity/evm/evm.h +++ b/security/integrity/evm/evm.h @@ -24,9 +24,11 @@ #define EVM_INIT_HMAC 0x0001 #define EVM_INIT_X509 0x0002 #define EVM_ALLOW_METADATA_WRITES 0x0004 +#define EVM_INIT_HMAC_RND_KEY 0x0008 #define EVM_SETUP_COMPLETE 0x80000000 /* userland has signaled key load */ -#define EVM_KEY_MASK (EVM_INIT_HMAC | EVM_INIT_X509) +#define EVM_PERSISTENT_KEY_MASK (EVM_INIT_HMAC | EVM_INIT_X509) +#define EVM_KEY_MASK (EVM_INIT_HMAC | EVM_INIT_X509 | EVM_INIT_HMAC_RND_KEY) #define EVM_INIT_MASK (EVM_INIT_HMAC | EVM_INIT_X509 | EVM_SETUP_COMPLETE | \ EVM_ALLOW_METADATA_WRITES) @@ -53,19 +55,21 @@ struct evm_digest { } __packed; int evm_init_key(void); +void evm_set_random_key(void); int evm_update_evmxattr(struct dentry *dentry, const char *req_xattr_name, const char *req_xattr_value, size_t req_xattr_value_len); int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name, const char *req_xattr_value, - size_t req_xattr_value_len, struct evm_digest *data); + size_t req_xattr_value_len, char type, + struct evm_digest *data); int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name, const char *req_xattr_value, size_t req_xattr_value_len, char type, struct evm_digest *data); int evm_init_hmac(struct inode *inode, const struct xattr *xattr, - char *hmac_val); + struct evm_ima_xattr_data *evm_xattr); int evm_init_secfs(void); #endif diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 82a38e801ee4..51a02200b057 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -19,6 +19,7 @@ #include <linux/crypto.h> #include <linux/xattr.h> #include <linux/evm.h> +#include <linux/random.h> #include <keys/encrypted-type.h> #include <crypto/hash.h> #include <crypto/hash_info.h> @@ -30,6 +31,7 @@ static unsigned char evmkey[MAX_KEY_SIZE]; static const int evmkey_len = MAX_KEY_SIZE; struct crypto_shash *hmac_tfm; +struct crypto_shash *hmac_rnd_tfm; static struct crypto_shash *evm_tfm[HASH_ALGO__LAST]; static DEFINE_MUTEX(mutex); @@ -62,8 +64,10 @@ int evm_set_key(void *key, size_t keylen) rc = -EINVAL; if (keylen > MAX_KEY_SIZE) goto inval; + memset(evmkey, 0, sizeof(evmkey)); memcpy(evmkey, key, keylen); evm_initialized |= EVM_INIT_HMAC; + evm_initialized &= ~EVM_INIT_HMAC_RND_KEY; pr_info("key initialized\n"); return 0; inval: @@ -74,6 +78,12 @@ int evm_set_key(void *key, size_t keylen) } EXPORT_SYMBOL_GPL(evm_set_key); +void evm_set_random_key(void) +{ + get_random_bytes(evmkey, sizeof(evmkey)); + evm_initialized |= EVM_INIT_HMAC_RND_KEY; +} + static struct shash_desc *init_desc(char type, uint8_t hash_algo) { long rc; @@ -88,6 +98,9 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo) } tfm = &hmac_tfm; algo = evm_hmac; + } else if (type == EVM_XATTR_HMAC_RND_KEY) { + tfm = &hmac_rnd_tfm; + algo = evm_hmac; } else { if (hash_algo >= HASH_ALGO__LAST) return ERR_PTR(-EINVAL); @@ -108,7 +121,7 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo) mutex_unlock(&mutex); return ERR_PTR(rc); } - if (type == EVM_XATTR_HMAC) { + if (type == EVM_XATTR_HMAC || EVM_XATTR_HMAC_RND_KEY) { rc = crypto_shash_setkey(*tfm, evmkey, evmkey_len); if (rc) { crypto_free_shash(*tfm); @@ -255,10 +268,10 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name, const char *req_xattr_value, size_t req_xattr_value_len, - struct evm_digest *data) + char type, struct evm_digest *data) { return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value, - req_xattr_value_len, EVM_XATTR_HMAC, data); + req_xattr_value_len, type, data); } int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name, @@ -296,6 +309,29 @@ static int evm_is_immutable(struct dentry *dentry, struct inode *inode) return rc; } +static enum evm_ima_xattr_type evm_get_default_type(struct dentry *dentry) +{ + enum evm_ima_xattr_type evm_default_type = EVM_XATTR_HMAC; + struct evm_ima_xattr_data xattr_data; + int rc; + + if (evm_initialized & EVM_INIT_HMAC_RND_KEY) + evm_default_type = EVM_XATTR_HMAC_RND_KEY; + else + goto out; + + rc = vfs_getxattr(dentry, XATTR_NAME_EVM, (char *)&xattr_data, + sizeof(xattr_data)); + + if (rc == sizeof(xattr_data)) + evm_default_type = xattr_data.type; +out: + if (evm_default_type != EVM_XATTR_HMAC_RND_KEY && + !(evm_initialized & EVM_INIT_HMAC)) + return IMA_XATTR_LAST; + + return evm_default_type; +} /* * Calculate the hmac and update security.evm xattr @@ -306,6 +342,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name, const char *xattr_value, size_t xattr_value_len) { struct inode *inode = d_backing_inode(dentry); + enum evm_ima_xattr_type evm_default_type; struct evm_digest data; int rc = 0; @@ -319,11 +356,15 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name, if (rc) return -EPERM; + evm_default_type = evm_get_default_type(dentry); + if (evm_default_type == IMA_XATTR_LAST) + return -ENOKEY; + data.hdr.algo = HASH_ALGO_SHA1; rc = evm_calc_hmac(dentry, xattr_name, xattr_value, - xattr_value_len, &data); + xattr_value_len, evm_default_type, &data); if (rc == 0) { - data.hdr.xattr.sha1.type = EVM_XATTR_HMAC; + data.hdr.xattr.sha1.type = evm_default_type; rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM, &data.hdr.xattr.data[1], SHA1_DIGEST_SIZE + 1, 0); @@ -334,18 +375,18 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name, } int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr, - char *hmac_val) + struct evm_ima_xattr_data *evm_xattr) { struct shash_desc *desc; - desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1); + desc = init_desc(evm_xattr->type, HASH_ALGO_SHA1); if (IS_ERR(desc)) { pr_info("init_desc failed\n"); return PTR_ERR(desc); } crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len); - hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val); + hmac_add_misc(desc, inode, evm_xattr->type, evm_xattr->digest); kfree(desc); return 0; } diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index b6d9f14bc234..faa4a02a3139 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -59,14 +59,16 @@ static struct xattr_list evm_config_default_xattrnames[] = { LIST_HEAD(evm_config_xattrnames); -static int evm_fixmode; -static int __init evm_set_fixmode(char *str) +static int evm_fixmode, evm_random_key; +static int __init evm_setup(char *str) { if (strncmp(str, "fix", 3) == 0) evm_fixmode = 1; + if (strncmp(str, "random", 6) == 0) + evm_random_key = 1; return 0; } -__setup("evm=", evm_set_fixmode); +__setup("evm=", evm_setup); static void __init evm_init_config(void) { @@ -92,6 +94,11 @@ static bool evm_key_loaded(void) return (bool)(evm_initialized & EVM_KEY_MASK); } +static bool evm_persistent_key_loaded(void) +{ + return (bool)(evm_initialized & EVM_PERSISTENT_KEY_MASK); +} + static int evm_find_protected_xattrs(struct dentry *dentry) { struct inode *inode = d_backing_inode(dentry); @@ -152,7 +159,9 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, GFP_NOFS); if (rc <= 0) { evm_status = INTEGRITY_FAIL; - if (rc == -ENODATA) { + if (!evm_persistent_key_loaded()) { + evm_status = INTEGRITY_UNKNOWN; + } else if (rc == -ENODATA) { rc = evm_find_protected_xattrs(dentry); if (rc > 0) evm_status = INTEGRITY_NOLABEL; @@ -164,11 +173,18 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, goto out; } + if (xattr_data->type != EVM_XATTR_HMAC_RND_KEY && + !evm_persistent_key_loaded()) { + evm_status = INTEGRITY_UNKNOWN; + goto out; + } + xattr_len = rc; /* check value type */ switch (xattr_data->type) { case EVM_XATTR_HMAC: + case EVM_XATTR_HMAC_RND_KEY: if (xattr_len != sizeof(struct evm_ima_xattr_data)) { evm_status = INTEGRITY_FAIL; goto out; @@ -176,7 +192,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, digest.hdr.algo = HASH_ALGO_SHA1; rc = evm_calc_hmac(dentry, xattr_name, xattr_value, - xattr_value_len, &digest); + xattr_value_len, xattr_data->type, &digest); if (rc) break; rc = crypto_memneq(xattr_data->digest, digest.digest, @@ -523,18 +539,26 @@ int evm_inode_init_security(struct inode *inode, const struct xattr *lsm_xattr, struct xattr *evm_xattr) { + enum evm_ima_xattr_type evm_default_type = EVM_XATTR_HMAC; struct evm_ima_xattr_data *xattr_data; int rc; if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name)) return 0; + if (!evm_persistent_key_loaded()) { + if (inode->i_sb->s_magic != TMPFS_MAGIC) + return 0; + + evm_default_type = EVM_XATTR_HMAC_RND_KEY; + } + xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS); if (!xattr_data) return -ENOMEM; - xattr_data->type = EVM_XATTR_HMAC; - rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest); + xattr_data->type = evm_default_type; + rc = evm_init_hmac(inode, lsm_xattr, xattr_data); if (rc < 0) goto out; @@ -584,6 +608,9 @@ static int __init init_evm(void) } } + if (!error && evm_random_key) + evm_set_random_key(); + return error; } diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 7de59f44cba3..a037d10db46f 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -74,6 +74,7 @@ enum evm_ima_xattr_type { EVM_IMA_XATTR_DIGSIG, IMA_XATTR_DIGEST_NG, EVM_XATTR_PORTABLE_DIGSIG, + EVM_XATTR_HMAC_RND_KEY, IMA_XATTR_LAST }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status 2019-06-06 11:26 [PATCH v3 0/2] ima/evm fixes for v5.2 Roberto Sassu 2019-06-06 11:26 ` [PATCH v3 1/2] evm: add option to set a random HMAC key at early boot Roberto Sassu @ 2019-06-06 11:26 ` Roberto Sassu 2019-06-07 14:24 ` Mimi Zohar 2019-06-06 11:43 ` [PATCH v3 0/2] ima/evm fixes for v5.2 Roberto Sassu 2019-06-12 11:28 ` Janne Karhunen 3 siblings, 1 reply; 21+ messages in thread From: Roberto Sassu @ 2019-06-06 11:26 UTC (permalink / raw) To: zohar, dmitry.kasatkin, mjg59 Cc: linux-integrity, linux-security-module, linux-doc, stable, linux-kernel, silviu.vlasceanu, Roberto Sassu IMA and EVM have been designed as two independent subsystems: the first for checking the integrity of file data; the second for checking file metadata. Making them independent allows users to adopt them incrementally. The point of intersection is in IMA-Appraisal, which calls evm_verifyxattr() to ensure that security.ima wasn't modified during an offline attack. The design choice, to ensure incremental adoption, was to continue appraisal verification if evm_verifyxattr() returns INTEGRITY_UNKNOWN. This value is returned when EVM is not enabled in the kernel configuration, or if the HMAC key has not been loaded yet. Although this choice appears legitimate, it might not be suitable for hardened systems, where the administrator expects that access is denied if there is any error. An attacker could intentionally delete the EVM keys from the system and set the file digest in security.ima to the actual file digest so that the final appraisal status is INTEGRITY_PASS. This patch allows such hardened systems to strictly enforce an access control policy based on the validity of signatures/HMACs, by introducing two new values for the ima_appraise= kernel option: enforce-evm and log-evm. Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension") Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> Cc: stable@vger.kernel.org --- Documentation/admin-guide/kernel-parameters.txt | 3 ++- security/integrity/ima/ima_appraise.c | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index fe5cde58c11b..0585194ca736 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1587,7 +1587,8 @@ Set number of hash buckets for inode cache. ima_appraise= [IMA] appraise integrity measurements - Format: { "off" | "enforce" | "fix" | "log" } + Format: { "off" | "enforce" | "fix" | "log" | + "enforce-evm" | "log-evm" } default: "enforce" ima_appraise_tcb [IMA] Deprecated. Use ima_policy= instead. diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 5fb7127bbe68..afef06e10fb9 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -18,6 +18,7 @@ #include "ima.h" +static bool ima_appraise_req_evm __ro_after_init; static int __init default_appraise_setup(char *str) { #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM @@ -28,6 +29,9 @@ static int __init default_appraise_setup(char *str) else if (strncmp(str, "fix", 3) == 0) ima_appraise = IMA_APPRAISE_FIX; #endif + if (strcmp(str, "enforce-evm") == 0 || + strcmp(str, "log-evm") == 0) + ima_appraise_req_evm = true; return 1; } @@ -245,7 +249,11 @@ int ima_appraise_measurement(enum ima_hooks func, switch (status) { case INTEGRITY_PASS: case INTEGRITY_PASS_IMMUTABLE: + break; case INTEGRITY_UNKNOWN: + if (ima_appraise_req_evm && + xattr_value->type != EVM_IMA_XATTR_DIGSIG) + goto out; break; case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */ case INTEGRITY_NOLABEL: /* No security.evm xattr. */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status 2019-06-06 11:26 ` [PATCH v3 2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status Roberto Sassu @ 2019-06-07 14:24 ` Mimi Zohar 2019-06-07 14:40 ` Roberto Sassu 0 siblings, 1 reply; 21+ messages in thread From: Mimi Zohar @ 2019-06-07 14:24 UTC (permalink / raw) To: Roberto Sassu, dmitry.kasatkin, mjg59 Cc: linux-integrity, linux-security-module, linux-doc, stable, linux-kernel, silviu.vlasceanu Hi Roberto, Thank you for updating the patch description. On Thu, 2019-06-06 at 13:26 +0200, Roberto Sassu wrote: > IMA and EVM have been designed as two independent subsystems: the first for > checking the integrity of file data; the second for checking file metadata. > Making them independent allows users to adopt them incrementally. > > The point of intersection is in IMA-Appraisal, which calls > evm_verifyxattr() to ensure that security.ima wasn't modified during an > offline attack. The design choice, to ensure incremental adoption, was to > continue appraisal verification if evm_verifyxattr() returns > INTEGRITY_UNKNOWN. This value is returned when EVM is not enabled in the > kernel configuration, or if the HMAC key has not been loaded yet. > > Although this choice appears legitimate, it might not be suitable for > hardened systems, where the administrator expects that access is denied if > there is any error. An attacker could intentionally delete the EVM keys > from the system and set the file digest in security.ima to the actual file > digest so that the final appraisal status is INTEGRITY_PASS. Assuming that the EVM HMAC key is stored in the initramfs, not on some other file system, and the initramfs is signed, INTEGRITY_UNKNOWN would be limited to the rootfs filesystem. > > This patch allows such hardened systems to strictly enforce an access > control policy based on the validity of signatures/HMACs, by introducing > two new values for the ima_appraise= kernel option: enforce-evm and > log-evm. > This patch defines a global policy requiring EVM on all filesystems. I've previously suggested extending the IMA policy to support enforcing or maybe exempting EVM on a per IMA policy rule basis. As seen by the need for an additional patch, included in this patch set, which defines a temporary random number HMAC key to address INTEGRITY_UNKNOWN on the rootfs filesystem, exempting certain filesystems on a per policy rule basis might be simpler and achieve similar results. I'd like to hear other people's thoughts on defining a temporary, random number HMAC key. thanks, Mimi ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status 2019-06-07 14:24 ` Mimi Zohar @ 2019-06-07 14:40 ` Roberto Sassu 2019-06-07 15:08 ` Mimi Zohar 0 siblings, 1 reply; 21+ messages in thread From: Roberto Sassu @ 2019-06-07 14:40 UTC (permalink / raw) To: Mimi Zohar, dmitry.kasatkin, mjg59 Cc: linux-integrity, linux-security-module, linux-doc, stable, linux-kernel, silviu.vlasceanu On 6/7/2019 4:24 PM, Mimi Zohar wrote: > Hi Roberto, > > Thank you for updating the patch description. Hi Mimi no problem. > On Thu, 2019-06-06 at 13:26 +0200, Roberto Sassu wrote: >> IMA and EVM have been designed as two independent subsystems: the first for >> checking the integrity of file data; the second for checking file metadata. >> Making them independent allows users to adopt them incrementally. >> >> The point of intersection is in IMA-Appraisal, which calls >> evm_verifyxattr() to ensure that security.ima wasn't modified during an >> offline attack. The design choice, to ensure incremental adoption, was to >> continue appraisal verification if evm_verifyxattr() returns >> INTEGRITY_UNKNOWN. This value is returned when EVM is not enabled in the >> kernel configuration, or if the HMAC key has not been loaded yet. >> >> Although this choice appears legitimate, it might not be suitable for >> hardened systems, where the administrator expects that access is denied if >> there is any error. An attacker could intentionally delete the EVM keys >> from the system and set the file digest in security.ima to the actual file >> digest so that the final appraisal status is INTEGRITY_PASS. > > Assuming that the EVM HMAC key is stored in the initramfs, not on some > other file system, and the initramfs is signed, INTEGRITY_UNKNOWN > would be limited to the rootfs filesystem. There is another issue. The HMAC key, like the public keys, should be loaded when appraisal is disabled. This means that we have to create a trusted key at early boot and defer the unsealing. Roberto -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Jian LI, Yanli SHI ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status 2019-06-07 14:40 ` Roberto Sassu @ 2019-06-07 15:08 ` Mimi Zohar 2019-06-07 15:14 ` Roberto Sassu 0 siblings, 1 reply; 21+ messages in thread From: Mimi Zohar @ 2019-06-07 15:08 UTC (permalink / raw) To: Roberto Sassu, dmitry.kasatkin, mjg59 Cc: linux-integrity, linux-security-module, linux-doc, stable, linux-kernel, silviu.vlasceanu On Fri, 2019-06-07 at 16:40 +0200, Roberto Sassu wrote: > > On Thu, 2019-06-06 at 13:26 +0200, Roberto Sassu wrote: > >> Although this choice appears legitimate, it might not be suitable for > >> hardened systems, where the administrator expects that access is denied if > >> there is any error. An attacker could intentionally delete the EVM keys > >> from the system and set the file digest in security.ima to the actual file > >> digest so that the final appraisal status is INTEGRITY_PASS. > > > > Assuming that the EVM HMAC key is stored in the initramfs, not on some > > other file system, and the initramfs is signed, INTEGRITY_UNKNOWN > > would be limited to the rootfs filesystem. > > There is another issue. The HMAC key, like the public keys, should be > loaded when appraisal is disabled. This means that we have to create a > trusted key at early boot and defer the unsealing. There is no need for IMA to appraise the public key file signature, since the certificate is signed by a key on the builtin/secondary trusted keyring. With CONFIG_IMA_LOAD_X509 enabled, the public key can be loaded onto the IMA keyring with IMA-appraisal enabled, but without verifying the file signature. Mimi ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status 2019-06-07 15:08 ` Mimi Zohar @ 2019-06-07 15:14 ` Roberto Sassu 2019-06-07 15:25 ` Mimi Zohar 0 siblings, 1 reply; 21+ messages in thread From: Roberto Sassu @ 2019-06-07 15:14 UTC (permalink / raw) To: Mimi Zohar, dmitry.kasatkin, mjg59 Cc: linux-integrity, linux-security-module, linux-doc, stable, linux-kernel, silviu.vlasceanu On 6/7/2019 5:08 PM, Mimi Zohar wrote: > On Fri, 2019-06-07 at 16:40 +0200, Roberto Sassu wrote: >>> On Thu, 2019-06-06 at 13:26 +0200, Roberto Sassu wrote: > >>>> Although this choice appears legitimate, it might not be suitable for >>>> hardened systems, where the administrator expects that access is denied if >>>> there is any error. An attacker could intentionally delete the EVM keys >>>> from the system and set the file digest in security.ima to the actual file >>>> digest so that the final appraisal status is INTEGRITY_PASS. >>> >>> Assuming that the EVM HMAC key is stored in the initramfs, not on some >>> other file system, and the initramfs is signed, INTEGRITY_UNKNOWN >>> would be limited to the rootfs filesystem. >> >> There is another issue. The HMAC key, like the public keys, should be >> loaded when appraisal is disabled. This means that we have to create a >> trusted key at early boot and defer the unsealing. > > There is no need for IMA to appraise the public key file signature, > since the certificate is signed by a key on the builtin/secondary > trusted keyring. With CONFIG_IMA_LOAD_X509 enabled, the public key > can be loaded onto the IMA keyring with IMA-appraisal enabled, but > without verifying the file signature. Yes, but access to the files containing the master key and the EVM key is denied if appraisal is enabled. Roberto -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Jian LI, Yanli SHI ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status 2019-06-07 15:14 ` Roberto Sassu @ 2019-06-07 15:25 ` Mimi Zohar 0 siblings, 0 replies; 21+ messages in thread From: Mimi Zohar @ 2019-06-07 15:25 UTC (permalink / raw) To: Roberto Sassu, dmitry.kasatkin, mjg59 Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel, silviu.vlasceanu On Fri, 2019-06-07 at 17:14 +0200, Roberto Sassu wrote: > On 6/7/2019 5:08 PM, Mimi Zohar wrote: > > On Fri, 2019-06-07 at 16:40 +0200, Roberto Sassu wrote: > >>> On Thu, 2019-06-06 at 13:26 +0200, Roberto Sassu wrote: > > > >>>> Although this choice appears legitimate, it might not be suitable for > >>>> hardened systems, where the administrator expects that access is denied if > >>>> there is any error. An attacker could intentionally delete the EVM keys > >>>> from the system and set the file digest in security.ima to the actual file > >>>> digest so that the final appraisal status is INTEGRITY_PASS. > >>> > >>> Assuming that the EVM HMAC key is stored in the initramfs, not on some > >>> other file system, and the initramfs is signed, INTEGRITY_UNKNOWN > >>> would be limited to the rootfs filesystem. > >> > >> There is another issue. The HMAC key, like the public keys, should be > >> loaded when appraisal is disabled. This means that we have to create a > >> trusted key at early boot and defer the unsealing. > > > > There is no need for IMA to appraise the public key file signature, > > since the certificate is signed by a key on the builtin/secondary > > trusted keyring. With CONFIG_IMA_LOAD_X509 enabled, the public key > > can be loaded onto the IMA keyring with IMA-appraisal enabled, but > > without verifying the file signature. > > Yes, but access to the files containing the master key and the EVM key > is denied if appraisal is enabled. This is a key loading ordering issue. Assuming you load the IMA key first, you should be able to verify the master and EVM keys. Mimi ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-06 11:26 [PATCH v3 0/2] ima/evm fixes for v5.2 Roberto Sassu 2019-06-06 11:26 ` [PATCH v3 1/2] evm: add option to set a random HMAC key at early boot Roberto Sassu 2019-06-06 11:26 ` [PATCH v3 2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status Roberto Sassu @ 2019-06-06 11:43 ` Roberto Sassu 2019-06-06 14:49 ` Mimi Zohar 2019-06-12 11:28 ` Janne Karhunen 3 siblings, 1 reply; 21+ messages in thread From: Roberto Sassu @ 2019-06-06 11:43 UTC (permalink / raw) To: zohar, dmitry.kasatkin, mjg59 Cc: linux-integrity, linux-security-module, linux-doc, stable, linux-kernel, silviu.vlasceanu On 6/6/2019 1:26 PM, Roberto Sassu wrote: > Previous versions included the patch 'ima: don't ignore INTEGRITY_UNKNOWN > EVM status'. However, I realized that this patch cannot be accepted alone > because IMA-Appraisal would deny access to new files created during the > boot. With the current behavior, those files are accessible because they > have a valid security.ima (not protected by EVM) created after the first > write. > > A solution for this problem is to initialize EVM very early with a random > key. Access to created files will be granted, even with the strict > appraisal, because after the first write those files will have both > security.ima and security.evm (HMAC calculated with the random key). > > Strict appraisal will work only if it is done with signatures until the > persistent HMAC key is loaded. Changelog v2: - remove patch 1/3 (evm: check hash algorithm passed to init_desc()); already accepted - remove patch 3/3 (ima: show rules with IMA_INMASK correctly); already accepted - add new patch (evm: add option to set a random HMAC key at early boot) - patch 2/3: modify patch description v1: - remove patch 2/4 (evm: reset status in evm_inode_post_setattr()); file attributes cannot be set if the signature is portable and immutable - patch 3/4: add __ro_after_init to ima_appraise_req_evm variable declaration - patch 3/4: remove ima_appraise_req_evm kernel option and introduce 'enforce-evm' and 'log-evm' as possible values for ima_appraise= - remove patch 4/4 (ima: only audit failed appraisal verifications) - add new patch (ima: show rules with IMA_INMASK correctly) > Roberto Sassu (2): > evm: add option to set a random HMAC key at early boot > ima: add enforce-evm and log-evm modes to strictly check EVM status > > .../admin-guide/kernel-parameters.txt | 11 ++-- > security/integrity/evm/evm.h | 10 +++- > security/integrity/evm/evm_crypto.c | 57 ++++++++++++++++--- > security/integrity/evm/evm_main.c | 41 ++++++++++--- > security/integrity/ima/ima_appraise.c | 8 +++ > security/integrity/integrity.h | 1 + > 6 files changed, 106 insertions(+), 22 deletions(-) > -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Jian LI, Yanli SHI ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-06 11:43 ` [PATCH v3 0/2] ima/evm fixes for v5.2 Roberto Sassu @ 2019-06-06 14:49 ` Mimi Zohar 2019-06-06 15:22 ` Roberto Sassu 0 siblings, 1 reply; 21+ messages in thread From: Mimi Zohar @ 2019-06-06 14:49 UTC (permalink / raw) To: Roberto Sassu, dmitry.kasatkin, mjg59 Cc: linux-integrity, linux-security-module, linux-doc, stable, linux-kernel, silviu.vlasceanu On Thu, 2019-06-06 at 13:43 +0200, Roberto Sassu wrote: > On 6/6/2019 1:26 PM, Roberto Sassu wrote: > > Previous versions included the patch 'ima: don't ignore INTEGRITY_UNKNOWN > > EVM status'. However, I realized that this patch cannot be accepted alone > > because IMA-Appraisal would deny access to new files created during the > > boot. With the current behavior, those files are accessible because they > > have a valid security.ima (not protected by EVM) created after the first > > write. > > > > A solution for this problem is to initialize EVM very early with a random > > key. Access to created files will be granted, even with the strict > > appraisal, because after the first write those files will have both > > security.ima and security.evm (HMAC calculated with the random key). > > > > Strict appraisal will work only if it is done with signatures until the > > persistent HMAC key is loaded. > > Changelog > > v2: > - remove patch 1/3 (evm: check hash algorithm passed to init_desc()); > already accepted > - remove patch 3/3 (ima: show rules with IMA_INMASK correctly); > already accepted > - add new patch (evm: add option to set a random HMAC key at early boot) > - patch 2/3: modify patch description Roberto, as I tried explaining previously, this feature is not a simple bug fix. These patches, if upstreamed, will be upstreamed the normal way, during an open window. Whether they are classified as a bug fix has yet to be decided. Please stop Cc'ing stable. If I don't Cc stable before sending the pull request, then Greg and Sasha have been really good about deciding which patches should be backported. (Please refer to the comment on "Cc'ing stable" in section "5) Select the recipients for your patch" in Documentation/process/submitting-patches.rst.) I'll review these patches, but in the future please use an appropriate patch set cover letter title in the subject line. thanks, Mimi > > v1: > - remove patch 2/4 (evm: reset status in evm_inode_post_setattr()); file > attributes cannot be set if the signature is portable and immutable > - patch 3/4: add __ro_after_init to ima_appraise_req_evm variable > declaration > - patch 3/4: remove ima_appraise_req_evm kernel option and introduce > 'enforce-evm' and 'log-evm' as possible values for ima_appraise= > - remove patch 4/4 (ima: only audit failed appraisal verifications) > - add new patch (ima: show rules with IMA_INMASK correctly) > > > > Roberto Sassu (2): > > evm: add option to set a random HMAC key at early boot > > ima: add enforce-evm and log-evm modes to strictly check EVM status > > > > .../admin-guide/kernel-parameters.txt | 11 ++-- > > security/integrity/evm/evm.h | 10 +++- > > security/integrity/evm/evm_crypto.c | 57 ++++++++++++++++--- > > security/integrity/evm/evm_main.c | 41 ++++++++++--- > > security/integrity/ima/ima_appraise.c | 8 +++ > > security/integrity/integrity.h | 1 + > > 6 files changed, 106 insertions(+), 22 deletions(-) > > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-06 14:49 ` Mimi Zohar @ 2019-06-06 15:22 ` Roberto Sassu 0 siblings, 0 replies; 21+ messages in thread From: Roberto Sassu @ 2019-06-06 15:22 UTC (permalink / raw) To: Mimi Zohar, dmitry.kasatkin, mjg59 Cc: linux-integrity, linux-security-module, linux-doc, stable, linux-kernel, silviu.vlasceanu On 6/6/2019 4:49 PM, Mimi Zohar wrote: > On Thu, 2019-06-06 at 13:43 +0200, Roberto Sassu wrote: >> On 6/6/2019 1:26 PM, Roberto Sassu wrote: >>> Previous versions included the patch 'ima: don't ignore INTEGRITY_UNKNOWN >>> EVM status'. However, I realized that this patch cannot be accepted alone >>> because IMA-Appraisal would deny access to new files created during the >>> boot. With the current behavior, those files are accessible because they >>> have a valid security.ima (not protected by EVM) created after the first >>> write. >>> >>> A solution for this problem is to initialize EVM very early with a random >>> key. Access to created files will be granted, even with the strict >>> appraisal, because after the first write those files will have both >>> security.ima and security.evm (HMAC calculated with the random key). >>> >>> Strict appraisal will work only if it is done with signatures until the >>> persistent HMAC key is loaded. >> >> Changelog >> >> v2: >> - remove patch 1/3 (evm: check hash algorithm passed to init_desc()); >> already accepted >> - remove patch 3/3 (ima: show rules with IMA_INMASK correctly); >> already accepted >> - add new patch (evm: add option to set a random HMAC key at early boot) >> - patch 2/3: modify patch description > > Roberto, as I tried explaining previously, this feature is not a > simple bug fix. These patches, if upstreamed, will be upstreamed the > normal way, during an open window. Whether they are classified as a > bug fix has yet to be decided. Sorry, I understood that I can claim that there is a bug. I provided a motivation in patch 2/2. > Please stop Cc'ing stable. If I don't Cc stable before sending the pull request, then Greg and Sasha have been really good about deciding which patches should be backported. (Please refer to the comment on "Cc'ing stable" in section "5) Select the recipients for your patch" in Documentation/process/submitting-patches.rst.) > > I'll review these patches, but in the future please use an appropriate patch set cover letter title in the subject line. Ok. Thanks Roberto -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Jian LI, Yanli SHI ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-06 11:26 [PATCH v3 0/2] ima/evm fixes for v5.2 Roberto Sassu ` (2 preceding siblings ...) 2019-06-06 11:43 ` [PATCH v3 0/2] ima/evm fixes for v5.2 Roberto Sassu @ 2019-06-12 11:28 ` Janne Karhunen 2019-06-12 13:11 ` Roberto Sassu 3 siblings, 1 reply; 21+ messages in thread From: Janne Karhunen @ 2019-06-12 11:28 UTC (permalink / raw) To: Roberto Sassu Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity, linux-security-module, silviu.vlasceanu On Thu, Jun 6, 2019 at 3:27 PM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > Previous versions included the patch 'ima: don't ignore INTEGRITY_UNKNOWN > EVM status'. However, I realized that this patch cannot be accepted alone > because IMA-Appraisal would deny access to new files created during the > boot. The early initialization logic seems to have been changing, the original one as I have understood it: - before initialization - allow reading anything without security.ima - deny reading anything with security.ima - allow all writes - after initialization - deny reading|writing anything without security.ima - deny reading|writing anything invalid - allow everything else The logic is pretty handy as it even creates additional layer of security around the early initialization files as they become unreadable after use. Now, if we initialize the system with a random key like in your patch, this logic is to change quite drastically? It sounds to me the userland may actually break, all the userland initialization files in the existing ima configurations that do not use digsigs would become unreadable given that the random key is put in? Remember, those files can be protected via other means (most commonly signed ramdisk). -- Janne ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-12 11:28 ` Janne Karhunen @ 2019-06-12 13:11 ` Roberto Sassu 2019-06-12 13:38 ` Janne Karhunen 0 siblings, 1 reply; 21+ messages in thread From: Roberto Sassu @ 2019-06-12 13:11 UTC (permalink / raw) To: Janne Karhunen Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity, linux-security-module, silviu.vlasceanu On 6/12/2019 1:28 PM, Janne Karhunen wrote: > On Thu, Jun 6, 2019 at 3:27 PM Roberto Sassu <roberto.sassu@huawei.com> wrote: >> >> Previous versions included the patch 'ima: don't ignore INTEGRITY_UNKNOWN >> EVM status'. However, I realized that this patch cannot be accepted alone >> because IMA-Appraisal would deny access to new files created during the >> boot. > > The early initialization logic seems to have been changing, the > original one as I have understood it: > - before initialization > - allow reading anything without security.ima > - deny reading anything with security.ima These two should be probably inverted: deny..., allow... > - allow all writes Allow writing anything with security.ima Allow writing new files > - after initialization > - deny reading|writing anything without security.ima > - deny reading|writing anything invalid > - allow everything else > > The logic is pretty handy as it even creates additional layer of > security around the early initialization files as they become > unreadable after use. What if they should be legitimately used after the HMAC key is unsealed and before switching to the persistent root file system? > Now, if we initialize the system with a random key like in your patch, > this logic is to change quite drastically? It sounds to me the > userland may actually break, all the userland initialization files in > the existing ima configurations that do not use digsigs would become > unreadable given that the random key is put in? Remember, those files > can be protected via other means (most commonly signed ramdisk). No, the first patch is about adding the ability to verify files created during each boot. For any other file, EVM returns INTEGRITY_UNKNOWN as before. The second patch changes the behavior, as INTEGRITY_UNKNOWN is considered as an error for the enforce-evm appraisal mode. The second patch aims at making the system more secure, as no file would be accessible unless it is verified. It is true that configurations without digsigs won't work anymore but the alternative is accepting any file until the HMAC key is unsealed. Signing the ramdisk is for sure a possibility, but IMA would be sufficient to provide integrity protection as it checks any file in the ram disk. Unfortunately I found an issue in patch 1/2. These changes should be applied: -- diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index faa4a02a3139..f4af595678fe 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -310,10 +310,14 @@ EXPORT_SYMBOL_GPL(evm_verifyxattr); static enum integrity_status evm_verify_current_integrity(struct dentry *dentry) { struct inode *inode = d_backing_inode(dentry); + int rc; if (!evm_key_loaded() || !S_ISREG(inode->i_mode) || evm_fixmode) return 0; - return evm_verify_hmac(dentry, NULL, NULL, 0, NULL); + rc = evm_verify_hmac(dentry, NULL, NULL, 0, NULL); + if (rc == INTEGRITY_UNKNOWN && !evm_persistent_key_loaded()) + return 0; + return rc; } /* -- Roberto -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Jian LI, Yanli SHI ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-12 13:11 ` Roberto Sassu @ 2019-06-12 13:38 ` Janne Karhunen 2019-06-12 16:33 ` Roberto Sassu 0 siblings, 1 reply; 21+ messages in thread From: Janne Karhunen @ 2019-06-12 13:38 UTC (permalink / raw) To: Roberto Sassu Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity, linux-security-module, silviu.vlasceanu On Wed, Jun 12, 2019 at 4:11 PM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > - after initialization > > - deny reading|writing anything without security.ima > > - deny reading|writing anything invalid > > - allow everything else > > > > The logic is pretty handy as it even creates additional layer of > > security around the early initialization files as they become > > unreadable after use. > > What if they should be legitimately used after the HMAC key is unsealed > and before switching to the persistent root file system? Any examples? Log files and such are mostly 'one way' and should probably be whitelisted in the policy? > > Now, if we initialize the system with a random key like in your patch, > > this logic is to change quite drastically? It sounds to me the > > userland may actually break, all the userland initialization files in > > the existing ima configurations that do not use digsigs would become > > unreadable given that the random key is put in? Remember, those files > > can be protected via other means (most commonly signed ramdisk). > > No, the first patch is about adding the ability to verify files created > during each boot. For any other file, EVM returns INTEGRITY_UNKNOWN as > before. The second patch changes the behavior, as INTEGRITY_UNKNOWN is > considered as an error for the enforce-evm appraisal mode. The second > patch aims at making the system more secure, as no file would be > accessible unless it is verified. > > It is true that configurations without digsigs won't work anymore but > the alternative is accepting any file until the HMAC key is unsealed. That's a pretty big change for the userland IMHO. Quite a few configurations out there will break, including mine I believe, so I hope there is a solid reason asking people to change their stuff. I'm fine holding off all writing until it is safe to do so for now.. -- Janne ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-12 13:38 ` Janne Karhunen @ 2019-06-12 16:33 ` Roberto Sassu 2019-06-13 6:01 ` Janne Karhunen 0 siblings, 1 reply; 21+ messages in thread From: Roberto Sassu @ 2019-06-12 16:33 UTC (permalink / raw) To: Janne Karhunen Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity, linux-security-module, silviu.vlasceanu On 6/12/2019 3:38 PM, Janne Karhunen wrote: > On Wed, Jun 12, 2019 at 4:11 PM Roberto Sassu <roberto.sassu@huawei.com> wrote: > >>> - after initialization >>> - deny reading|writing anything without security.ima >>> - deny reading|writing anything invalid >>> - allow everything else >>> >>> The logic is pretty handy as it even creates additional layer of >>> security around the early initialization files as they become >>> unreadable after use. >> >> What if they should be legitimately used after the HMAC key is unsealed >> and before switching to the persistent root file system? > > Any examples? Log files and such are mostly 'one way' and should > probably be whitelisted in the policy? I checked better when the random key would be used to verify files created during the boot. If we consider rootfs only, basically it would be used for dracut-state.sh. Before I was using a rule to measure digest lists in tmpfs. I had many errors due to the fact that appraisal denied access to files in /run. The default policy does not appraise files in tmpfs, and also for digest lists it is not necessary (now I use: measure/appraise fsname=rootfs). >>> Now, if we initialize the system with a random key like in your patch, >>> this logic is to change quite drastically? It sounds to me the >>> userland may actually break, all the userland initialization files in >>> the existing ima configurations that do not use digsigs would become >>> unreadable given that the random key is put in? Remember, those files >>> can be protected via other means (most commonly signed ramdisk). >> >> No, the first patch is about adding the ability to verify files created >> during each boot. For any other file, EVM returns INTEGRITY_UNKNOWN as >> before. The second patch changes the behavior, as INTEGRITY_UNKNOWN is >> considered as an error for the enforce-evm appraisal mode. The second >> patch aims at making the system more secure, as no file would be >> accessible unless it is verified. >> >> It is true that configurations without digsigs won't work anymore but >> the alternative is accepting any file until the HMAC key is unsealed. > > That's a pretty big change for the userland IMHO. Quite a few > configurations out there will break, including mine I believe, so I > hope there is a solid reason asking people to change their stuff. I'm > fine holding off all writing until it is safe to do so for now.. The goal of appraisal is to allow access only to files with a valid signature or HMAC. With the current behavior, that cannot be guaranteed. Unfortunately, dracut-state.sh is created very early. It could be possible to unseal the key before, but this probably means modifying systemd. Roberto -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Jian LI, Yanli SHI ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-12 16:33 ` Roberto Sassu @ 2019-06-13 6:01 ` Janne Karhunen 2019-06-13 6:57 ` Roberto Sassu 0 siblings, 1 reply; 21+ messages in thread From: Janne Karhunen @ 2019-06-13 6:01 UTC (permalink / raw) To: Roberto Sassu Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity, linux-security-module, silviu.vlasceanu On Wed, Jun 12, 2019 at 7:33 PM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > That's a pretty big change for the userland IMHO. Quite a few > > configurations out there will break, including mine I believe, so I > > hope there is a solid reason asking people to change their stuff. I'm > > fine holding off all writing until it is safe to do so for now.. > > The goal of appraisal is to allow access only to files with a valid > signature or HMAC. With the current behavior, that cannot be guaranteed. > > Unfortunately, dracut-state.sh is created very early. It could be > possible to unseal the key before, but this probably means modifying > systemd. Ok, I see the use case. Now, if you pull a urandom key that early on during the boot, the state of the system entropy is at all time low, and you are not really protecting against any sort of offline attack since the file is created during that boot cycle. Is there really use for using such key? Wouldn't it be possible to create a new config option, say IMA_ALLOW_EARLY_WRITERS, that would hold the NEW_FILE flag until the persistent key becomes available? In other words, it would start the measuring at the point when the key becomes online? -- Janne ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-13 6:01 ` Janne Karhunen @ 2019-06-13 6:57 ` Roberto Sassu 2019-06-13 7:39 ` Janne Karhunen 0 siblings, 1 reply; 21+ messages in thread From: Roberto Sassu @ 2019-06-13 6:57 UTC (permalink / raw) To: Janne Karhunen Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity, linux-security-module, silviu.vlasceanu On 6/13/2019 8:01 AM, Janne Karhunen wrote: > On Wed, Jun 12, 2019 at 7:33 PM Roberto Sassu <roberto.sassu@huawei.com> wrote: > >>> That's a pretty big change for the userland IMHO. Quite a few >>> configurations out there will break, including mine I believe, so I >>> hope there is a solid reason asking people to change their stuff. I'm >>> fine holding off all writing until it is safe to do so for now.. >> >> The goal of appraisal is to allow access only to files with a valid >> signature or HMAC. With the current behavior, that cannot be guaranteed. >> >> Unfortunately, dracut-state.sh is created very early. It could be >> possible to unseal the key before, but this probably means modifying >> systemd. > > Ok, I see the use case. Now, if you pull a urandom key that early on > during the boot, the state of the system entropy is at all time low, > and you are not really protecting against any sort of offline attack > since the file is created during that boot cycle. Is there really use > for using such key? Wouldn't it be possible to create a new config > option, say IMA_ALLOW_EARLY_WRITERS, that would hold the NEW_FILE flag > until the persistent key becomes available? In other words, it would > start the measuring at the point when the key becomes online? I also thought about similar solutions. Another is for example to keep the appraisal flags at file close, if security.ima is successfully added to the file. Initializing EVM with a key is not a trivial change, but it seemed better to me as it does not introduce exceptions in the IMA behavior. Roberto -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Jian LI, Yanli SHI ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-13 6:57 ` Roberto Sassu @ 2019-06-13 7:39 ` Janne Karhunen 2019-06-13 7:50 ` Roberto Sassu 0 siblings, 1 reply; 21+ messages in thread From: Janne Karhunen @ 2019-06-13 7:39 UTC (permalink / raw) To: Roberto Sassu Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity, linux-security-module, silviu.vlasceanu On Thu, Jun 13, 2019 at 9:57 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > Ok, I see the use case. Now, if you pull a urandom key that early on > > during the boot, the state of the system entropy is at all time low, > > and you are not really protecting against any sort of offline attack > > since the file is created during that boot cycle. Is there really use > > for using such key? Wouldn't it be possible to create a new config > > option, say IMA_ALLOW_EARLY_WRITERS, that would hold the NEW_FILE flag > > until the persistent key becomes available? In other words, it would > > start the measuring at the point when the key becomes online? > > I also thought about similar solutions. Another is for example to keep > the appraisal flags at file close, if security.ima is successfully > added to the file. > > Initializing EVM with a key is not a trivial change, but it seemed > better to me as it does not introduce exceptions in the IMA behavior. Would the appraise actually need any changes, just keep the IMA_NEW_FILE in ima_check_last_writer()? Of course it's not that easy (it never is) as the iint could go away and things like that, but with some tweaks? -- Janne ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-13 7:39 ` Janne Karhunen @ 2019-06-13 7:50 ` Roberto Sassu 2019-06-13 8:04 ` Janne Karhunen 0 siblings, 1 reply; 21+ messages in thread From: Roberto Sassu @ 2019-06-13 7:50 UTC (permalink / raw) To: Janne Karhunen Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity, linux-security-module, silviu.vlasceanu On 6/13/2019 9:39 AM, Janne Karhunen wrote: > On Thu, Jun 13, 2019 at 9:57 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > >>> Ok, I see the use case. Now, if you pull a urandom key that early on >>> during the boot, the state of the system entropy is at all time low, >>> and you are not really protecting against any sort of offline attack >>> since the file is created during that boot cycle. Is there really use >>> for using such key? Wouldn't it be possible to create a new config >>> option, say IMA_ALLOW_EARLY_WRITERS, that would hold the NEW_FILE flag >>> until the persistent key becomes available? In other words, it would >>> start the measuring at the point when the key becomes online? >> >> I also thought about similar solutions. Another is for example to keep >> the appraisal flags at file close, if security.ima is successfully >> added to the file. >> >> Initializing EVM with a key is not a trivial change, but it seemed >> better to me as it does not introduce exceptions in the IMA behavior. > > Would the appraise actually need any changes, just keep the > IMA_NEW_FILE in ima_check_last_writer()? Of course it's not that easy > (it never is) as the iint could go away and things like that, but with > some tweaks? I think the problem would be that the code that sets the status to INTEGRITY_PASS is not executed, because the file gets security.ima after the first write. Roberto -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Jian LI, Yanli SHI ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-13 7:50 ` Roberto Sassu @ 2019-06-13 8:04 ` Janne Karhunen 2019-06-13 8:51 ` Roberto Sassu 0 siblings, 1 reply; 21+ messages in thread From: Janne Karhunen @ 2019-06-13 8:04 UTC (permalink / raw) To: Roberto Sassu Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity, linux-security-module, silviu.vlasceanu On Thu, Jun 13, 2019 at 10:50 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > Would the appraise actually need any changes, just keep the > > IMA_NEW_FILE in ima_check_last_writer()? Of course it's not that easy > > (it never is) as the iint could go away and things like that, but with > > some tweaks? > > I think the problem would be that the code that sets the status to > INTEGRITY_PASS is not executed, because the file gets security.ima after > the first write. We have a patchset coming shortly that starts tracking the inode changes as we go, so first time we fix it is when the file is created before it has any content (!); diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 5fb7127bbe68..da4f0afe0348 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -236,8 +236,10 @@ int ima_appraise_measurement(enum ima_hooks func, iint->flags |= IMA_NEW_FILE; if ((iint->flags & IMA_NEW_FILE) && (!(iint->flags & IMA_DIGSIG_REQUIRED) || - (inode->i_size == 0))) + (inode->i_size == 0))) { + ima_fix_xattr(dentry, iint); status = INTEGRITY_PASS; + } goto out; } -- Janne ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] ima/evm fixes for v5.2 2019-06-13 8:04 ` Janne Karhunen @ 2019-06-13 8:51 ` Roberto Sassu 0 siblings, 0 replies; 21+ messages in thread From: Roberto Sassu @ 2019-06-13 8:51 UTC (permalink / raw) To: Janne Karhunen Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity, linux-security-module, silviu.vlasceanu On 6/13/2019 10:04 AM, Janne Karhunen wrote: > On Thu, Jun 13, 2019 at 10:50 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > >>> Would the appraise actually need any changes, just keep the >>> IMA_NEW_FILE in ima_check_last_writer()? Of course it's not that easy >>> (it never is) as the iint could go away and things like that, but with >>> some tweaks? >> >> I think the problem would be that the code that sets the status to >> INTEGRITY_PASS is not executed, because the file gets security.ima after >> the first write. > > We have a patchset coming shortly that starts tracking the inode > changes as we go, so first time we fix it is when the file is created > before it has any content (!); > > diff --git a/security/integrity/ima/ima_appraise.c > b/security/integrity/ima/ima_appraise.c > index 5fb7127bbe68..da4f0afe0348 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -236,8 +236,10 @@ int ima_appraise_measurement(enum ima_hooks func, > iint->flags |= IMA_NEW_FILE; > if ((iint->flags & IMA_NEW_FILE) && > (!(iint->flags & IMA_DIGSIG_REQUIRED) || > - (inode->i_size == 0))) > + (inode->i_size == 0))) { > + ima_fix_xattr(dentry, iint); > status = INTEGRITY_PASS; Some time ago I developed this patch: http://kernsec.org/pipermail/linux-security-module-archive/2017-November/004569.html Since the appraisal flags are not cleared, ima_appraise_measurement() is not executed again and the problem with EVM does not arise. Roberto -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Jian LI, Yanli SHI ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-06-13 16:45 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-06 11:26 [PATCH v3 0/2] ima/evm fixes for v5.2 Roberto Sassu 2019-06-06 11:26 ` [PATCH v3 1/2] evm: add option to set a random HMAC key at early boot Roberto Sassu 2019-06-06 11:26 ` [PATCH v3 2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status Roberto Sassu 2019-06-07 14:24 ` Mimi Zohar 2019-06-07 14:40 ` Roberto Sassu 2019-06-07 15:08 ` Mimi Zohar 2019-06-07 15:14 ` Roberto Sassu 2019-06-07 15:25 ` Mimi Zohar 2019-06-06 11:43 ` [PATCH v3 0/2] ima/evm fixes for v5.2 Roberto Sassu 2019-06-06 14:49 ` Mimi Zohar 2019-06-06 15:22 ` Roberto Sassu 2019-06-12 11:28 ` Janne Karhunen 2019-06-12 13:11 ` Roberto Sassu 2019-06-12 13:38 ` Janne Karhunen 2019-06-12 16:33 ` Roberto Sassu 2019-06-13 6:01 ` Janne Karhunen 2019-06-13 6:57 ` Roberto Sassu 2019-06-13 7:39 ` Janne Karhunen 2019-06-13 7:50 ` Roberto Sassu 2019-06-13 8:04 ` Janne Karhunen 2019-06-13 8:51 ` Roberto Sassu
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).