Menu
▾
▴
linux-ima-user
|
From: Mikhail K. <vie...@vi...> - 2016-12-23 22:59:24
|
I am not sure, if portable EVM signature version is still in discussion or not, but, in case of someone interested in this feature too, I propose to discuss patch that I am using. This patch are used for custom kernels in order to provide initial EVM signed files in packages from package build server to desktop PCs. This patch add portable EVM signature version support (can be tested on files signed by evmctl util with "-i" flag). This patch based on previous Dmitry's patch (https://sourceforge.net/p/linux-ima/mailman/message/32987311/) cleaned and revised in order to provide only portable EVM signature version support and nothing more. Signed-off-by: Dmitry Kasatkin <d.k...@sa...> Signed-off-by: Mikhail Kurinnoi <vie...@vi...> --- a/security/integrity/digsig.c 2016-07-24 22:23:50.000000000 +0300 +++ b/security/integrity/digsig.c 2016-12-22 21:18:16.504929652 +0300 @@ -71,6 +71,7 @@ int integrity_digsig_verify(const unsign return digsig_verify(keyring[id], sig + 1, siglen - 1, digest, digestlen); case 2: + case 3: return asymmetric_verify(keyring[id], sig, siglen, digest, digestlen); } --- a/security/integrity/evm/evm.h 2016-07-24 22:23:50.000000000 +0300 +++ b/security/integrity/evm/evm.h 2016-12-22 21:19:02.225835301 +0300 @@ -48,7 +48,7 @@ int evm_calc_hmac(struct dentry *dentry, size_t req_xattr_value_len, char *digest); int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name, const char *req_xattr_value, - size_t req_xattr_value_len, char *digest); + size_t req_xattr_value_len, int version, char *digest); int evm_init_hmac(struct inode *inode, const struct xattr *xattr, char *hmac_val); int evm_init_secfs(void); --- a/security/integrity/evm/evm_crypto.c 2016-07-24 22:23:50.000000000 +0300 +++ b/security/integrity/evm/evm_crypto.c 2016-12-22 21:20:36.667704195 +0300 @@ -161,6 +161,23 @@ static void hmac_add_misc(struct shash_d crypto_shash_final(desc, digest); } +static void hmac_add_misc_digsig(struct shash_desc *desc, struct inode *inode, + char *digest) +{ + struct h_misc { + uid_t uid; + gid_t gid; + umode_t mode; + } hmac_misc; + + memset(&hmac_misc, 0, sizeof(hmac_misc)); + hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid); + hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid); + hmac_misc.mode = inode->i_mode; + crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc)); + crypto_shash_final(desc, digest); +} + /* * Calculate the HMAC value across the set of protected security xattrs. * @@ -210,7 +227,10 @@ static int evm_calc_hmac_or_hash(struct xattr_size = size; crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size); } - hmac_add_misc(desc, inode, digest); + if (type == EVM_IMA_XATTR_DIGSIG) + hmac_add_misc_digsig(desc, inode, digest); + else + hmac_add_misc(desc, inode, digest); out: kfree(xattr_value); @@ -228,10 +248,12 @@ int evm_calc_hmac(struct dentry *dentry, int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name, const char *req_xattr_value, size_t req_xattr_value_len, - char *digest) + int version, char *digest) { return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value, - req_xattr_value_len, IMA_XATTR_DIGEST, digest); + req_xattr_value_len, + version == 3 ? EVM_IMA_XATTR_DIGSIG : IMA_XATTR_DIGEST, + digest); } /* --- a/security/integrity/evm/evm_main.c 2016-12-22 21:16:15.159523000 +0300 +++ b/security/integrity/evm/evm_main.c 2016-12-22 21:21:42.445004576 +0300 @@ -120,7 +120,7 @@ static enum integrity_status evm_verify_ struct evm_ima_xattr_data *xattr_data = NULL; struct evm_ima_xattr_data calc; enum integrity_status evm_status = INTEGRITY_PASS; - int rc, xattr_len; + int rc, xattr_len, version; if (iint && iint->evm_status == INTEGRITY_PASS) return iint->evm_status; @@ -159,8 +159,9 @@ static enum integrity_status evm_verify_ rc = -EINVAL; break; case EVM_IMA_XATTR_DIGSIG: + version = ((const char *)xattr_data)[1]; rc = evm_calc_hash(dentry, xattr_name, xattr_value, - xattr_value_len, calc.digest); + xattr_value_len, version, calc.digest); if (rc) break; rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM, |
|
From: Mimi Z. <zo...@li...> - 2016-12-26 14:11:30
|
[cc'ing Dmitry Kasatkin] Hi Mikhail, On Sat, 2016-12-24 at 01:59 +0300, Mikhail Kurinnoi wrote: > I am not sure, if portable EVM signature version is still in > discussion or not, but, in case of someone interested in this feature > too, I propose to discuss patch that I am using. This patch are used > for custom kernels in order to provide initial EVM signed files in > packages from package build server to desktop PCs. A portable EVM signature, which can be included in an archive, is important. There were good reasons for including file system specific information in the HMAC calculation. By removing these fields, the new format does not provide the same security guarantees as the existing format. Instead of converting the EVM signature to an HMAC on first access, I would prefer that the new format never be written out to the file system, but converted to an HMAC after verification in evm_inode_post_setxattr(). This would provide the benefits of a portable EVM format, without loosing the existing security guarantees. Mimi |
|
From: Mikhail K. <vie...@vi...> - 2016-12-27 05:28:30
|
Hi, Mimi > On Sat, 2016-12-24 at 01:59 +0300, Mikhail Kurinnoi wrote: > > I am not sure, if portable EVM signature version is still in > > discussion or not, but, in case of someone interested in this > > feature too, I propose to discuss patch that I am using. This patch > > are used for custom kernels in order to provide initial EVM signed > > files in packages from package build server to desktop PCs. > > A portable EVM signature, which can be included in an archive, is > important. There were good reasons for including file system > specific information in the HMAC calculation. By removing these > fields, the new format does not provide the same security guarantees > as the existing format. > > Instead of converting the EVM signature to an HMAC on first access, I > would prefer that the new format never be written out to the file > system, but converted to an HMAC after verification in > evm_inode_post_setxattr(). This would provide the benefits of a > portable EVM format, without loosing the existing security guarantees. Yes, I think, we can use additional verification in evm_inode_post_setxattr() since we have xattr_value and could use it directly in evm_calc_hash() and integrity_digsig_verify(). In the same time, all work with EVM portable signature version must be prohibited in evm_verify_hmac() in order to prevent any work with it for sure. I will think more about this. I hope, Dmitry will take a part in this thread discussion, probably, he have some ideas too. -- Best regards, Mikhail Kurinnoi |
|
From: Mikhail K. <vie...@vi...> - 2017-01-05 21:42:10
|
version 2 changes:
1) evm_verify_hmac() prevent work with EVM portable signature. If file signed by EVM portable signature, evm_verify_hmac() return INTEGRITY_FAIL;
2) evm_inode_post_setxattr() now count on EVM portable signature.
If EVM digsig verification is ok - update securety.evm in order to have HMAC.
If EVM digsig verification fail, sign file with zero-filled EVM signature, we should have INTEGRITY_FAIL return from evm_verify_hmac() on next call for sure. Remove EVM signature in this case not enought, since in this case evm_verify_hmac() could return INTEGRITY_NOLABEL or INTEGRITY_NOXATTRS.
Patch could be tested evmctl with "i" flag.
Warning about usage: EVM portable signature should be used with patched archiver, that will add "security.evm" xattr after all xattrs are added to the file during unpack. I tested with "security.evm"+"security.ima" and it's working fine with tar default code, but "security.capability"+"security.evm"+"security.ima" fail because "security.evm" added before "security.capability".
Signed-off-by: Dmitry Kasatkin <d.k...@sa...>
Signed-off-by: Mikhail Kurinnoi <vie...@vi...>
--- a/security/integrity/digsig.c 2016-12-31 13:06:26.331828091 +0300
+++ b/security/integrity/digsig.c 2017-01-05 23:33:23.614541837 +0300
@@ -71,6 +71,7 @@ int integrity_digsig_verify(const unsign
return digsig_verify(keyring[id], sig + 1, siglen - 1,
digest, digestlen);
case 2:
+ case 3:
return asymmetric_verify(keyring[id], sig, siglen,
digest, digestlen);
}
--- a/security/integrity/evm/evm_crypto.c 2016-12-31 13:06:26.331828091 +0300
+++ b/security/integrity/evm/evm_crypto.c 2017-01-05 23:35:48.577389813 +0300
@@ -169,6 +169,23 @@ static void hmac_add_misc(struct shash_d
crypto_shash_final(desc, digest);
}
+static void hmac_add_misc_portable_digsig(struct shash_desc *desc, struct inode *inode,
+ char *digest)
+{
+ struct h_misc {
+ uid_t uid;
+ gid_t gid;
+ umode_t mode;
+ } hmac_misc;
+
+ memset(&hmac_misc, 0, sizeof(hmac_misc));
+ hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
+ hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
+ hmac_misc.mode = inode->i_mode;
+ crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
+ crypto_shash_final(desc, digest);
+}
+
/*
* Calculate the HMAC value across the set of protected security xattrs.
*
@@ -219,7 +236,10 @@ static int evm_calc_hmac_or_hash(struct
xattr_size = size;
crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size);
}
- hmac_add_misc(desc, inode, digest);
+ if (type == EVM_IMA_XATTR_DIGSIG)
+ hmac_add_misc_portable_digsig(desc, inode, digest);
+ else
+ hmac_add_misc(desc, inode, digest);
out:
kfree(xattr_value);
@@ -237,10 +257,12 @@ int evm_calc_hmac(struct dentry *dentry,
int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
const char *req_xattr_value, size_t req_xattr_value_len,
- char *digest)
+ int version, char *digest)
{
return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
- req_xattr_value_len, IMA_XATTR_DIGEST, digest);
+ req_xattr_value_len,
+ version == 3 ? EVM_IMA_XATTR_DIGSIG : IMA_XATTR_DIGEST,
+ digest);
}
/*
--- a/security/integrity/evm/evm.h 2016-11-29 01:32:07.248219481 +0300
+++ b/security/integrity/evm/evm.h 2017-01-05 23:34:06.551383033 +0300
@@ -48,7 +48,7 @@ int evm_calc_hmac(struct dentry *dentry,
size_t req_xattr_value_len, char *digest);
int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
const char *req_xattr_value,
- size_t req_xattr_value_len, char *digest);
+ size_t req_xattr_value_len, int version, char *digest);
int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
char *hmac_val);
int evm_init_secfs(void);
--- a/security/integrity/evm/evm_main.c 2016-12-31 13:06:26.331828091 +0300
+++ b/security/integrity/evm/evm_main.c 2017-01-05 23:37:07.266943910 +0300
@@ -116,7 +116,7 @@ static enum integrity_status evm_verify_
struct evm_ima_xattr_data *xattr_data = NULL;
struct evm_ima_xattr_data calc;
enum integrity_status evm_status = INTEGRITY_PASS;
- int rc, xattr_len;
+ int rc, xattr_len, version;
if (iint && iint->evm_status == INTEGRITY_PASS)
return iint->evm_status;
@@ -159,8 +159,11 @@ static enum integrity_status evm_verify_
rc = -EINVAL;
break;
case EVM_IMA_XATTR_DIGSIG:
+ version = ((const char *)xattr_data)[1];
+ if (version == 3)
+ break;
rc = evm_calc_hash(dentry, xattr_name, xattr_value,
- xattr_value_len, calc.digest);
+ xattr_value_len, version, calc.digest);
if (rc)
break;
rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
@@ -372,6 +375,44 @@ static void evm_reset_status(struct inod
iint->evm_status = INTEGRITY_UNKNOWN;
}
+void evm_update_portable_digsig(struct dentry *dentry, const char *xattr_name,
+ const void *xattr_value, size_t xattr_value_len)
+{
+ const struct evm_ima_xattr_data *xattr_data = xattr_value;
+ struct evm_ima_xattr_data calc;
+ struct evm_ima_xattr_data xattr_data_fail;
+ int version, rc;
+
+ if ((xattr_data->type != EVM_IMA_XATTR_DIGSIG) || \
+ (strcmp(xattr_name, XATTR_NAME_EVM) != 0))
+ return;
+
+ version = ((const char *)xattr_data)[1];
+ if (version != 3)
+ return;
+
+ evm_reset_status(dentry->d_inode);
+
+ rc = evm_calc_hash(dentry, xattr_name, xattr_value,
+ xattr_value_len, version, calc.digest);
+ if (rc)
+ goto digsig_fail;
+ rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
+ (const char *)xattr_data, xattr_value_len,
+ calc.digest, sizeof(calc.digest));
+ if (rc)
+ goto digsig_fail;
+
+ evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
+ return;
+
+digsig_fail:
+ memset(&xattr_data_fail, 0, sizeof(xattr_data_fail));
+ __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
+ &xattr_data_fail,
+ sizeof(xattr_data_fail), 0);
+}
+
/**
* evm_inode_post_setxattr - update 'security.evm' to reflect the changes
* @dentry: pointer to the affected dentry
@@ -388,8 +429,12 @@ static void evm_reset_status(struct inod
void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
const void *xattr_value, size_t xattr_value_len)
{
- if (!evm_initialized || (!evm_protected_xattr(xattr_name)
- && !posix_xattr_acl(xattr_name)))
+ if (!evm_initialized)
+ return;
+
+ evm_update_portable_digsig(dentry, xattr_name, xattr_value, xattr_value_len);
+
+ if (!evm_protected_xattr(xattr_name) && !posix_xattr_acl(xattr_name))
return;
evm_reset_status(dentry->d_inode);
|
|
From: Mikhail K. <vie...@vi...> - 2017-01-06 22:24:00
|
version 3 changes:
1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM signature verification failed.
Since, __vfs_setxattr_noperm() don't count on evm_inode_setxattr(), during file unpacking from atchive EVM zero-filled sign could be updated by IMA without EVM signature status checking. So, even if you have EVM portable signature signed by another key, IMA will update EVM signature to HMAC any way, no matter what you have in security.evm xattr or you don't have it at all.
2) Added integrity_audit_msg() in order to get some error in audit syslog on EVM portable signature verification fail.
version 2 changes:
1) evm_verify_hmac() prevent work with EVM portable signature. If file signed by EVM portable signature, evm_verify_hmac() return INTEGRITY_FAIL;
2) evm_inode_post_setxattr() now count on EVM portable signature.
~~~~~~~~~~~~~~~~~~
patch for tar v1.29 provide EVM portable signature support for testing.
~~~~~~~~~~~~~~~~~~
--- a/src/extract.c
+++ b/src/extract.c
@@ -391,6 +391,10 @@ set_stat (char const *file_name,
xattrs_xattrs_set (st, file_name, typeflag, 1);
xattrs_acls_set (st, file_name, typeflag);
xattrs_selinux_set (st, file_name, typeflag);
+ /* IMA signature. */
+ xattrs_xattrs_set (st, file_name, typeflag, 2);
+ /* EVM portable signature should be the last one. */
+ xattrs_xattrs_set (st, file_name, typeflag, 3);
}
/* For each entry H in the leading prefix of entries in HEAD that do
--- a/src/xattrs.c
+++ b/src/xattrs.c
@@ -668,9 +668,22 @@ xattrs_xattrs_set (struct tar_stat_info const *st,
For a regular files: all extended attributes are restored during
the first run except 'security.capability' which is restored in
'later_run == 1'. */
- if (typeflag == REGTYPE
- && later_run == !!strcmp (keyword, "security.capability"))
- continue;
+ if (typeflag == REGTYPE) {
+ if (later_run != 1 && !strcmp (keyword, "security.capability"))
+ continue;
+ if (later_run == 1 && strcmp (keyword, "security.capability"))
+ continue;
+ /* IMA signature. */
+ if (later_run != 2 && !strcmp (keyword, "security.ima"))
+ continue;
+ if (later_run == 2 && strcmp (keyword, "security.ima"))
+ continue;
+ /* EVM portable signature should be the last one. */
+ if (later_run != 3 && !strcmp (keyword, "security.evm"))
+ continue;
+ if (later_run == 3 && strcmp (keyword, "security.evm"))
+ continue;
+ }
if (xattrs_masked_out (keyword, false /* extracting */ ))
/* we don't want to restore this keyword */
~~~~~~~~~~~~~~~~~~
Signed-off-by: Dmitry Kasatkin <d.k...@sa...>
Signed-off-by: Mikhail Kurinnoi <vie...@vi...>
--- a/security/integrity/digsig.c 2016-12-31 13:06:26.331828091 +0300
+++ b/security/integrity/digsig.c 2017-01-05 23:33:23.614541837 +0300
@@ -71,6 +71,7 @@ int integrity_digsig_verify(const unsign
return digsig_verify(keyring[id], sig + 1, siglen - 1,
digest, digestlen);
case 2:
+ case 3:
return asymmetric_verify(keyring[id], sig, siglen,
digest, digestlen);
}
--- a/security/integrity/evm/evm_crypto.c 2016-12-31 13:06:26.331828091 +0300
+++ b/security/integrity/evm/evm_crypto.c 2017-01-05 23:35:48.577389813 +0300
@@ -169,6 +169,23 @@ static void hmac_add_misc(struct shash_d
crypto_shash_final(desc, digest);
}
+static void hmac_add_misc_portable_digsig(struct shash_desc *desc, struct inode *inode,
+ char *digest)
+{
+ struct h_misc {
+ uid_t uid;
+ gid_t gid;
+ umode_t mode;
+ } hmac_misc;
+
+ memset(&hmac_misc, 0, sizeof(hmac_misc));
+ hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
+ hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
+ hmac_misc.mode = inode->i_mode;
+ crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
+ crypto_shash_final(desc, digest);
+}
+
/*
* Calculate the HMAC value across the set of protected security xattrs.
*
@@ -219,7 +236,10 @@ static int evm_calc_hmac_or_hash(struct
xattr_size = size;
crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size);
}
- hmac_add_misc(desc, inode, digest);
+ if (type == EVM_IMA_XATTR_DIGSIG)
+ hmac_add_misc_portable_digsig(desc, inode, digest);
+ else
+ hmac_add_misc(desc, inode, digest);
out:
kfree(xattr_value);
@@ -237,10 +257,12 @@ int evm_calc_hmac(struct dentry *dentry,
int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
const char *req_xattr_value, size_t req_xattr_value_len,
- char *digest)
+ int version, char *digest)
{
return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
- req_xattr_value_len, IMA_XATTR_DIGEST, digest);
+ req_xattr_value_len,
+ version == 3 ? EVM_IMA_XATTR_DIGSIG : IMA_XATTR_DIGEST,
+ digest);
}
/*
--- a/security/integrity/evm/evm.h 2016-11-29 01:32:07.248219481 +0300
+++ b/security/integrity/evm/evm.h 2017-01-05 23:34:06.551383033 +0300
@@ -48,7 +48,7 @@ int evm_calc_hmac(struct dentry *dentry,
size_t req_xattr_value_len, char *digest);
int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
const char *req_xattr_value,
- size_t req_xattr_value_len, char *digest);
+ size_t req_xattr_value_len, int version, char *digest);
int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
char *hmac_val);
int evm_init_secfs(void);
--- a/security/integrity/evm/evm_main.c 2016-12-31 13:06:26.331828091 +0300
+++ b/security/integrity/evm/evm_main.c 2017-01-07 00:39:24.414082237 +0300
@@ -116,7 +116,7 @@ static enum integrity_status evm_verify_
struct evm_ima_xattr_data *xattr_data = NULL;
struct evm_ima_xattr_data calc;
enum integrity_status evm_status = INTEGRITY_PASS;
- int rc, xattr_len;
+ int rc, xattr_len, version;
if (iint && iint->evm_status == INTEGRITY_PASS)
return iint->evm_status;
@@ -159,8 +159,11 @@ static enum integrity_status evm_verify_
rc = -EINVAL;
break;
case EVM_IMA_XATTR_DIGSIG:
+ version = ((const char *)xattr_data)[1];
+ if (version == 3)
+ break;
rc = evm_calc_hash(dentry, xattr_name, xattr_value,
- xattr_value_len, calc.digest);
+ xattr_value_len, version, calc.digest);
if (rc)
break;
rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
@@ -236,7 +239,7 @@ enum integrity_status evm_verifyxattr(st
void *xattr_value, size_t xattr_value_len,
struct integrity_iint_cache *iint)
{
- if (!evm_initialized || !evm_protected_xattr(xattr_name))
+ if (!evm_initialized || (xattr_name && !evm_protected_xattr(xattr_name)))
return INTEGRITY_UNKNOWN;
if (!iint) {
@@ -372,6 +375,47 @@ static void evm_reset_status(struct inod
iint->evm_status = INTEGRITY_UNKNOWN;
}
+void evm_update_portable_digsig(struct dentry *dentry, const char *xattr_name,
+ const void *xattr_value, size_t xattr_value_len)
+{
+ const struct evm_ima_xattr_data *xattr_data = xattr_value;
+ struct evm_ima_xattr_data calc;
+ struct evm_ima_xattr_data xattr_data_fail;
+ int version, rc;
+
+ if ((xattr_data->type != EVM_IMA_XATTR_DIGSIG) || \
+ (strcmp(xattr_name, XATTR_NAME_EVM) != 0))
+ return;
+
+ version = ((const char *)xattr_data)[1];
+ if (version != 3)
+ return;
+
+ evm_reset_status(dentry->d_inode);
+
+ rc = evm_calc_hash(dentry, xattr_name, xattr_value,
+ xattr_value_len, version, calc.digest);
+ if (rc)
+ goto digsig_fail;
+ rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
+ (const char *)xattr_data, xattr_value_len,
+ calc.digest, sizeof(calc.digest));
+ if (rc)
+ goto digsig_fail;
+
+ evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
+ return;
+
+digsig_fail:
+ integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
+ dentry->d_name.name, "update_metadata",
+ integrity_status_msg[INTEGRITY_FAIL], -EPERM, 0);
+ memset(&xattr_data_fail, 0, sizeof(xattr_data_fail));
+ __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
+ &xattr_data_fail,
+ sizeof(xattr_data_fail), 0);
+}
+
/**
* evm_inode_post_setxattr - update 'security.evm' to reflect the changes
* @dentry: pointer to the affected dentry
@@ -388,8 +432,12 @@ static void evm_reset_status(struct inod
void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
const void *xattr_value, size_t xattr_value_len)
{
- if (!evm_initialized || (!evm_protected_xattr(xattr_name)
- && !posix_xattr_acl(xattr_name)))
+ if (!evm_initialized)
+ return;
+
+ evm_update_portable_digsig(dentry, xattr_name, xattr_value, xattr_value_len);
+
+ if (!evm_protected_xattr(xattr_name) && !posix_xattr_acl(xattr_name))
return;
evm_reset_status(dentry->d_inode);
--- a/security/integrity/ima/ima_appraise.c 2017-01-03 19:30:16.340669333 +0300
+++ b/security/integrity/ima/ima_appraise.c 2017-01-07 00:39:03.525673274 +0300
@@ -48,8 +48,13 @@ static int ima_fix_xattr(struct dentry *
struct integrity_iint_cache *iint)
{
int rc, offset;
+ enum integrity_status status = INTEGRITY_UNKNOWN;
u8 algo = iint->ima_hash->algo;
+ status = evm_verifyxattr(dentry, NULL, NULL, 0, iint);
+ if (status == INTEGRITY_FAIL)
+ return -EINVAL;
+
if (algo <= HASH_ALGO_SHA1) {
offset = 1;
iint->ima_hash->xattr.sha1.type = IMA_XATTR_DIGEST;
|
|
From: Mimi Z. <zo...@li...> - 2017-01-09 13:41:02
|
On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote:
> version 3 changes:
> 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM
> signature verification failed.
This can't be right. The boot comnand line option "ima_appraise=fix"
allows IMA to go into a special mode to "fix" the xattrs.
> Since, __vfs_setxattr_noperm() don't count on evm_inode_setxattr(),
> during file unpacking from atchive EVM zero-filled sign could be
> updated by IMA without EVM signature status checking. So, even if you
> have EVM portable signature signed by another key, IMA will update EVM
> signature to HMAC any way, no matter what you have in security.evm
> xattr or you don't have it at all.
EVM and IMA are two separate modules, which communicate with each other
via the evm_verifyxattr() API. Changes to EVM, should not affect IMA.
If they do, then something is not right.
> 2) Added integrity_audit_msg() in order to get some error in audit
> syslog on EVM portable signature verification fail.
>
> version 2 changes:
> 1) evm_verify_hmac() prevent work with EVM portable signature. If file
> signed by EVM portable signature, evm_verify_hmac() return
> INTEGRITY_FAIL;
> 2) evm_inode_post_setxattr() now count on EVM portable signature.
I originally suggested that the portable EVM xattr should be converted
before it is written (in evm_inode_post_setxattr()), but by the time we
get to evm_inode_post_setxattr(), the portable EVM xattr has already
been written. Can the verification be done earlier in
evm_inode_setxattr(), before the portable EVM xattr is written?
Stefan Berger, a colleague, asked if the portable EVM xattr is never
written to disk or immediately converted to an HMAC, how will it be
included in an archive?
> ~~~~~~~~~~~~~~~~~~
> patch for tar v1.29 provide EVM portable signature support for testing.
> ~~~~~~~~~~~~~~~~~~
> --- a/src/extract.c
> +++ b/src/extract.c
> @@ -391,6 +391,10 @@ set_stat (char const *file_name,
> xattrs_xattrs_set (st, file_name, typeflag, 1);
> xattrs_acls_set (st, file_name, typeflag);
> xattrs_selinux_set (st, file_name, typeflag);
> + /* IMA signature. */
> + xattrs_xattrs_set (st, file_name, typeflag, 2);
> + /* EVM portable signature should be the last one. */
> + xattrs_xattrs_set (st, file_name, typeflag, 3);
> }
>
> /* For each entry H in the leading prefix of entries in HEAD that do
>
> --- a/src/xattrs.c
> +++ b/src/xattrs.c
> @@ -668,9 +668,22 @@ xattrs_xattrs_set (struct tar_stat_info const *st,
> For a regular files: all extended attributes are restored during
> the first run except 'security.capability' which is restored in
> 'later_run == 1'. */
> - if (typeflag == REGTYPE
> - && later_run == !!strcmp (keyword, "security.capability"))
> - continue;
> + if (typeflag == REGTYPE) {
> + if (later_run != 1 && !strcmp (keyword, "security.capability"))
> + continue;
> + if (later_run == 1 && strcmp (keyword, "security.capability"))
> + continue;
> + /* IMA signature. */
> + if (later_run != 2 && !strcmp (keyword, "security.ima"))
> + continue;
> + if (later_run == 2 && strcmp (keyword, "security.ima"))
> + continue;
> + /* EVM portable signature should be the last one. */
> + if (later_run != 3 && !strcmp (keyword, "security.evm"))
> + continue;
> + if (later_run == 3 && strcmp (keyword, "security.evm"))
> + continue;
> + }
>
> if (xattrs_masked_out (keyword, false /* extracting */ ))
> /* we don't want to restore this keyword */
>
> ~~~~~~~~~~~~~~~~~~
>
>
>
>
> Signed-off-by: Dmitry Kasatkin <d.k...@sa...>
> Signed-off-by: Mikhail Kurinnoi <vie...@vi...>
>
> --- a/security/integrity/digsig.c 2016-12-31 13:06:26.331828091 +0300
> +++ b/security/integrity/digsig.c 2017-01-05 23:33:23.614541837 +0300
> @@ -71,6 +71,7 @@ int integrity_digsig_verify(const unsign
> return digsig_verify(keyring[id], sig + 1, siglen - 1,
> digest, digestlen);
> case 2:
> + case 3:
> return asymmetric_verify(keyring[id], sig, siglen,
> digest, digestlen);
> }
> --- a/security/integrity/evm/evm_crypto.c 2016-12-31 13:06:26.331828091 +0300
> +++ b/security/integrity/evm/evm_crypto.c 2017-01-05 23:35:48.577389813 +0300
> @@ -169,6 +169,23 @@ static void hmac_add_misc(struct shash_d
> crypto_shash_final(desc, digest);
> }
>
> +static void hmac_add_misc_portable_digsig(struct shash_desc *desc, struct inode *inode,
> + char *digest)
> +{
> + struct h_misc {
> + uid_t uid;
> + gid_t gid;
> + umode_t mode;
> + } hmac_misc;
> +
> + memset(&hmac_misc, 0, sizeof(hmac_misc));
> + hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
> + hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
> + hmac_misc.mode = inode->i_mode;
> + crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
> + crypto_shash_final(desc, digest);
> +}
> +
> /*
> * Calculate the HMAC value across the set of protected security xattrs.
> *
> @@ -219,7 +236,10 @@ static int evm_calc_hmac_or_hash(struct
> xattr_size = size;
> crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size);
> }
> - hmac_add_misc(desc, inode, digest);
> + if (type == EVM_IMA_XATTR_DIGSIG)
> + hmac_add_misc_portable_digsig(desc, inode, digest);
Calling "hmac_add_misc_portable_digsig" for all signatures is confusing.
It should only be called for the new portable version.
> + else
> + hmac_add_misc(desc, inode, digest);
>
> out:
> kfree(xattr_value);
> @@ -237,10 +257,12 @@ int evm_calc_hmac(struct dentry *dentry,
>
> int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
> const char *req_xattr_value, size_t req_xattr_value_len,
> - char *digest)
> + int version, char *digest)
> {
> return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
> - req_xattr_value_len, IMA_XATTR_DIGEST, digest);
> + req_xattr_value_len,
> + version == 3 ? EVM_IMA_XATTR_DIGSIG : IMA_XATTR_DIGEST,
> + digest);
> }
>
> /*
> --- a/security/integrity/evm/evm.h 2016-11-29 01:32:07.248219481 +0300
> +++ b/security/integrity/evm/evm.h 2017-01-05 23:34:06.551383033 +0300
> @@ -48,7 +48,7 @@ int evm_calc_hmac(struct dentry *dentry,
> size_t req_xattr_value_len, char *digest);
> int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
> const char *req_xattr_value,
> - size_t req_xattr_value_len, char *digest);
> + size_t req_xattr_value_len, int version, char *digest);
> int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
> char *hmac_val);
> int evm_init_secfs(void);
> --- a/security/integrity/evm/evm_main.c 2016-12-31 13:06:26.331828091 +0300
> +++ b/security/integrity/evm/evm_main.c 2017-01-07 00:39:24.414082237 +0300
> @@ -116,7 +116,7 @@ static enum integrity_status evm_verify_
> struct evm_ima_xattr_data *xattr_data = NULL;
> struct evm_ima_xattr_data calc;
> enum integrity_status evm_status = INTEGRITY_PASS;
> - int rc, xattr_len;
> + int rc, xattr_len, version;
>
> if (iint && iint->evm_status == INTEGRITY_PASS)
> return iint->evm_status;
> @@ -159,8 +159,11 @@ static enum integrity_status evm_verify_
> rc = -EINVAL;
> break;
> case EVM_IMA_XATTR_DIGSIG:
> + version = ((const char *)xattr_data)[1];
> + if (version == 3)
Please use and define an enumeration.
> + break;
> rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> - xattr_value_len, calc.digest);
> + xattr_value_len, version, calc.digest);
> if (rc)
> break;
> rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> @@ -236,7 +239,7 @@ enum integrity_status evm_verifyxattr(st
> void *xattr_value, size_t xattr_value_len,
> struct integrity_iint_cache *iint)
> {
> - if (!evm_initialized || !evm_protected_xattr(xattr_name))
> + if (!evm_initialized || (xattr_name && !evm_protected_xattr(xattr_name)))
> return INTEGRITY_UNKNOWN;
>
> if (!iint) {
> @@ -372,6 +375,47 @@ static void evm_reset_status(struct inod
> iint->evm_status = INTEGRITY_UNKNOWN;
> }
>
> +void evm_update_portable_digsig(struct dentry *dentry, const char *xattr_name,
> + const void *xattr_value, size_t xattr_value_len)
> +{
> + const struct evm_ima_xattr_data *xattr_data = xattr_value;
> + struct evm_ima_xattr_data calc;
> + struct evm_ima_xattr_data xattr_data_fail;
> + int version, rc;
> +
> + if ((xattr_data->type != EVM_IMA_XATTR_DIGSIG) || \
> + (strcmp(xattr_name, XATTR_NAME_EVM) != 0))
> + return;
I think it would be clearer if the test was in the caller.
> + version = ((const char *)xattr_data)[1];
> + if (version != 3)
> + return;
Please use an enumeration.
> + evm_reset_status(dentry->d_inode);
> +
> + rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> + xattr_value_len, version, calc.digest);
> + if (rc)
> + goto digsig_fail;
> + rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> + (const char *)xattr_data, xattr_value_len,
> + calc.digest, sizeof(calc.digest));
> + if (rc)
> + goto digsig_fail;
> +
> + evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
Instead of updating the portable EVM xattr, this function should just
verify the portable EVM xattr?
> + return;
> +
> +digsig_fail:
> + integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
> + dentry->d_name.name, "update_metadata",
> + integrity_status_msg[INTEGRITY_FAIL], -EPERM, 0);
> + memset(&xattr_data_fail, 0, sizeof(xattr_data_fail));
> + __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
> + &xattr_data_fail,
> + sizeof(xattr_data_fail), 0);
> +}
> +
> /**
> * evm_inode_post_setxattr - update 'security.evm' to reflect the changes
> * @dentry: pointer to the affected dentry
> @@ -388,8 +432,12 @@ static void evm_reset_status(struct inod
> void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
> const void *xattr_value, size_t xattr_value_len)
> {
> - if (!evm_initialized || (!evm_protected_xattr(xattr_name)
> - && !posix_xattr_acl(xattr_name)))
> + if (!evm_initialized)
> + return;
> +
> + evm_update_portable_digsig(dentry, xattr_name, xattr_value, xattr_value_len);
If a "protected" xattr was written, security.evm is updated to reflect
the change below.
It would be clearer to move the "security.evm" test here, call to verify
the portable xattr*, and then fall through to do the normal xattr update
below.
*assuming that the portable EVM xattr was written to disk.
Mimi
|
|
From: Mikhail K. <vie...@vi...> - 2017-01-09 21:49:25
|
В Mon, 09 Jan 2017 08:40:45 -0500
Mimi Zohar <zo...@li...> пишет:
> On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote:
> > Signed-off-by: Dmitry Kasatkin <d.k...@sa...>
> > Signed-off-by: Mikhail Kurinnoi <vie...@vi...>
> >
> > --- a/security/integrity/digsig.c 2016-12-31
> > 13:06:26.331828091 +0300 +++ b/security/integrity/digsig.c
> > 2017-01-05 23:33:23.614541837 +0300 @@ -71,6 +71,7 @@ int
> > integrity_digsig_verify(const unsign return
> > digsig_verify(keyring[id], sig + 1, siglen - 1, digest, digestlen);
> > case 2:
> > + case 3:
> > return asymmetric_verify(keyring[id], sig, siglen,
> > digest, digestlen);
> > }
> > --- a/security/integrity/evm/evm_crypto.c 2016-12-31
> > 13:06:26.331828091 +0300 +++
> > b/security/integrity/evm/evm_crypto.c 2017-01-05
> > 23:35:48.577389813 +0300 @@ -169,6 +169,23 @@ static void
> > hmac_add_misc(struct shash_d crypto_shash_final(desc, digest); }
> >
> > +static void hmac_add_misc_portable_digsig(struct shash_desc *desc,
> > struct inode *inode,
> > + char *digest)
> > +{
> > + struct h_misc {
> > + uid_t uid;
> > + gid_t gid;
> > + umode_t mode;
> > + } hmac_misc;
> > +
> > + memset(&hmac_misc, 0, sizeof(hmac_misc));
> > + hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
> > + hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
> > + hmac_misc.mode = inode->i_mode;
> > + crypto_shash_update(desc, (const u8 *)&hmac_misc,
> > sizeof(hmac_misc));
> > + crypto_shash_final(desc, digest);
> > +}
> > +
> > /*
> > * Calculate the HMAC value across the set of protected security
> > xattrs. *
> > @@ -219,7 +236,10 @@ static int evm_calc_hmac_or_hash(struct
> > xattr_size = size;
> > crypto_shash_update(desc, (const u8 *)xattr_value,
> > xattr_size); }
> > - hmac_add_misc(desc, inode, digest);
> > + if (type == EVM_IMA_XATTR_DIGSIG)
> > + hmac_add_misc_portable_digsig(desc, inode,
> > digest);
>
> Calling "hmac_add_misc_portable_digsig" for all signatures is
> confusing. It should only be called for the new portable version.
It called for the new portable version only. Dmitry just re-use
available constant from enumeration that not used in this part of code.
Will be fixed.
> > + else
> > + hmac_add_misc(desc, inode, digest);
> >
> > out:
> > kfree(xattr_value);
> > @@ -237,10 +257,12 @@ int evm_calc_hmac(struct dentry *dentry,
> >
> > int evm_calc_hash(struct dentry *dentry, const char
> > *req_xattr_name, const char *req_xattr_value, size_t
> > req_xattr_value_len,
> > - char *digest)
> > + int version, char *digest)
> > {
> > return evm_calc_hmac_or_hash(dentry, req_xattr_name,
> > req_xattr_value,
> > - req_xattr_value_len,
> > IMA_XATTR_DIGEST, digest);
> > + req_xattr_value_len,
> > + version == 3 ?
> > EVM_IMA_XATTR_DIGSIG : IMA_XATTR_DIGEST,
> > + digest);
> > }
> >
> > /*
> > --- a/security/integrity/evm/evm.h 2016-11-29
> > 01:32:07.248219481 +0300 +++ b/security/integrity/evm/evm.h
> > 2017-01-05 23:34:06.551383033 +0300 @@ -48,7 +48,7 @@ int
> > evm_calc_hmac(struct dentry *dentry, size_t req_xattr_value_len,
> > char *digest); int evm_calc_hash(struct dentry *dentry, const char
> > *req_xattr_name, const char *req_xattr_value,
> > - size_t req_xattr_value_len, char *digest);
> > + size_t req_xattr_value_len, int version, char
> > *digest); int evm_init_hmac(struct inode *inode, const struct xattr
> > *xattr, char *hmac_val);
> > int evm_init_secfs(void);
> > --- a/security/integrity/evm/evm_main.c 2016-12-31
> > 13:06:26.331828091 +0300 +++
> > b/security/integrity/evm/evm_main.c 2017-01-07
> > 00:39:24.414082237 +0300 @@ -116,7 +116,7 @@ static enum
> > integrity_status evm_verify_ struct evm_ima_xattr_data *xattr_data
> > = NULL; struct evm_ima_xattr_data calc; enum integrity_status
> > evm_status = INTEGRITY_PASS;
> > - int rc, xattr_len;
> > + int rc, xattr_len, version;
> >
> > if (iint && iint->evm_status == INTEGRITY_PASS)
> > return iint->evm_status;
> > @@ -159,8 +159,11 @@ static enum integrity_status evm_verify_
> > rc = -EINVAL;
> > break;
> > case EVM_IMA_XATTR_DIGSIG:
> > + version = ((const char *)xattr_data)[1];
> > + if (version == 3)
>
> Please use and define an enumeration.
Will be fixed.
> > + break;
> > rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> > - xattr_value_len, calc.digest);
> > + xattr_value_len, version,
> > calc.digest); if (rc)
> > break;
> > rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> > @@ -236,7 +239,7 @@ enum integrity_status evm_verifyxattr(st
> > void *xattr_value, size_t
> > xattr_value_len, struct integrity_iint_cache *iint)
> > {
> > - if (!evm_initialized || !evm_protected_xattr(xattr_name))
> > + if (!evm_initialized || (xattr_name
> > && !evm_protected_xattr(xattr_name))) return INTEGRITY_UNKNOWN;
> >
> > if (!iint) {
> > @@ -372,6 +375,47 @@ static void evm_reset_status(struct inod
> > iint->evm_status = INTEGRITY_UNKNOWN;
> > }
> >
> > +void evm_update_portable_digsig(struct dentry *dentry, const char
> > *xattr_name,
> > + const void *xattr_value, size_t
> > xattr_value_len) +{
> > + const struct evm_ima_xattr_data *xattr_data = xattr_value;
> > + struct evm_ima_xattr_data calc;
> > + struct evm_ima_xattr_data xattr_data_fail;
> > + int version, rc;
> > +
> > + if ((xattr_data->type != EVM_IMA_XATTR_DIGSIG) || \
> > + (strcmp(xattr_name, XATTR_NAME_EVM) != 0))
> > + return;
>
> I think it would be clearer if the test was in the caller.
Will be fixed.
> > + version = ((const char *)xattr_data)[1];
> > + if (version != 3)
> > + return;
>
> Please use an enumeration.
Will be fixed.
> > + evm_reset_status(dentry->d_inode);
> > +
> > + rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> > + xattr_value_len, version, calc.digest);
> > + if (rc)
> > + goto digsig_fail;
> > + rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> > + (const char *)xattr_data,
> > xattr_value_len,
> > + calc.digest,
> > sizeof(calc.digest));
> > + if (rc)
> > + goto digsig_fail;
> > +
> > + evm_update_evmxattr(dentry, xattr_name, xattr_value,
> > xattr_value_len);
>
> Instead of updating the portable EVM xattr, this function should just
> verify the portable EVM xattr?
No, this function should verify and update EVM xattr depending from
results of verification.
I see the point. Will be fixed.
> > + return;
> > +
> > +digsig_fail:
> > + integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > d_backing_inode(dentry),
> > + dentry->d_name.name, "update_metadata",
> > + integrity_status_msg[INTEGRITY_FAIL],
> > -EPERM, 0);
> > + memset(&xattr_data_fail, 0, sizeof(xattr_data_fail));
> > + __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
> > + &xattr_data_fail,
> > +
> > sizeof(xattr_data_fail), 0); +}
> > +
> > /**
> > * evm_inode_post_setxattr - update 'security.evm' to reflect the
> > changes
> > * @dentry: pointer to the affected dentry
> > @@ -388,8 +432,12 @@ static void evm_reset_status(struct inod
> > void evm_inode_post_setxattr(struct dentry *dentry, const char
> > *xattr_name, const void *xattr_value, size_t xattr_value_len)
> > {
> > - if (!evm_initialized || (!evm_protected_xattr(xattr_name)
> > - && !posix_xattr_acl(xattr_name)))
> > + if (!evm_initialized)
> > + return;
> > +
> > + evm_update_portable_digsig(dentry, xattr_name,
> > xattr_value, xattr_value_len);
>
> If a "protected" xattr was written, security.evm is updated to reflect
> the change below.
>
> It would be clearer to move the "security.evm" test here, call to
> verify the portable xattr*, and then fall through to do the normal
> xattr update below.
>
> *assuming that the portable EVM xattr was written to disk.
Will be fixed.
--
Best regards,
Mikhail Kurinnoi
|
|
From: Mikhail K. <vie...@vi...> - 2017-01-09 21:06:30
|
В Mon, 09 Jan 2017 08:40:45 -0500
Mimi Zohar <zo...@li...> пишет:
> On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote:
> > version 3 changes:
> > 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM
> > signature verification failed.
>
> This can't be right. The boot comnand line option "ima_appraise=fix"
> allows IMA to go into a special mode to "fix" the xattrs.
>
> > Since, __vfs_setxattr_noperm() don't count on evm_inode_setxattr(),
> > during file unpacking from atchive EVM zero-filled sign could be
> > updated by IMA without EVM signature status checking. So, even if
> > you have EVM portable signature signed by another key, IMA will
> > update EVM signature to HMAC any way, no matter what you have in
> > security.evm xattr or you don't have it at all.
>
> EVM and IMA are two separate modules, which communicate with each
> other via the evm_verifyxattr() API. Changes to EVM, should not
> affect IMA. If they do, then something is not right.
Something is wrong, because if we have FS mounted with iversion flag, we
call ima_file_free() -> ima_check_last_writer(), that don't care about
evm_verifyxattr() API at all.
I have this issue without IMA/EVM fix mode. I mean, I faced with this
issue on IMA/EVM normal mode during archive unpacking with EVM
portable signature signed by wrong cert, that was replaced on EVM
zero-filled signature with evm_reset_status(). And after that IMA
update inode EVM xattr with proper HMAC by ima_file_free() call.
> > 2) Added integrity_audit_msg() in order to get some error in audit
> > syslog on EVM portable signature verification fail.
> >
> > version 2 changes:
> > 1) evm_verify_hmac() prevent work with EVM portable signature. If
> > file signed by EVM portable signature, evm_verify_hmac() return
> > INTEGRITY_FAIL;
> > 2) evm_inode_post_setxattr() now count on EVM portable signature.
>
> I originally suggested that the portable EVM xattr should be converted
> before it is written (in evm_inode_post_setxattr()), but by the time
> we get to evm_inode_post_setxattr(), the portable EVM xattr has
> already been written. Can the verification be done earlier in
> evm_inode_setxattr(), before the portable EVM xattr is written?
We could do everything in evm_inode_setxattr() and return some special
error code into vfs_setxattr() that will:
1) don't allow call __vfs_setxattr_noperm();
2) prevent special error code return and return 0 instead in the same
time.
What I mean:
./security/integrity/evm/evm_main.c
int evm_inode_setxattr(struct dentry *dentry, const char *xattr_name,
const void *xattr_value, size_t xattr_value_len)
{
const struct evm_ima_xattr_data *xattr_data = xattr_value;
if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) {
if (!xattr_value_len)
return -EINVAL;
if (xattr_data->type != EVM_IMA_XATTR_DIGSIG)
return -EPERM;
+ if (evm_update_portable_digsig(dentry, xattr_name,
+ xattr_value,
+ xattr_value_len))
+ return -ESPECIALERRORCODE;
}
return evm_protect_xattr(dentry, xattr_name, xattr_value,
xattr_value_len);
}
./fs/xattr.c
int
vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
size_t size, int flags)
{
...
out:
inode_unlock(inode);
+ return (error == -ESPECIALERRORCODE) ? 0 : error;
}
But, what error code we could use in this case?
> Stefan Berger, a colleague, asked if the portable EVM xattr is never
> written to disk or immediately converted to an HMAC, how will it be
> included in an archive?
If EVM is enabled, the only way is patched archiver that will store EVM
portable signature during file packing instead of HMAC (I am testing
such patch now for tar with libimaevm). The idea is - prevent HMAC to
be stored in archive (store EVM portable digsig in this case), since we
can copy only EVM digsig xattr from archive during unpacking any way.
But, we also have security issue here. Since EVM xattr could be removed
manually from archive, and during unpacking new file will have HMAC, we
must have archiver patched on unpacking too (on dest computer), that
will count on protected xattrs and provide some fake EVM portable
signature if we have protected xattrs but don't have EVM xattr itself,
or we could have security fail here. Probably, this should be controlled
by archiver flags during unpacking (demand EVM xattr or not).
--
Best regards,
Mikhail Kurinnoi
|
|
From: Mimi Z. <zo...@li...> - 2017-01-09 23:11:16
|
On Tue, 2017-01-10 at 00:06 +0300, Mikhail Kurinnoi wrote:
> В Mon, 09 Jan 2017 08:40:45 -0500
> Mimi Zohar <zo...@li...> пишет:
>
> > On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote:
> > > version 3 changes:
> > > 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM
> > > signature verification failed.
> >
> > This can't be right. The boot comnand line option "ima_appraise=fix"
> > allows IMA to go into a special mode to "fix" the xattrs.
> >
> > > Since, __vfs_setxattr_noperm() don't count on evm_inode_setxattr(),
> > > during file unpacking from atchive EVM zero-filled sign could be
> > > updated by IMA without EVM signature status checking. So, even if
> > > you have EVM portable signature signed by another key, IMA will
> > > update EVM signature to HMAC any way, no matter what you have in
> > > security.evm xattr or you don't have it at all.
> >
> > EVM and IMA are two separate modules, which communicate with each
> > other via the evm_verifyxattr() API. Changes to EVM, should not
> > affect IMA. If they do, then something is not right.
>
> Something is wrong, because if we have FS mounted with iversion flag, we
> call ima_file_free() -> ima_check_last_writer(), that don't care about
> evm_verifyxattr() API at all.
This is working as designed. The file integrity verification occurred
on file open. On __fput() the file hash is updated, which causes
security.evm to be updated.
> I have this issue without IMA/EVM fix mode. I mean, I faced with this
> issue on IMA/EVM normal mode during archive unpacking with EVM
> portable signature signed by wrong cert, that was replaced on EVM
> zero-filled signature with evm_reset_status(). And after that IMA
> update inode EVM xattr with proper HMAC by ima_file_free() call.
>
> > > 2) Added integrity_audit_msg() in order to get some error in audit
> > > syslog on EVM portable signature verification fail.
> > >
> > > version 2 changes:
> > > 1) evm_verify_hmac() prevent work with EVM portable signature. If
> > > file signed by EVM portable signature, evm_verify_hmac() return
> > > INTEGRITY_FAIL;
> > > 2) evm_inode_post_setxattr() now count on EVM portable signature.
> >
> > I originally suggested that the portable EVM xattr should be converted
> > before it is written (in evm_inode_post_setxattr()), but by the time
> > we get to evm_inode_post_setxattr(), the portable EVM xattr has
> > already been written. Can the verification be done earlier in
> > evm_inode_setxattr(), before the portable EVM xattr is written?
>
> We could do everything in evm_inode_setxattr() and return some special
> error code into vfs_setxattr() that will:
> 1) don't allow call __vfs_setxattr_noperm();
> 2) prevent special error code return and return 0 instead in the same
> time.
I'm wondering if there will be an issue with the order in which the
security xattrs are installed. For example, what guarantees that
security.ima is installed before security.evm?
Currently, after each xattr is written, the security.evm is updated. Do
we remove the existing security.evm before verifying the portable
security.evm?
> What I mean:
>
> ./security/integrity/evm/evm_main.c
>
> int evm_inode_setxattr(struct dentry *dentry, const char *xattr_name,
> const void *xattr_value, size_t xattr_value_len)
> {
> const struct evm_ima_xattr_data *xattr_data = xattr_value;
>
> if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) {
> if (!xattr_value_len)
> return -EINVAL;
> if (xattr_data->type != EVM_IMA_XATTR_DIGSIG)
> return -EPERM;
> + if (evm_update_portable_digsig(dentry, xattr_name,
> + xattr_value,
> + xattr_value_len))
> + return -ESPECIALERRORCODE;
> }
> return evm_protect_xattr(dentry, xattr_name, xattr_value,
> xattr_value_len);
> }
>
> ./fs/xattr.c
>
> int
> vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
> size_t size, int flags)
> {
>
> ...
>
> out:
> inode_unlock(inode);
> + return (error == -ESPECIALERRORCODE) ? 0 : error;
> }
>
> But, what error code we could use in this case?
>
>
> > Stefan Berger, a colleague, asked if the portable EVM xattr is never
> > written to disk or immediately converted to an HMAC, how will it be
> > included in an archive?
>
> If EVM is enabled, the only way is patched archiver that will store EVM
> portable signature during file packing instead of HMAC (I am testing
> such patch now for tar with libimaevm). The idea is - prevent HMAC to
> be stored in archive (store EVM portable digsig in this case), since we
> can copy only EVM digsig xattr from archive during unpacking any way.
The archiver is normally general purpose. Getting EVM or IMA specific
code upstreamed in the archive code will be difficult.
I think adding support for a portable EVM signature is important, but
there seems to be a lot of details that need to be resolved first.
We've touched on a few of them in this thread.
Mimi
> But, we also have security issue here. Since EVM xattr could be removed
> manually from archive, and during unpacking new file will have HMAC, we
> must have archiver patched on unpacking too (on dest computer), that
> will count on protected xattrs and provide some fake EVM portable
> signature if we have protected xattrs but don't have EVM xattr itself,
> or we could have security fail here. Probably, this should be controlled
> by archiver flags during unpacking (demand EVM xattr or not).
|
|
From: Patrick O. <pat...@in...> - 2017-01-10 08:52:30
|
On Mon, 2017-01-09 at 18:11 -0500, Mimi Zohar wrote: > On Tue, 2017-01-10 at 00:06 +0300, Mikhail Kurinnoi wrote: > > If EVM is enabled, the only way is patched archiver that will store EVM > > portable signature during file packing instead of HMAC (I am testing > > such patch now for tar with libimaevm). The idea is - prevent HMAC to > > be stored in archive (store EVM portable digsig in this case), since we > > can copy only EVM digsig xattr from archive during unpacking any way. > > The archiver is normally general purpose. Getting EVM or IMA specific > code upstreamed in the archive code will be difficult. I agree; probably it would be better (and fairly easy) to build a custom pack/unpack tool based on libarchive. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. |
|
From: Mikhail K. <vie...@vi...> - 2017-01-10 01:29:51
|
В Mon, 09 Jan 2017 18:11:01 -0500
Mimi Zohar <zo...@li...> пишет:
> On Tue, 2017-01-10 at 00:06 +0300, Mikhail Kurinnoi wrote:
> > В Mon, 09 Jan 2017 08:40:45 -0500
> > Mimi Zohar <zo...@li...> пишет:
> >
> > > On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote:
> > > > version 3 changes:
> > > > 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM
> > > > signature verification failed.
> > >
> > > This can't be right. The boot comnand line option
> > > "ima_appraise=fix" allows IMA to go into a special mode to "fix"
> > > the xattrs.
> > > > Since, __vfs_setxattr_noperm() don't count on
> > > > evm_inode_setxattr(), during file unpacking from atchive EVM
> > > > zero-filled sign could be updated by IMA without EVM signature
> > > > status checking. So, even if you have EVM portable signature
> > > > signed by another key, IMA will update EVM signature to HMAC
> > > > any way, no matter what you have in security.evm xattr or you
> > > > don't have it at all.
> > >
> > > EVM and IMA are two separate modules, which communicate with each
> > > other via the evm_verifyxattr() API. Changes to EVM, should not
> > > affect IMA. If they do, then something is not right.
> >
> > Something is wrong, because if we have FS mounted with iversion
> > flag, we call ima_file_free() -> ima_check_last_writer(), that
> > don't care about evm_verifyxattr() API at all.
>
> This is working as designed. The file integrity verification
> occurred on file open. On __fput() the file hash is updated, which
> causes security.evm to be updated.
I see.
Probably, we could add into evm_ima_xattr_type enumeration
EVM_XATTR_PROTECTED, that we could use as EVM xattr signature type
if we have EVM portable signature verification fail, that will be used
only for "security.evm" attr, as additional check with
evm_protected_xattr() in evm_inode_post_setxattr() and
evm_inode_post_removexattr(). In this way we have INTEGRITY_FAIL in
evm_verify_hmac(), since evm_verify_hmac() have no idea about
EVM_XATTR_PROTECTED signature type, and prevent EVM xattr update on
__fput() if we use IMA xattr hash but not IMA_DIGSIG.
Is it possible?
> > I have this issue without IMA/EVM fix mode. I mean, I faced with
> > this issue on IMA/EVM normal mode during archive unpacking with EVM
> > portable signature signed by wrong cert, that was replaced on EVM
> > zero-filled signature with evm_reset_status(). And after that IMA
> > update inode EVM xattr with proper HMAC by ima_file_free() call.
>
> >
> > > > 2) Added integrity_audit_msg() in order to get some error in
> > > > audit syslog on EVM portable signature verification fail.
> > > >
> > > > version 2 changes:
> > > > 1) evm_verify_hmac() prevent work with EVM portable signature.
> > > > If file signed by EVM portable signature, evm_verify_hmac()
> > > > return INTEGRITY_FAIL;
> > > > 2) evm_inode_post_setxattr() now count on EVM portable
> > > > signature.
> > >
> > > I originally suggested that the portable EVM xattr should be
> > > converted before it is written (in evm_inode_post_setxattr()),
> > > but by the time we get to evm_inode_post_setxattr(), the
> > > portable EVM xattr has already been written. Can the
> > > verification be done earlier in evm_inode_setxattr(), before the
> > > portable EVM xattr is written?
> >
> > We could do everything in evm_inode_setxattr() and return some
> > special error code into vfs_setxattr() that will:
> > 1) don't allow call __vfs_setxattr_noperm();
> > 2) prevent special error code return and return 0 instead in the
> > same time.
>
> I'm wondering if there will be an issue with the order in which the
> security xattrs are installed. For example, what guarantees that
> security.ima is installed before security.evm?
This is archiver's part of work. As I wrote before (and you also could
see this in my patch for tar), archiver must copy EVM xattr the last
one. We have nothing to do with this in kernel.
> Currently, after each xattr is written, the security.evm is updated.
> Do we remove the existing security.evm before verifying the portable
> security.evm?
After each xattr is written, the security.evm is updated, so, we have
proper HMAC all the time, this is exactly what we need. We don't need
remove existing security.evm nor before, nor after if EVM portable
signature verification is ok.
If we move all work with EVM portable signature into
evm_inode_setxattr():
1) If we have EVM portable signature verification is ok - no changes in
EVM xattr are needed. We should only prevent EVM xattr changes by
__vfs_setxattr_noperm() in vfs_setxattr(). EVM code will do everything
what we need. We don't need update EVM xattr or something. Everything
is fine by current EVM/IMA code. This mean, we should have HMAC on
protected xattrs copy and/or file data copy, lets EVM/IMA care about
HMAC.
2) If we have EVM portable signature verification is failed - we should
prevent EVM xattr changes by __vfs_setxattr_noperm() in
vfs_setxattr(), we should replace EVM xattr by some type of EVM
signature that:
a) should not be changed/removed if we not in fix mode;
b) return INTEGRITY_FAIL in evm_verify_hmac().
This is all about my previous proposition about EVM_XATTR_PROTECTED.
> > What I mean:
> >
> > ./security/integrity/evm/evm_main.c
> >
> > int evm_inode_setxattr(struct dentry *dentry, const char
> > *xattr_name, const void *xattr_value, size_t xattr_value_len)
> > {
> > const struct evm_ima_xattr_data *xattr_data = xattr_value;
> >
> > if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) {
> > if (!xattr_value_len)
> > return -EINVAL;
> > if (xattr_data->type != EVM_IMA_XATTR_DIGSIG)
> > return -EPERM;
> > + if (evm_update_portable_digsig(dentry, xattr_name,
> > + xattr_value,
> > + xattr_value_len))
> > + return -ESPECIALERRORCODE;
> > }
> > return evm_protect_xattr(dentry, xattr_name, xattr_value,
> > xattr_value_len);
> > }
> >
> > ./fs/xattr.c
> >
> > int
> > vfs_setxattr(struct dentry *dentry, const char *name, const void
> > *value, size_t size, int flags)
> > {
> >
> > ...
> >
> > out:
> > inode_unlock(inode);
> > + return (error == -ESPECIALERRORCODE) ? 0 : error;
> > }
> >
> > But, what error code we could use in this case?
> >
> >
> > > Stefan Berger, a colleague, asked if the portable EVM xattr is
> > > never written to disk or immediately converted to an HMAC, how
> > > will it be included in an archive?
> >
> > If EVM is enabled, the only way is patched archiver that will store
> > EVM portable signature during file packing instead of HMAC (I am
> > testing such patch now for tar with libimaevm). The idea is -
> > prevent HMAC to be stored in archive (store EVM portable digsig in
> > this case), since we can copy only EVM digsig xattr from archive
> > during unpacking any way.
>
> The archiver is normally general purpose. Getting EVM or IMA specific
> code upstreamed in the archive code will be difficult.
>
> I think adding support for a portable EVM signature is important, but
> there seems to be a lot of details that need to be resolved first.
> We've touched on a few of them in this thread.
Yes, but we could start from kernel, right?
As you can see, archivers (at least tar) care about SELinux security
context and this is not a general purpose. I believe, after some
time we will have EVM portable version support in some archivers.
We have a lot of details, but almost all of them connected to
archiver's work, but not kernel itself.
--
Best regards,
Mikhail Kurinnoi
|
|
From: Mimi Z. <zo...@li...> - 2017-01-10 02:38:56
|
On Tue, 2017-01-10 at 04:29 +0300, Mikhail Kurinnoi wrote:
> В Mon, 09 Jan 2017 18:11:01 -0500
> Mimi Zohar <zo...@li...> пишет:
>
> > On Tue, 2017-01-10 at 00:06 +0300, Mikhail Kurinnoi wrote:
> > > В Mon, 09 Jan 2017 08:40:45 -0500
> > > Mimi Zohar <zo...@li...> пишет:
> > >
> > > > On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote:
> > > > > version 3 changes:
> > > > > 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM
> > > > > signature verification failed.
> > > >
> > > > This can't be right. The boot comnand line option
> > > > "ima_appraise=fix" allows IMA to go into a special mode to "fix"
> > > > the xattrs.
> > > > > Since, __vfs_setxattr_noperm() don't count on
> > > > > evm_inode_setxattr(), during file unpacking from atchive EVM
> > > > > zero-filled sign could be updated by IMA without EVM signature
> > > > > status checking. So, even if you have EVM portable signature
> > > > > signed by another key, IMA will update EVM signature to HMAC
> > > > > any way, no matter what you have in security.evm xattr or you
> > > > > don't have it at all.
> > > >
> > > > EVM and IMA are two separate modules, which communicate with each
> > > > other via the evm_verifyxattr() API. Changes to EVM, should not
> > > > affect IMA. If they do, then something is not right.
> > >
> > > Something is wrong, because if we have FS mounted with iversion
> > > flag, we call ima_file_free() -> ima_check_last_writer(), that
> > > don't care about evm_verifyxattr() API at all.
> >
> > This is working as designed. The file integrity verification
> > occurred on file open. On __fput() the file hash is updated, which
> > causes security.evm to be updated.
>
> I see.
>
> Probably, we could add into evm_ima_xattr_type enumeration
> EVM_XATTR_PROTECTED, that we could use as EVM xattr signature type
> if we have EVM portable signature verification fail, that will be used
> only for "security.evm" attr, as additional check with
> evm_protected_xattr() in evm_inode_post_setxattr() and
> evm_inode_post_removexattr(). In this way we have INTEGRITY_FAIL in
> evm_verify_hmac(), since evm_verify_hmac() have no idea about
> EVM_XATTR_PROTECTED signature type, and prevent EVM xattr update on
> __fput() if we use IMA xattr hash but not IMA_DIGSIG.
>
> Is it possible?
>
>
> > > I have this issue without IMA/EVM fix mode. I mean, I faced with
> > > this issue on IMA/EVM normal mode during archive unpacking with EVM
> > > portable signature signed by wrong cert, that was replaced on EVM
> > > zero-filled signature with evm_reset_status(). And after that IMA
> > > update inode EVM xattr with proper HMAC by ima_file_free() call.
> >
> > >
> > > > > 2) Added integrity_audit_msg() in order to get some error in
> > > > > audit syslog on EVM portable signature verification fail.
> > > > >
> > > > > version 2 changes:
> > > > > 1) evm_verify_hmac() prevent work with EVM portable signature.
> > > > > If file signed by EVM portable signature, evm_verify_hmac()
> > > > > return INTEGRITY_FAIL;
> > > > > 2) evm_inode_post_setxattr() now count on EVM portable
> > > > > signature.
> > > >
> > > > I originally suggested that the portable EVM xattr should be
> > > > converted before it is written (in evm_inode_post_setxattr()),
> > > > but by the time we get to evm_inode_post_setxattr(), the
> > > > portable EVM xattr has already been written. Can the
> > > > verification be done earlier in evm_inode_setxattr(), before the
> > > > portable EVM xattr is written?
> > >
> > > We could do everything in evm_inode_setxattr() and return some
> > > special error code into vfs_setxattr() that will:
> > > 1) don't allow call __vfs_setxattr_noperm();
> > > 2) prevent special error code return and return 0 instead in the
> > > same time.
> >
> > I'm wondering if there will be an issue with the order in which the
> > security xattrs are installed. For example, what guarantees that
> > security.ima is installed before security.evm?
>
> This is archiver's part of work. As I wrote before (and you also could
> see this in my patch for tar), archiver must copy EVM xattr the last
> one. We have nothing to do with this in kernel.
>
>
> > Currently, after each xattr is written, the security.evm is updated.
> > Do we remove the existing security.evm before verifying the portable
> > security.evm?
>
> After each xattr is written, the security.evm is updated, so, we have
> proper HMAC all the time, this is exactly what we need. We don't need
> remove existing security.evm nor before, nor after if EVM portable
> signature verification is ok.
>
> If we move all work with EVM portable signature into
> evm_inode_setxattr():
>
> 1) If we have EVM portable signature verification is ok - no changes in
> EVM xattr are needed. We should only prevent EVM xattr changes by
> __vfs_setxattr_noperm() in vfs_setxattr(). EVM code will do everything
> what we need. We don't need update EVM xattr or something. Everything
> is fine by current EVM/IMA code. This mean, we should have HMAC on
> protected xattrs copy and/or file data copy, lets EVM/IMA care about
> HMAC.
Failing the evm_inode_setxattr() will prevent the portable xattr from
being written.
> 2) If we have EVM portable signature verification is failed - we should
> prevent EVM xattr changes by __vfs_setxattr_noperm() in
> vfs_setxattr(), we should replace EVM xattr by some type of EVM
> signature that:
> a) should not be changed/removed if we not in fix mode;
> b) return INTEGRITY_FAIL in evm_verify_hmac().
> This is all about my previous proposition about EVM_XATTR_PROTECTED.
Unlike IMA, where file signatures are considered immutable, EVM converts
signatures to HMACs. We probably don't need a new signature format,
just a new type which isn't converted to an HMAC.
For failed portable signatures, we would write security.evm as this new
type, but with an invalid payload.
> > > What I mean:
> > >
> > > ./security/integrity/evm/evm_main.c
> > >
> > > int evm_inode_setxattr(struct dentry *dentry, const char
> > > *xattr_name, const void *xattr_value, size_t xattr_value_len)
> > > {
> > > const struct evm_ima_xattr_data *xattr_data = xattr_value;
> > >
> > > if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) {
> > > if (!xattr_value_len)
> > > return -EINVAL;
> > > if (xattr_data->type != EVM_IMA_XATTR_DIGSIG)
> > > return -EPERM;
> > > + if (evm_update_portable_digsig(dentry, xattr_name,
> > > + xattr_value,
> > > + xattr_value_len))
> > > + return -ESPECIALERRORCODE;
> > > }
> > > return evm_protect_xattr(dentry, xattr_name, xattr_value,
> > > xattr_value_len);
> > > }
> > >
> > > ./fs/xattr.c
> > >
> > > int
> > > vfs_setxattr(struct dentry *dentry, const char *name, const void
> > > *value, size_t size, int flags)
> > > {
> > >
> > > ...
> > >
> > > out:
> > > inode_unlock(inode);
> > > + return (error == -ESPECIALERRORCODE) ? 0 : error;
> > > }
> > >
> > > But, what error code we could use in this case?
> > >
> > >
> > > > Stefan Berger, a colleague, asked if the portable EVM xattr is
> > > > never written to disk or immediately converted to an HMAC, how
> > > > will it be included in an archive?
> > >
> > > If EVM is enabled, the only way is patched archiver that will store
> > > EVM portable signature during file packing instead of HMAC (I am
> > > testing such patch now for tar with libimaevm). The idea is -
> > > prevent HMAC to be stored in archive (store EVM portable digsig in
> > > this case), since we can copy only EVM digsig xattr from archive
> > > during unpacking any way.
> >
> > The archiver is normally general purpose. Getting EVM or IMA specific
> > code upstreamed in the archive code will be difficult.
> >
> > I think adding support for a portable EVM signature is important, but
> > there seems to be a lot of details that need to be resolved first.
> > We've touched on a few of them in this thread.
>
> Yes, but we could start from kernel, right?
Yes
> As you can see, archivers (at least tar) care about SELinux security
> context and this is not a general purpose. I believe, after some
> time we will have EVM portable version support in some archivers.
> We have a lot of details, but almost all of them connected to
> archiver's work, but not kernel itself.
There's a long history of GNU not accepting any IMA patches. Doubtful,
but perhaps they'll differentiate between EVM and IMA, and be willing to
accept EVM patches.
Mimi
|
|
From: Mikhail K. <vie...@vi...> - 2017-01-10 04:26:07
|
В Mon, 09 Jan 2017 21:38:41 -0500
Mimi Zohar <zo...@li...> пишет:
> On Tue, 2017-01-10 at 04:29 +0300, Mikhail Kurinnoi wrote:
> > В Mon, 09 Jan 2017 18:11:01 -0500
> > Mimi Zohar <zo...@li...> пишет:
> >
> > > On Tue, 2017-01-10 at 00:06 +0300, Mikhail Kurinnoi wrote:
> > > > В Mon, 09 Jan 2017 08:40:45 -0500
> > > > Mimi Zohar <zo...@li...> пишет:
> > > >
> > > > > On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote:
> > > > > > version 3 changes:
> > > > > > 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if
> > > > > > EVM signature verification failed.
> > > > >
> > > > > This can't be right. The boot comnand line option
> > > > > "ima_appraise=fix" allows IMA to go into a special mode to
> > > > > "fix" the xattrs.
> > > > > > Since, __vfs_setxattr_noperm() don't count on
> > > > > > evm_inode_setxattr(), during file unpacking from atchive EVM
> > > > > > zero-filled sign could be updated by IMA without EVM
> > > > > > signature status checking. So, even if you have EVM
> > > > > > portable signature signed by another key, IMA will update
> > > > > > EVM signature to HMAC any way, no matter what you have in
> > > > > > security.evm xattr or you don't have it at all.
> > > > >
> > > > > EVM and IMA are two separate modules, which communicate with
> > > > > each other via the evm_verifyxattr() API. Changes to EVM,
> > > > > should not affect IMA. If they do, then something is not
> > > > > right.
> > > >
> > > > Something is wrong, because if we have FS mounted with iversion
> > > > flag, we call ima_file_free() -> ima_check_last_writer(), that
> > > > don't care about evm_verifyxattr() API at all.
> > >
> > > This is working as designed. The file integrity verification
> > > occurred on file open. On __fput() the file hash is updated,
> > > which causes security.evm to be updated.
> >
> > I see.
> >
> > Probably, we could add into evm_ima_xattr_type enumeration
> > EVM_XATTR_PROTECTED, that we could use as EVM xattr signature type
> > if we have EVM portable signature verification fail, that will be
> > used only for "security.evm" attr, as additional check with
> > evm_protected_xattr() in evm_inode_post_setxattr() and
> > evm_inode_post_removexattr(). In this way we have INTEGRITY_FAIL in
> > evm_verify_hmac(), since evm_verify_hmac() have no idea about
> > EVM_XATTR_PROTECTED signature type, and prevent EVM xattr update on
> > __fput() if we use IMA xattr hash but not IMA_DIGSIG.
> >
> > Is it possible?
> >
> >
> > > > I have this issue without IMA/EVM fix mode. I mean, I faced with
> > > > this issue on IMA/EVM normal mode during archive unpacking with
> > > > EVM portable signature signed by wrong cert, that was replaced
> > > > on EVM zero-filled signature with evm_reset_status(). And after
> > > > that IMA update inode EVM xattr with proper HMAC by
> > > > ima_file_free() call.
> > >
> > > >
> > > > > > 2) Added integrity_audit_msg() in order to get some error in
> > > > > > audit syslog on EVM portable signature verification fail.
> > > > > >
> > > > > > version 2 changes:
> > > > > > 1) evm_verify_hmac() prevent work with EVM portable
> > > > > > signature. If file signed by EVM portable signature,
> > > > > > evm_verify_hmac() return INTEGRITY_FAIL;
> > > > > > 2) evm_inode_post_setxattr() now count on EVM portable
> > > > > > signature.
> > > > >
> > > > > I originally suggested that the portable EVM xattr should be
> > > > > converted before it is written (in evm_inode_post_setxattr()),
> > > > > but by the time we get to evm_inode_post_setxattr(), the
> > > > > portable EVM xattr has already been written. Can the
> > > > > verification be done earlier in evm_inode_setxattr(), before
> > > > > the portable EVM xattr is written?
> > > >
> > > > We could do everything in evm_inode_setxattr() and return some
> > > > special error code into vfs_setxattr() that will:
> > > > 1) don't allow call __vfs_setxattr_noperm();
> > > > 2) prevent special error code return and return 0 instead in the
> > > > same time.
> > >
> > > I'm wondering if there will be an issue with the order in which
> > > the security xattrs are installed. For example, what guarantees
> > > that security.ima is installed before security.evm?
> >
> > This is archiver's part of work. As I wrote before (and you also
> > could see this in my patch for tar), archiver must copy EVM xattr
> > the last one. We have nothing to do with this in kernel.
> >
> >
> > > Currently, after each xattr is written, the security.evm is
> > > updated. Do we remove the existing security.evm before verifying
> > > the portable security.evm?
> >
> > After each xattr is written, the security.evm is updated, so, we
> > have proper HMAC all the time, this is exactly what we need. We
> > don't need remove existing security.evm nor before, nor after if
> > EVM portable signature verification is ok.
> >
> > If we move all work with EVM portable signature into
> > evm_inode_setxattr():
> >
> > 1) If we have EVM portable signature verification is ok - no
> > changes in EVM xattr are needed. We should only prevent EVM xattr
> > changes by __vfs_setxattr_noperm() in vfs_setxattr(). EVM code will
> > do everything what we need. We don't need update EVM xattr or
> > something. Everything is fine by current EVM/IMA code. This mean,
> > we should have HMAC on protected xattrs copy and/or file data copy,
> > lets EVM/IMA care about HMAC.
>
> Failing the evm_inode_setxattr() will prevent the portable xattr from
> being written.
Exactly my point. We could use some error code returned by
evm_inode_setxattr() in order to prevent the portable xattr from being
written, but return 0 from vfs_setxattr() instead of error code what we
are using in the evm_inode_setxattr(), since we should return "all
is ok" if EVM portable signature verification is ok and we stay
with HMAC.
> > 2) If we have EVM portable signature verification is failed - we
> > should prevent EVM xattr changes by __vfs_setxattr_noperm() in
> > vfs_setxattr(), we should replace EVM xattr by some type of EVM
> > signature that:
>
> > a) should not be changed/removed if we not in fix mode;
> > b) return INTEGRITY_FAIL in evm_verify_hmac().
> > This is all about my previous proposition about
> > EVM_XATTR_PROTECTED.
>
> Unlike IMA, where file signatures are considered immutable, EVM
> converts signatures to HMACs. We probably don't need a new signature
> format, just a new type which isn't converted to an HMAC.
>
> For failed portable signatures, we would write security.evm as this
> new type, but with an invalid payload.
Exactly. We don't need any new signature format for EVM. We don't need
immutable EVM xattr or something like that for sure.
We just need new type that we could use for failed portable signatures
that will not be converted to an HMAC. And sure, it must have invalid
payload, I even propose stay with zero-filled xattr (what I am using
now in patch), since we don't really need anything else for our
purposes.
> > > > What I mean:
> > > >
> > > > ./security/integrity/evm/evm_main.c
> > > >
> > > > int evm_inode_setxattr(struct dentry *dentry, const char
> > > > *xattr_name, const void *xattr_value, size_t xattr_value_len)
> > > > {
> > > > const struct evm_ima_xattr_data *xattr_data =
> > > > xattr_value;
> > > >
> > > > if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) {
> > > > if (!xattr_value_len)
> > > > return -EINVAL;
> > > > if (xattr_data->type != EVM_IMA_XATTR_DIGSIG)
> > > > return -EPERM;
> > > > + if (evm_update_portable_digsig(dentry,
> > > > xattr_name,
> > > > + xattr_value,
> > > > +
> > > > xattr_value_len))
> > > > + return -ESPECIALERRORCODE;
> > > > }
> > > > return evm_protect_xattr(dentry, xattr_name,
> > > > xattr_value, xattr_value_len);
> > > > }
> > > >
> > > > ./fs/xattr.c
> > > >
> > > > int
> > > > vfs_setxattr(struct dentry *dentry, const char *name, const void
> > > > *value, size_t size, int flags)
> > > > {
> > > >
> > > > ...
> > > >
> > > > out:
> > > > inode_unlock(inode);
> > > > + return (error == -ESPECIALERRORCODE) ? 0 : error;
> > > > }
> > > >
> > > > But, what error code we could use in this case?
> > > >
> > > >
> > > > > Stefan Berger, a colleague, asked if the portable EVM xattr is
> > > > > never written to disk or immediately converted to an HMAC, how
> > > > > will it be included in an archive?
> > > >
> > > > If EVM is enabled, the only way is patched archiver that will
> > > > store EVM portable signature during file packing instead of
> > > > HMAC (I am testing such patch now for tar with libimaevm). The
> > > > idea is - prevent HMAC to be stored in archive (store EVM
> > > > portable digsig in this case), since we can copy only EVM
> > > > digsig xattr from archive during unpacking any way.
> > >
> > > The archiver is normally general purpose. Getting EVM or IMA
> > > specific code upstreamed in the archive code will be difficult.
> > >
> > > I think adding support for a portable EVM signature is important,
> > > but there seems to be a lot of details that need to be resolved
> > > first. We've touched on a few of them in this thread.
> >
> > Yes, but we could start from kernel, right?
>
> Yes
>
> > As you can see, archivers (at least tar) care about SELinux security
> > context and this is not a general purpose. I believe, after some
> > time we will have EVM portable version support in some archivers.
> > We have a lot of details, but almost all of them connected to
> > archiver's work, but not kernel itself.
>
> There's a long history of GNU not accepting any IMA patches.
> Doubtful, but perhaps they'll differentiate between EVM and IMA, and
> be willing to accept EVM patches.
>
> Mimi
>
--
Best regards,
Mikhail Kurinnoi
|
|
From: Mimi Z. <zo...@li...> - 2017-01-10 11:33:44
|
On Tue, 2017-01-10 at 07:25 +0300, Mikhail Kurinnoi wrote: > В Mon, 09 Jan 2017 21:38:41 -0500 > Mimi Zohar <zo...@li...> пишет: > > > > > Currently, after each xattr is written, the security.evm is > > > > updated. Do we remove the existing security.evm before verifying > > > > the portable security.evm? > > > > > > After each xattr is written, the security.evm is updated, so, we > > > have proper HMAC all the time, this is exactly what we need. We > > > don't need remove existing security.evm nor before, nor after if > > > EVM portable signature verification is ok. > > > > > > If we move all work with EVM portable signature into > > > evm_inode_setxattr(): > > > > > > 1) If we have EVM portable signature verification is ok - no > > > changes in EVM xattr are needed. We should only prevent EVM xattr > > > changes by __vfs_setxattr_noperm() in vfs_setxattr(). EVM code will > > > do everything what we need. We don't need update EVM xattr or > > > something. Everything is fine by current EVM/IMA code. This mean, > > > we should have HMAC on protected xattrs copy and/or file data copy, > > > lets EVM/IMA care about HMAC. > > > > Failing the evm_inode_setxattr() will prevent the portable xattr from > > being written. > > Exactly my point. We could use some error code returned by > evm_inode_setxattr() in order to prevent the portable xattr from being > written, but return 0 from vfs_setxattr() instead of error code what we > are using in the evm_inode_setxattr(), since we should return "all > is ok" if EVM portable signature verification is ok and we stay > with HMAC. Agreed this works. The LSM hooks expect 0 on success and non zero on failure. Here we're inverting that. Please introduce this concept of a valid, non-zero return code from evm_inode_setxattr() in security_inode_setxattr() as a separate patch. > > > 2) If we have EVM portable signature verification is failed - we > > > should prevent EVM xattr changes by __vfs_setxattr_noperm() in > > > vfs_setxattr(), we should replace EVM xattr by some type of EVM > > > signature that: > > > > > a) should not be changed/removed if we not in fix mode; > > > b) return INTEGRITY_FAIL in evm_verify_hmac(). > > > This is all about my previous proposition about > > > EVM_XATTR_PROTECTED. > > > > Unlike IMA, where file signatures are considered immutable, EVM > > converts signatures to HMACs. We probably don't need a new signature > > format, just a new type which isn't converted to an HMAC. > > > > For failed portable signatures, we would write security.evm as this > > new type, but with an invalid payload. > > Exactly. We don't need any new signature format for EVM. We don't need > immutable EVM xattr or something like that for sure. "immutable" in this context just means that the security.evm xattr can not be replaced with an HMAC. > We just need new type that we could use for failed portable signatures > that will not be converted to an HMAC. And sure, it must have invalid > payload, I even propose stay with zero-filled xattr (what I am using > now in patch), since we don't really need anything else for our > purposes. Others have requested an immutable file signature type. The only difference between this signature type and the original one, is that this type is never replaced with an HMAC. We've defined the "portable" signature to not include certain fields (eg. i_version, i_ino). Instead of removing them, would it make sense to zero out these fields. Then converting a "portable" signature to an "immutable" one is a matter of changing just the file signature type. With these fields zeroed out, the normal file signature verification will fail. Assuming you agree with an "immutable" file signature type, the first patch in this series should introduce the concept. Mimi |
|
From: Mimi Z. <zo...@li...> - 2017-01-10 12:10:20
|
On Tue, 2017-01-10 at 06:33 -0500, Mimi Zohar wrote: > On Tue, 2017-01-10 at 07:25 +0300, Mikhail Kurinnoi wrote: > > В Mon, 09 Jan 2017 21:38:41 -0500 > > Mimi Zohar <zo...@li...> пишет: > > > > > > > Currently, after each xattr is written, the security.evm is > > > > > updated. Do we remove the existing security.evm before verifying > > > > > the portable security.evm? > > > > > > > > After each xattr is written, the security.evm is updated, so, we > > > > have proper HMAC all the time, this is exactly what we need. We > > > > don't need remove existing security.evm nor before, nor after if > > > > EVM portable signature verification is ok. > > > > > > > > If we move all work with EVM portable signature into > > > > evm_inode_setxattr(): > > > > > > > > 1) If we have EVM portable signature verification is ok - no > > > > changes in EVM xattr are needed. We should only prevent EVM xattr > > > > changes by __vfs_setxattr_noperm() in vfs_setxattr(). EVM code will > > > > do everything what we need. We don't need update EVM xattr or > > > > something. Everything is fine by current EVM/IMA code. This mean, > > > > we should have HMAC on protected xattrs copy and/or file data copy, > > > > lets EVM/IMA care about HMAC. > > > > > > Failing the evm_inode_setxattr() will prevent the portable xattr from > > > being written. > > > > Exactly my point. We could use some error code returned by > > evm_inode_setxattr() in order to prevent the portable xattr from being > > written, but return 0 from vfs_setxattr() instead of error code what we > > are using in the evm_inode_setxattr(), since we should return "all > > is ok" if EVM portable signature verification is ok and we stay > > with HMAC. > > Agreed this works. The LSM hooks expect 0 on success and non zero on > failure. Here we're inverting that. Please introduce this concept of > a valid, non-zero return code from evm_inode_setxattr() in > security_inode_setxattr() as a separate patch. > > > > > 2) If we have EVM portable signature verification is failed - we > > > > should prevent EVM xattr changes by __vfs_setxattr_noperm() in > > > > vfs_setxattr(), we should replace EVM xattr by some type of EVM > > > > signature that: > > > > > > > a) should not be changed/removed if we not in fix mode; > > > > b) return INTEGRITY_FAIL in evm_verify_hmac(). > > > > This is all about my previous proposition about > > > > EVM_XATTR_PROTECTED. > > > > > > Unlike IMA, where file signatures are considered immutable, EVM > > > converts signatures to HMACs. We probably don't need a new signature > > > format, just a new type which isn't converted to an HMAC. > > > > > > For failed portable signatures, we would write security.evm as this > > > new type, but with an invalid payload. > > > > Exactly. We don't need any new signature format for EVM. We don't need > > immutable EVM xattr or something like that for sure. > > "immutable" in this context just means that the security.evm xattr can > not be replaced with an HMAC. > > > We just need new type that we could use for failed portable signatures > > that will not be converted to an HMAC. And sure, it must have invalid > > payload, I even propose stay with zero-filled xattr (what I am using > > now in patch), since we don't really need anything else for our > > purposes. > > Others have requested an immutable file signature type. The only > difference between this signature type and the original one, is that > this type is never replaced with an HMAC. > > We've defined the "portable" signature to not include certain fields > (eg. i_version, i_ino). Instead of removing them, would it make sense > to zero out these fields. Then converting a "portable" signature to an > "immutable" one is a matter of changing just the file signature type. > With these fields zeroed out, the normal file signature verification > will fail. That should have been the normal EVM file meta-data signature verification will fail. > Assuming you agree with an "immutable" file signature type, the first > patch in this series should introduce the concept. > > Mimi |
|
From: Mikhail K. <vie...@vi...> - 2017-01-11 00:59:32
|
В Tue, 10 Jan 2017 06:33:29 -0500 Mimi Zohar <zo...@li...> пишет: > On Tue, 2017-01-10 at 07:25 +0300, Mikhail Kurinnoi wrote: > > В Mon, 09 Jan 2017 21:38:41 -0500 > > Mimi Zohar <zo...@li...> пишет: > > > > > > > Currently, after each xattr is written, the security.evm is > > > > > updated. Do we remove the existing security.evm before > > > > > verifying the portable security.evm? > > > > > > > > After each xattr is written, the security.evm is updated, so, we > > > > have proper HMAC all the time, this is exactly what we need. We > > > > don't need remove existing security.evm nor before, nor after if > > > > EVM portable signature verification is ok. > > > > > > > > If we move all work with EVM portable signature into > > > > evm_inode_setxattr(): > > > > > > > > 1) If we have EVM portable signature verification is ok - no > > > > changes in EVM xattr are needed. We should only prevent EVM > > > > xattr changes by __vfs_setxattr_noperm() in vfs_setxattr(). EVM > > > > code will do everything what we need. We don't need update EVM > > > > xattr or something. Everything is fine by current EVM/IMA code. > > > > This mean, we should have HMAC on protected xattrs copy and/or > > > > file data copy, lets EVM/IMA care about HMAC. > > > > > > Failing the evm_inode_setxattr() will prevent the portable xattr > > > from being written. > > > > Exactly my point. We could use some error code returned by > > evm_inode_setxattr() in order to prevent the portable xattr from > > being written, but return 0 from vfs_setxattr() instead of error > > code what we are using in the evm_inode_setxattr(), since we should > > return "all is ok" if EVM portable signature verification is ok and > > we stay with HMAC. > > Agreed this works. The LSM hooks expect 0 on success and non zero on > failure. Here we're inverting that. Please introduce this concept > of a valid, non-zero return code from evm_inode_setxattr() in > security_inode_setxattr() as a separate patch. Ok. > > > > 2) If we have EVM portable signature verification is failed - we > > > > should prevent EVM xattr changes by __vfs_setxattr_noperm() in > > > > vfs_setxattr(), we should replace EVM xattr by some type of EVM > > > > signature that: > > > > > > > a) should not be changed/removed if we not in fix mode; > > > > b) return INTEGRITY_FAIL in evm_verify_hmac(). > > > > This is all about my previous proposition about > > > > EVM_XATTR_PROTECTED. > > > > > > Unlike IMA, where file signatures are considered immutable, EVM > > > converts signatures to HMACs. We probably don't need a new > > > signature format, just a new type which isn't converted to an > > > HMAC. > > > > > > For failed portable signatures, we would write security.evm as > > > this new type, but with an invalid payload. > > > > Exactly. We don't need any new signature format for EVM. We don't > > need immutable EVM xattr or something like that for sure. > > "immutable" in this context just means that the security.evm xattr can > not be replaced with an HMAC. > > > We just need new type that we could use for failed portable > > signatures that will not be converted to an HMAC. And sure, it must > > have invalid payload, I even propose stay with zero-filled xattr > > (what I am using now in patch), since we don't really need anything > > else for our purposes. > > Others have requested an immutable file signature type. The only > difference between this signature type and the original one, is that > this type is never replaced with an HMAC. I think, we could use feature we already have. For now EVM_IMA_XATTR_DIGSIG in EVM have "versions", this parameter stored in xattr_data[1] (xattr_data[0] - signature type). We have version 2 for standard, version 3 used for portable (since we already use it in evm-ima-util code), version 4 could be "immutable". In this way, we could sign file with: 1) standard digsig (version 2, that will be HMACed on first access); 2) "portable" digsig (version 3, that could be stored depending of verification status as HMAC or "immutable" signature with invalid payload); 3) "immutable" digsig (version 4, same as version 2, but never replaced with HMAC). > We've defined the "portable" signature to not include certain fields > (eg. i_version, i_ino). Instead of removing them, would it make > sense to zero out these fields. Then converting a "portable" > signature to an "immutable" one is a matter of changing just the file > signature type. With these fields zeroed out, the normal file > signature verification will fail. I think, no reason store previous failed "portable" signature into xattr at all. Sure, it will have verification fail, but, why we need store 100+ bytes into inode xattr with useless digsig if we need only 2 bytes? Don't forget, this is an exception, we need invalid payload and even 1 byte will be enough in this case. We never should check this xattr data, all what we check - xattr type, we will fail signature verification right after xattr type check since we have xattr_len <= 2 (for example), we don't even need call evm_calc_hash() in this case before integrity_digsig_verify() return fail. Also, no reason do any offline modification with signature type at all, since we don't have data stored in xattr. > Assuming you agree with an "immutable" file signature type, the first > patch in this series should introduce the concept. Agreed, we need introduce "immutable" file signature type first, since we need it for portable signature work. -- Best regards, Mikhail Kurinnoi |
|
From: Mimi Z. <zo...@li...> - 2017-01-11 01:16:21
|
On Wed, 2017-01-11 at 03:59 +0300, Mikhail Kurinnoi wrote: > В Tue, 10 Jan 2017 06:33:29 -0500 > Mimi Zohar <zo...@li...> пишет: > > > On Tue, 2017-01-10 at 07:25 +0300, Mikhail Kurinnoi wrote: > > > В Mon, 09 Jan 2017 21:38:41 -0500 > > > Mimi Zohar <zo...@li...> пишет: > > > > > > > > > Currently, after each xattr is written, the security.evm is > > > > > > updated. Do we remove the existing security.evm before > > > > > > verifying the portable security.evm? > > > > > > > > > > After each xattr is written, the security.evm is updated, so, we > > > > > have proper HMAC all the time, this is exactly what we need. We > > > > > don't need remove existing security.evm nor before, nor after if > > > > > EVM portable signature verification is ok. > > > > > > > > > > If we move all work with EVM portable signature into > > > > > evm_inode_setxattr(): > > > > > > > > > > 1) If we have EVM portable signature verification is ok - no > > > > > changes in EVM xattr are needed. We should only prevent EVM > > > > > xattr changes by __vfs_setxattr_noperm() in vfs_setxattr(). EVM > > > > > code will do everything what we need. We don't need update EVM > > > > > xattr or something. Everything is fine by current EVM/IMA code. > > > > > This mean, we should have HMAC on protected xattrs copy and/or > > > > > file data copy, lets EVM/IMA care about HMAC. > > > > > > > > Failing the evm_inode_setxattr() will prevent the portable xattr > > > > from being written. > > > > > > Exactly my point. We could use some error code returned by > > > evm_inode_setxattr() in order to prevent the portable xattr from > > > being written, but return 0 from vfs_setxattr() instead of error > > > code what we are using in the evm_inode_setxattr(), since we should > > > return "all is ok" if EVM portable signature verification is ok and > > > we stay with HMAC. > > > > Agreed this works. The LSM hooks expect 0 on success and non zero on > > failure. Here we're inverting that. Please introduce this concept > > of a valid, non-zero return code from evm_inode_setxattr() in > > security_inode_setxattr() as a separate patch. > > Ok. > > > > > > 2) If we have EVM portable signature verification is failed - we > > > > > should prevent EVM xattr changes by __vfs_setxattr_noperm() in > > > > > vfs_setxattr(), we should replace EVM xattr by some type of EVM > > > > > signature that: > > > > > > > > > a) should not be changed/removed if we not in fix mode; > > > > > b) return INTEGRITY_FAIL in evm_verify_hmac(). > > > > > This is all about my previous proposition about > > > > > EVM_XATTR_PROTECTED. > > > > > > > > Unlike IMA, where file signatures are considered immutable, EVM > > > > converts signatures to HMACs. We probably don't need a new > > > > signature format, just a new type which isn't converted to an > > > > HMAC. > > > > > > > > For failed portable signatures, we would write security.evm as > > > > this new type, but with an invalid payload. > > > > > > Exactly. We don't need any new signature format for EVM. We don't > > > need immutable EVM xattr or something like that for sure. > > > > "immutable" in this context just means that the security.evm xattr can > > not be replaced with an HMAC. > > > > > We just need new type that we could use for failed portable > > > signatures that will not be converted to an HMAC. And sure, it must > > > have invalid payload, I even propose stay with zero-filled xattr > > > (what I am using now in patch), since we don't really need anything > > > else for our purposes. > > > > Others have requested an immutable file signature type. The only > > difference between this signature type and the original one, is that > > this type is never replaced with an HMAC. > > I think, we could use feature we already have. For now > EVM_IMA_XATTR_DIGSIG in EVM have "versions", this parameter stored > in xattr_data[1] (xattr_data[0] - signature type). > We have version 2 for standard, version 3 used for portable (since we > already use it in evm-ima-util code), version 4 could be "immutable". > In this way, we could sign file with: > 1) standard digsig (version 2, that will be HMACed on first access); > 2) "portable" digsig (version 3, that could be stored depending of > verification status as HMAC or "immutable" signature with invalid > payload); > 3) "immutable" digsig (version 4, same as version 2, but never replaced > with HMAC). Right > > We've defined the "portable" signature to not include certain fields > > (eg. i_version, i_ino). Instead of removing them, would it make > > sense to zero out these fields. Then converting a "portable" > > signature to an "immutable" one is a matter of changing just the file > > signature type. With these fields zeroed out, the normal file > > signature verification will fail. > > I think, no reason store previous failed "portable" signature into xattr > at all. Sure, it will have verification fail, but, why we need store > 100+ bytes into inode xattr with useless digsig if we need only 2 bytes? > Don't forget, this is an exception, we need invalid payload and even 1 > byte will be enough in this case. We never should check this xattr > data, all what we check - xattr type, we will fail signature > verification right after xattr type check since we have xattr_len <= 2 > (for example), we don't even need call evm_calc_hash() in this case > before integrity_digsig_verify() return fail. Also, no reason do any > offline modification with signature type at all, since we don't have > data stored in xattr. Ok Mimi > > Assuming you agree with an "immutable" file signature type, the first > > patch in this series should introduce the concept. > > Agreed, we need introduce "immutable" file signature type first, since > we need it for portable signature work. |
|
From: Stefan B. <st...@li...> - 2017-01-11 20:15:29
|
On 01/09/2017 08:29 PM, Mikhail Kurinnoi wrote:
> В Mon, 09 Jan 2017 18:11:01 -0500
> Mimi Zohar <zo...@li...> пишет:
>
>> On Tue, 2017-01-10 at 00:06 +0300, Mikhail Kurinnoi wrote:
>>> В Mon, 09 Jan 2017 08:40:45 -0500
>>> Mimi Zohar <zo...@li...> пишет:
>>>
>>>> On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote:
>>>>> version 3 changes:
>>>>> 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM
>>>>> signature verification failed.
>>>> This can't be right. The boot comnand line option
>>>> "ima_appraise=fix" allows IMA to go into a special mode to "fix"
>>>> the xattrs.
>>>>> Since, __vfs_setxattr_noperm() don't count on
>>>>> evm_inode_setxattr(), during file unpacking from atchive EVM
>>>>> zero-filled sign could be updated by IMA without EVM signature
>>>>> status checking. So, even if you have EVM portable signature
>>>>> signed by another key, IMA will update EVM signature to HMAC
>>>>> any way, no matter what you have in security.evm xattr or you
>>>>> don't have it at all.
>>>> EVM and IMA are two separate modules, which communicate with each
>>>> other via the evm_verifyxattr() API. Changes to EVM, should not
>>>> affect IMA. If they do, then something is not right.
>>> Something is wrong, because if we have FS mounted with iversion
>>> flag, we call ima_file_free() -> ima_check_last_writer(), that
>>> don't care about evm_verifyxattr() API at all.
>> This is working as designed. The file integrity verification
>> occurred on file open. On __fput() the file hash is updated, which
>> causes security.evm to be updated.
> I see.
>
> Probably, we could add into evm_ima_xattr_type enumeration
> EVM_XATTR_PROTECTED, that we could use as EVM xattr signature type
> if we have EVM portable signature verification fail, that will be used
> only for "security.evm" attr, as additional check with
> evm_protected_xattr() in evm_inode_post_setxattr() and
> evm_inode_post_removexattr(). In this way we have INTEGRITY_FAIL in
> evm_verify_hmac(), since evm_verify_hmac() have no idea about
> EVM_XATTR_PROTECTED signature type, and prevent EVM xattr update on
> __fput() if we use IMA xattr hash but not IMA_DIGSIG.
>
> Is it possible?
>
>
>>> I have this issue without IMA/EVM fix mode. I mean, I faced with
>>> this issue on IMA/EVM normal mode during archive unpacking with EVM
>>> portable signature signed by wrong cert, that was replaced on EVM
>>> zero-filled signature with evm_reset_status(). And after that IMA
>>> update inode EVM xattr with proper HMAC by ima_file_free() call.
>>>
>>>>> 2) Added integrity_audit_msg() in order to get some error in
>>>>> audit syslog on EVM portable signature verification fail.
>>>>>
>>>>> version 2 changes:
>>>>> 1) evm_verify_hmac() prevent work with EVM portable signature.
>>>>> If file signed by EVM portable signature, evm_verify_hmac()
>>>>> return INTEGRITY_FAIL;
>>>>> 2) evm_inode_post_setxattr() now count on EVM portable
>>>>> signature.
>>>> I originally suggested that the portable EVM xattr should be
>>>> converted before it is written (in evm_inode_post_setxattr()),
>>>> but by the time we get to evm_inode_post_setxattr(), the
>>>> portable EVM xattr has already been written. Can the
>>>> verification be done earlier in evm_inode_setxattr(), before the
>>>> portable EVM xattr is written?
>>> We could do everything in evm_inode_setxattr() and return some
>>> special error code into vfs_setxattr() that will:
>>> 1) don't allow call __vfs_setxattr_noperm();
>>> 2) prevent special error code return and return 0 instead in the
>>> same time.
>> I'm wondering if there will be an issue with the order in which the
>> security xattrs are installed. For example, what guarantees that
>> security.ima is installed before security.evm?
> This is archiver's part of work. As I wrote before (and you also could
> see this in my patch for tar), archiver must copy EVM xattr the last
> one. We have nothing to do with this in kernel.
I didn't follow the entire discussion. But that this xattr needs to be
written last was kind of what I was looking for.
So now you are writing security.evm with a portable digital signature. I
suppose that this involves signature verification by the kernel. What
happens if the signature cannot be verified, e.g., target doesn't have
the public key. How does the kernel react?
Stefan
|
|
From: Mikhail K. <vie...@vi...> - 2017-01-11 23:18:39
|
В Wed, 11 Jan 2017 15:15:13 -0500 Stefan Berger <st...@li...> пишет: > On 01/09/2017 08:29 PM, Mikhail Kurinnoi wrote: > > В Mon, 09 Jan 2017 18:11:01 -0500 > > Mimi Zohar <zo...@li...> пишет: > > > >> On Tue, 2017-01-10 at 00:06 +0300, Mikhail Kurinnoi wrote: > >>> В Mon, 09 Jan 2017 08:40:45 -0500 > >>> Mimi Zohar <zo...@li...> пишет: > >>> > >>>> On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote: > >>>>> version 3 changes: > >>>>> 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM > >>>>> signature verification failed. > >>>> This can't be right. The boot comnand line option > >>>> "ima_appraise=fix" allows IMA to go into a special mode to "fix" > >>>> the xattrs. > >>>>> Since, __vfs_setxattr_noperm() don't count on > >>>>> evm_inode_setxattr(), during file unpacking from atchive EVM > >>>>> zero-filled sign could be updated by IMA without EVM signature > >>>>> status checking. So, even if you have EVM portable signature > >>>>> signed by another key, IMA will update EVM signature to HMAC > >>>>> any way, no matter what you have in security.evm xattr or you > >>>>> don't have it at all. > >>>> EVM and IMA are two separate modules, which communicate with each > >>>> other via the evm_verifyxattr() API. Changes to EVM, should not > >>>> affect IMA. If they do, then something is not right. > >>> Something is wrong, because if we have FS mounted with iversion > >>> flag, we call ima_file_free() -> ima_check_last_writer(), that > >>> don't care about evm_verifyxattr() API at all. > >> This is working as designed. The file integrity verification > >> occurred on file open. On __fput() the file hash is updated, which > >> causes security.evm to be updated. > > I see. > > > > Probably, we could add into evm_ima_xattr_type enumeration > > EVM_XATTR_PROTECTED, that we could use as EVM xattr signature type > > if we have EVM portable signature verification fail, that will be > > used only for "security.evm" attr, as additional check with > > evm_protected_xattr() in evm_inode_post_setxattr() and > > evm_inode_post_removexattr(). In this way we have INTEGRITY_FAIL in > > evm_verify_hmac(), since evm_verify_hmac() have no idea about > > EVM_XATTR_PROTECTED signature type, and prevent EVM xattr update on > > __fput() if we use IMA xattr hash but not IMA_DIGSIG. > > > > Is it possible? > > > > > >>> I have this issue without IMA/EVM fix mode. I mean, I faced with > >>> this issue on IMA/EVM normal mode during archive unpacking with > >>> EVM portable signature signed by wrong cert, that was replaced on > >>> EVM zero-filled signature with evm_reset_status(). And after that > >>> IMA update inode EVM xattr with proper HMAC by ima_file_free() > >>> call. > >>>>> 2) Added integrity_audit_msg() in order to get some error in > >>>>> audit syslog on EVM portable signature verification fail. > >>>>> > >>>>> version 2 changes: > >>>>> 1) evm_verify_hmac() prevent work with EVM portable signature. > >>>>> If file signed by EVM portable signature, evm_verify_hmac() > >>>>> return INTEGRITY_FAIL; > >>>>> 2) evm_inode_post_setxattr() now count on EVM portable > >>>>> signature. > >>>> I originally suggested that the portable EVM xattr should be > >>>> converted before it is written (in evm_inode_post_setxattr()), > >>>> but by the time we get to evm_inode_post_setxattr(), the > >>>> portable EVM xattr has already been written. Can the > >>>> verification be done earlier in evm_inode_setxattr(), before the > >>>> portable EVM xattr is written? > >>> We could do everything in evm_inode_setxattr() and return some > >>> special error code into vfs_setxattr() that will: > >>> 1) don't allow call __vfs_setxattr_noperm(); > >>> 2) prevent special error code return and return 0 instead in the > >>> same time. > >> I'm wondering if there will be an issue with the order in which the > >> security xattrs are installed. For example, what guarantees that > >> security.ima is installed before security.evm? > > This is archiver's part of work. As I wrote before (and you also > > could see this in my patch for tar), archiver must copy EVM xattr > > the last one. We have nothing to do with this in kernel. > > I didn't follow the entire discussion. But that this xattr needs to > be written last was kind of what I was looking for. > > So now you are writing security.evm with a portable digital > signature. I suppose that this involves signature verification by the > kernel. What happens if the signature cannot be verified, e.g., > target doesn't have the public key. How does the kernel react? Kernel will react in the same way as for file signed by standard EVM digsig without public key loaded - verification will be failed. Plus, kernel will install EVM "immutable" signature with invalid payload that will never changed to HMAC. If you have target system without public key and you have archive with "portable" "security.evm", you could use "exclude" archiver's flag for "security.evm" xattr during file extracting. In this way, you will have all xattrs installed and HMAC (generated by EVM). We could have a lot of options here in archiver. We could "exclude" or "demand" "security.evm" xattr depending of what we need for target system. -- Best regards, Mikhail Kurinnoi |
|
From: Stefan B. <st...@li...> - 2017-01-11 23:31:19
|
On 01/11/2017 06:18 PM, Mikhail Kurinnoi wrote: > В Wed, 11 Jan 2017 15:15:13 -0500 > Stefan Berger <st...@li...> пишет: > >> On 01/09/2017 08:29 PM, Mikhail Kurinnoi wrote: >>> В Mon, 09 Jan 2017 18:11:01 -0500 >>> Mimi Zohar <zo...@li...> пишет: >>> >>>> On Tue, 2017-01-10 at 00:06 +0300, Mikhail Kurinnoi wrote: >>>>> В Mon, 09 Jan 2017 08:40:45 -0500 >>>>> Mimi Zohar <zo...@li...> пишет: >>>>> >>>>>> On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote: >>>>>>> version 3 changes: >>>>>>> 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM >>>>>>> signature verification failed. >>>>>> This can't be right. The boot comnand line option >>>>>> "ima_appraise=fix" allows IMA to go into a special mode to "fix" >>>>>> the xattrs. >>>>>>> Since, __vfs_setxattr_noperm() don't count on >>>>>>> evm_inode_setxattr(), during file unpacking from atchive EVM >>>>>>> zero-filled sign could be updated by IMA without EVM signature >>>>>>> status checking. So, even if you have EVM portable signature >>>>>>> signed by another key, IMA will update EVM signature to HMAC >>>>>>> any way, no matter what you have in security.evm xattr or you >>>>>>> don't have it at all. >>>>>> EVM and IMA are two separate modules, which communicate with each >>>>>> other via the evm_verifyxattr() API. Changes to EVM, should not >>>>>> affect IMA. If they do, then something is not right. >>>>> Something is wrong, because if we have FS mounted with iversion >>>>> flag, we call ima_file_free() -> ima_check_last_writer(), that >>>>> don't care about evm_verifyxattr() API at all. >>>> This is working as designed. The file integrity verification >>>> occurred on file open. On __fput() the file hash is updated, which >>>> causes security.evm to be updated. >>> I see. >>> >>> Probably, we could add into evm_ima_xattr_type enumeration >>> EVM_XATTR_PROTECTED, that we could use as EVM xattr signature type >>> if we have EVM portable signature verification fail, that will be >>> used only for "security.evm" attr, as additional check with >>> evm_protected_xattr() in evm_inode_post_setxattr() and >>> evm_inode_post_removexattr(). In this way we have INTEGRITY_FAIL in >>> evm_verify_hmac(), since evm_verify_hmac() have no idea about >>> EVM_XATTR_PROTECTED signature type, and prevent EVM xattr update on >>> __fput() if we use IMA xattr hash but not IMA_DIGSIG. >>> >>> Is it possible? >>> >>> >>>>> I have this issue without IMA/EVM fix mode. I mean, I faced with >>>>> this issue on IMA/EVM normal mode during archive unpacking with >>>>> EVM portable signature signed by wrong cert, that was replaced on >>>>> EVM zero-filled signature with evm_reset_status(). And after that >>>>> IMA update inode EVM xattr with proper HMAC by ima_file_free() >>>>> call. >>>>>>> 2) Added integrity_audit_msg() in order to get some error in >>>>>>> audit syslog on EVM portable signature verification fail. >>>>>>> >>>>>>> version 2 changes: >>>>>>> 1) evm_verify_hmac() prevent work with EVM portable signature. >>>>>>> If file signed by EVM portable signature, evm_verify_hmac() >>>>>>> return INTEGRITY_FAIL; >>>>>>> 2) evm_inode_post_setxattr() now count on EVM portable >>>>>>> signature. >>>>>> I originally suggested that the portable EVM xattr should be >>>>>> converted before it is written (in evm_inode_post_setxattr()), >>>>>> but by the time we get to evm_inode_post_setxattr(), the >>>>>> portable EVM xattr has already been written. Can the >>>>>> verification be done earlier in evm_inode_setxattr(), before the >>>>>> portable EVM xattr is written? >>>>> We could do everything in evm_inode_setxattr() and return some >>>>> special error code into vfs_setxattr() that will: >>>>> 1) don't allow call __vfs_setxattr_noperm(); >>>>> 2) prevent special error code return and return 0 instead in the >>>>> same time. >>>> I'm wondering if there will be an issue with the order in which the >>>> security xattrs are installed. For example, what guarantees that >>>> security.ima is installed before security.evm? >>> This is archiver's part of work. As I wrote before (and you also >>> could see this in my patch for tar), archiver must copy EVM xattr >>> the last one. We have nothing to do with this in kernel. >> I didn't follow the entire discussion. But that this xattr needs to >> be written last was kind of what I was looking for. >> >> So now you are writing security.evm with a portable digital >> signature. I suppose that this involves signature verification by the >> kernel. What happens if the signature cannot be verified, e.g., >> target doesn't have the public key. How does the kernel react? > Kernel will react in the same way as for file signed by standard EVM > digsig without public key loaded - verification will be failed. Plus, > kernel will install EVM "immutable" signature with invalid payload that > will never changed to HMAC. What does that mean now? Can I access that file or do I have to remove it ? > > If you have target system without public key and you have archive with > "portable" "security.evm", you could use "exclude" archiver's flag for > "security.evm" xattr during file extracting. In this way, you will have > all xattrs installed and HMAC (generated by EVM). So at least the following applications likely need to learn about this new feature: rsync, libarchive (bsdtar), GNU tar, libattr (copying xattrs when copying files). > > We could have a lot of options here in archiver. We could "exclude" or > "demand" "security.evm" xattr depending of what we need for target > system. > I guess several apps may need to have such a flag then... |
|
From: Mikhail K. <vie...@vi...> - 2017-01-12 00:00:15
|
В Wed, 11 Jan 2017 18:31:02 -0500 Stefan Berger <st...@li...> пишет: > On 01/11/2017 06:18 PM, Mikhail Kurinnoi wrote: > > В Wed, 11 Jan 2017 15:15:13 -0500 > > Stefan Berger <st...@li...> пишет: > > > >> On 01/09/2017 08:29 PM, Mikhail Kurinnoi wrote: > >>> В Mon, 09 Jan 2017 18:11:01 -0500 > >>> Mimi Zohar <zo...@li...> пишет: > >>> > >>>> On Tue, 2017-01-10 at 00:06 +0300, Mikhail Kurinnoi wrote: > >>>>> В Mon, 09 Jan 2017 08:40:45 -0500 > >>>>> Mimi Zohar <zo...@li...> пишет: > >>>>> > >>>>>> On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote: > >>>>>>> version 3 changes: > >>>>>>> 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM > >>>>>>> signature verification failed. > >>>>>> This can't be right. The boot comnand line option > >>>>>> "ima_appraise=fix" allows IMA to go into a special mode to > >>>>>> "fix" the xattrs. > >>>>>>> Since, __vfs_setxattr_noperm() don't count on > >>>>>>> evm_inode_setxattr(), during file unpacking from atchive EVM > >>>>>>> zero-filled sign could be updated by IMA without EVM signature > >>>>>>> status checking. So, even if you have EVM portable signature > >>>>>>> signed by another key, IMA will update EVM signature to HMAC > >>>>>>> any way, no matter what you have in security.evm xattr or you > >>>>>>> don't have it at all. > >>>>>> EVM and IMA are two separate modules, which communicate with > >>>>>> each other via the evm_verifyxattr() API. Changes to EVM, > >>>>>> should not affect IMA. If they do, then something is not > >>>>>> right. > >>>>> Something is wrong, because if we have FS mounted with iversion > >>>>> flag, we call ima_file_free() -> ima_check_last_writer(), that > >>>>> don't care about evm_verifyxattr() API at all. > >>>> This is working as designed. The file integrity verification > >>>> occurred on file open. On __fput() the file hash is updated, > >>>> which causes security.evm to be updated. > >>> I see. > >>> > >>> Probably, we could add into evm_ima_xattr_type enumeration > >>> EVM_XATTR_PROTECTED, that we could use as EVM xattr signature type > >>> if we have EVM portable signature verification fail, that will be > >>> used only for "security.evm" attr, as additional check with > >>> evm_protected_xattr() in evm_inode_post_setxattr() and > >>> evm_inode_post_removexattr(). In this way we have INTEGRITY_FAIL > >>> in evm_verify_hmac(), since evm_verify_hmac() have no idea about > >>> EVM_XATTR_PROTECTED signature type, and prevent EVM xattr update > >>> on __fput() if we use IMA xattr hash but not IMA_DIGSIG. > >>> > >>> Is it possible? > >>> > >>> > >>>>> I have this issue without IMA/EVM fix mode. I mean, I faced with > >>>>> this issue on IMA/EVM normal mode during archive unpacking with > >>>>> EVM portable signature signed by wrong cert, that was replaced > >>>>> on EVM zero-filled signature with evm_reset_status(). And after > >>>>> that IMA update inode EVM xattr with proper HMAC by > >>>>> ima_file_free() call. > >>>>>>> 2) Added integrity_audit_msg() in order to get some error in > >>>>>>> audit syslog on EVM portable signature verification fail. > >>>>>>> > >>>>>>> version 2 changes: > >>>>>>> 1) evm_verify_hmac() prevent work with EVM portable signature. > >>>>>>> If file signed by EVM portable signature, evm_verify_hmac() > >>>>>>> return INTEGRITY_FAIL; > >>>>>>> 2) evm_inode_post_setxattr() now count on EVM portable > >>>>>>> signature. > >>>>>> I originally suggested that the portable EVM xattr should be > >>>>>> converted before it is written (in evm_inode_post_setxattr()), > >>>>>> but by the time we get to evm_inode_post_setxattr(), the > >>>>>> portable EVM xattr has already been written. Can the > >>>>>> verification be done earlier in evm_inode_setxattr(), before > >>>>>> the portable EVM xattr is written? > >>>>> We could do everything in evm_inode_setxattr() and return some > >>>>> special error code into vfs_setxattr() that will: > >>>>> 1) don't allow call __vfs_setxattr_noperm(); > >>>>> 2) prevent special error code return and return 0 instead in the > >>>>> same time. > >>>> I'm wondering if there will be an issue with the order in which > >>>> the security xattrs are installed. For example, what > >>>> guarantees that security.ima is installed before security.evm? > >>> This is archiver's part of work. As I wrote before (and you also > >>> could see this in my patch for tar), archiver must copy EVM xattr > >>> the last one. We have nothing to do with this in kernel. > >> I didn't follow the entire discussion. But that this xattr needs to > >> be written last was kind of what I was looking for. > >> > >> So now you are writing security.evm with a portable digital > >> signature. I suppose that this involves signature verification by > >> the kernel. What happens if the signature cannot be verified, e.g., > >> target doesn't have the public key. How does the kernel react? > > Kernel will react in the same way as for file signed by standard EVM > > digsig without public key loaded - verification will be failed. > > Plus, kernel will install EVM "immutable" signature with invalid > > payload that will never changed to HMAC. > > What does that mean now? Can I access that file or do I have to > remove it ? You can't access that file, you be able only remove it. > > > > If you have target system without public key and you have archive > > with "portable" "security.evm", you could use "exclude" archiver's > > flag for "security.evm" xattr during file extracting. In this way, > > you will have all xattrs installed and HMAC (generated by EVM). > > So at least the following applications likely need to learn about > this new feature: > rsync, libarchive (bsdtar), GNU tar, libattr (copying xattrs when > copying files). Yes, we need application support for sure. But we don't need portable signature support during file copying. The main support for EVM portable signature should be realized in archivers. You don't need EVM portable signature support during file copying, all you need in this case - don't allow any "security.evm" be copyed, since on source you will have HMAC or EVM digsig connected to inode. No reason convert HMAC > portable > HMAC if we do this at realtime, dest. will have proper HMAC for sure or you will see an error (if you can't copy some xattr, mode/gid/uid...). EVM portable digsig aimed to protect archived data from manipulation. > > > > We could have a lot of options here in archiver. We could "exclude" > > or "demand" "security.evm" xattr depending of what we need for > > target system. > > > I guess several apps may need to have such a flag then... > Exactly. -- Best regards, Mikhail Kurinnoi |
Thanks for helping keep SourceForge clean.
X