| Message ID | 20171026083144.16247-1-mjg59@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
В Thu, 26 Oct 2017 01:31:44 -0700 Matthew Garrett <mjg59@google.com> пишет: > @@ -317,7 +319,7 @@ void ima_update_xattr(struct integrity_iint_cache > *iint, struct file *file) int rc = 0; > > /* do not collect and update hash for digital signatures */ > - if (iint->flags & IMA_DIGSIG) > + if (iint->flags & IMA_DIGSIG || iint->flags & > EVM_IMMUTABLE_DIGSIG) return; > Isn't this mean, we already changed files data, and we just don't allow IMA xattr update? This file will not pass integrity verification next time. I thought, the idea was prevent data changes, and in this way prevent IMA xattr update.
On Thu, Oct 26, 2017 at 2:03 AM, Mikhail Kurinnoi <viewizard@viewizard.com> wrote: > В Thu, 26 Oct 2017 01:31:44 -0700 > Matthew Garrett <mjg59@google.com> пишет: > >> @@ -317,7 +319,7 @@ void ima_update_xattr(struct integrity_iint_cache >> *iint, struct file *file) int rc = 0; >> >> /* do not collect and update hash for digital signatures */ >> - if (iint->flags & IMA_DIGSIG) >> + if (iint->flags & IMA_DIGSIG || iint->flags & >> EVM_IMMUTABLE_DIGSIG) return; >> > > Isn't this mean, we already changed files data, and we just don't allow > IMA xattr update? This file will not pass integrity verification > next time. That's fine - policy may not care. It's easier to sign all files and then leave enforcement up to the local policy than it is to determine in advance which files will need protection. > I thought, the idea was prevent data changes, and in this way prevent > IMA xattr update. No, the goal is to be able to detect when files have been modified and (optionally) restrict access as a result. Otherwise the packaging system has to be able to identify all files that may be legitimately modified, which is something that may differ depending on the client. It's much easier to permit the data modification and have the local policy block reading or execution if it's actually a sensitive file.
В Thu, 26 Oct 2017 02:08:25 -0700 Matthew Garrett <mjg59@google.com> пишет: > On Thu, Oct 26, 2017 at 2:03 AM, Mikhail Kurinnoi > <viewizard@viewizard.com> wrote: > > В Thu, 26 Oct 2017 01:31:44 -0700 > > Matthew Garrett <mjg59@google.com> пишет: > > > >> @@ -317,7 +319,7 @@ void ima_update_xattr(struct > >> integrity_iint_cache *iint, struct file *file) int rc = 0; > >> > >> /* do not collect and update hash for digital signatures */ > >> - if (iint->flags & IMA_DIGSIG) > >> + if (iint->flags & IMA_DIGSIG || iint->flags & > >> EVM_IMMUTABLE_DIGSIG) return; > >> > > > > Isn't this mean, we already changed files data, and we just don't > > allow IMA xattr update? This file will not pass integrity > > verification next time. > > That's fine - policy may not care. It's easier to sign all files and > then leave enforcement up to the local policy than it is to determine > in advance which files will need protection. > > > I thought, the idea was prevent data changes, and in this way > > prevent IMA xattr update. > > No, the goal is to be able to detect when files have been modified and > (optionally) restrict access as a result. Otherwise the packaging > system has to be able to identify all files that may be legitimately > modified, which is something that may differ depending on the client. > It's much easier to permit the data modification and have the local > policy block reading or execution if it's actually a sensitive file. Hmm... http://www.spinics.net/lists/linux-integrity/msg00151.html probably, we should decide first, what exactly immutable EVM mean. It's hard to propose something or test patch if we still have misunderstanding in concept of immutable EVM.
On Thu, 2017-10-26 at 12:46 +0300, Mikhail Kurinnoi wrote: > В Thu, 26 Oct 2017 02:08:25 -0700 > Matthew Garrett <mjg59@google.com> пишет: > > > On Thu, Oct 26, 2017 at 2:03 AM, Mikhail Kurinnoi > > <viewizard@viewizard.com> wrote: > > > В Thu, 26 Oct 2017 01:31:44 -0700 > > > Matthew Garrett <mjg59@google.com> пишет: > > > > > >> @@ -317,7 +319,7 @@ void ima_update_xattr(struct > > >> integrity_iint_cache *iint, struct file *file) int rc = 0; > > >> > > >> /* do not collect and update hash for digital signatures */ > > >> - if (iint->flags & IMA_DIGSIG) > > >> + if (iint->flags & IMA_DIGSIG || iint->flags & > > >> EVM_IMMUTABLE_DIGSIG) return; > > >> > > > > > > Isn't this mean, we already changed files data, and we just don't > > > allow IMA xattr update? This file will not pass integrity > > > verification next time. > > > > That's fine - policy may not care. It's easier to sign all files and > > then leave enforcement up to the local policy than it is to determine > > in advance which files will need protection. You're both correct. Signing the file data will prevent security.ima from changing, assuming the file is in policy and there is a FILE_CHECK rule. We're requiring the signed file-metadata include security.ima, but it currently doesn't require it to contain a file signature. Only having a single file signature, was one of Matthew's requirements. IMA accessing the EVM flags crosses the boundary between EVM/IMA. Just as the LSMs protect their own xattr label, EVM should protect security.evm, preventing it from changing. There's no need for the test here in IMA. An additional patch could prevent IMA from allowing files with the portable/immutable signatures from changing, just as it currently prevents signed file data from changing. Refer to the IMA_XATTR_DIGEST case statement in ima_appraise_measurement(). It should be based on the result returned from evm_verifyxattr(), as Mikhail suggested. > > > I thought, the idea was prevent data changes, and in this way > > > prevent IMA xattr update. > > > > No, the goal is to be able to detect when files have been modified and > > (optionally) restrict access as a result. Otherwise the packaging > > system has to be able to identify all files that may be legitimately > > modified, which is something that may differ depending on the client. > > It's much easier to permit the data modification and have the local > > policy block reading or execution if it's actually a sensitive file. > > Hmm... > http://www.spinics.net/lists/linux-integrity/msg00151.html > > probably, we should decide first, what exactly immutable EVM mean. > It's hard to propose something or test patch if we still have > misunderstanding in concept of immutable EVM. Agreed. I think EVM should prevent any changes to the file metadata that would cause the portable/immutable EVM signature to be invalid. For example, the change in evm_inode_setattr() permits the file metadata to change. The same is true for removing files or writing security xattrs. Mimi
> -----Original Message----- > From: Matthew Garrett [mailto:mjg59@google.com] > Sent: 26 October 2017 11:32 > To: linux-integrity@vger.kernel.org > Cc: zohar@linux.vnet.ibm.com; Matthew Garrett <mjg59@google.com>; > Dmitry Kasatkin <dmitry.kasatkin@huawei.com>; Mikhail Kurinnoi > <viewizard@viewizard.com> > Subject: [RFC] EVM: Add support for portable signature format > > Ok I /think/ I have everything covered here. I'm away from my workstation > this week so can't test this beyond building it right now, but thought I should > send it out so people can check my reasoning. > > The EVM signature includes the inode number and (optionally) the filesystem > UUID, making it impractical to ship EVM signatures in packages. This patch > adds a new portable format intended to allow distributions to include EVM > signatures. It is identical to the existing format but hardcodes the inode and > generation numbers to 0 and does not include the filesystem UUID even if > the kernel is configured to do so. > > Removing the inode means that the metadata and signature from one file > could be copied to another file without invalidating it. This is avoided by > ensuring that an IMA xattr is present during EVM validation. > > Portable signatures are intended to be immutable - ie, they will never be > transformed into HMACs. This is achieved via the following changes: > > 1) An update is not triggered on initial xattr write > 2) evm_verifyxattr returns INTEGRITY_PASS_IMMUTABLE for valid portable > digital signatures and sets the EVM_IMMUTABLE_DIGSIG flag > 3) ima_appraise_measurement() is updated to treat > INTEGRITY_PASS_IMMUTABLE as valid, allowing IMA appraisal to pass > 4) ima_update_xattr() does not update the IMA xattr if > EVM_IMMUTABLE_DIGSIG is set > 5) any other codepath that calls evm_update_evmxattr() treats > INTEGRITY_PASS_IMMUTABLE as a failure, and prevents the write that would > trigger an HMAC writeout > > The only path that will still result in an update will be if IMA is in "fix" mode > while a valid HMAC key is loaded. > > Based on earlier work by Dmitry Kasatkin and Mikhail Kurinnoi. > > Cc: Dmitry Kasatkin <dmitry.kasatkin@huawei.com> > Cc: Mikhail Kurinnoi <viewizard@viewizard.com> > --- > include/linux/integrity.h | 1 + > security/integrity/evm/evm.h | 2 +- > security/integrity/evm/evm_crypto.c | 37 ++++++++++++++++++++++++++--- > ------ > security/integrity/evm/evm_main.c | 23 ++++++++++++++-------- > security/integrity/ima/ima_appraise.c | 6 ++++-- > security/integrity/integrity.h | 2 ++ > 6 files changed, 51 insertions(+), 20 deletions(-) > > diff --git a/include/linux/integrity.h b/include/linux/integrity.h index > c2d6082a1a4c..858d3f4a2241 100644 > --- a/include/linux/integrity.h > +++ b/include/linux/integrity.h > @@ -14,6 +14,7 @@ > > enum integrity_status { > INTEGRITY_PASS = 0, > + INTEGRITY_PASS_IMMUTABLE, > INTEGRITY_FAIL, > INTEGRITY_NOLABEL, > INTEGRITY_NOXATTRS, > diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h > index f5f12727771a..2ff02459fcfd 100644 > --- a/security/integrity/evm/evm.h > +++ b/security/integrity/evm/evm.h > @@ -48,7 +48,7 @@ int evm_calc_hmac(struct dentry *dentry, const char > *req_xattr_name, > 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, char type, char *digest); > int evm_init_hmac(struct inode *inode, const struct xattr *xattr, > char *hmac_val); > int evm_init_secfs(void); > diff --git a/security/integrity/evm/evm_crypto.c > b/security/integrity/evm/evm_crypto.c > index 1d32cd20009a..8855722529d4 100644 > --- a/security/integrity/evm/evm_crypto.c > +++ b/security/integrity/evm/evm_crypto.c > @@ -138,7 +138,7 @@ static struct shash_desc *init_desc(char type) > * protection.) > */ > static void hmac_add_misc(struct shash_desc *desc, struct inode *inode, > - char *digest) > + char type, char *digest) > { > struct h_misc { > unsigned long ino; > @@ -149,8 +149,13 @@ static void hmac_add_misc(struct shash_desc *desc, > struct inode *inode, > } hmac_misc; > > memset(&hmac_misc, 0, sizeof(hmac_misc)); > - hmac_misc.ino = inode->i_ino; > - hmac_misc.generation = inode->i_generation; > + /* Don't include the inode or generation number in portable > + * signatures > + */ > + if (type != EVM_XATTR_PORTABLE_DIGSIG) { > + hmac_misc.ino = inode->i_ino; > + hmac_misc.generation = inode->i_generation; > + } > /* The hmac uid and gid must be encoded in the initial user > * namespace (not the filesystems user namespace) as encoding > * them in the filesystems user namespace allows an attack @@ - > 163,7 +168,8 @@ static void hmac_add_misc(struct shash_desc *desc, struct > inode *inode, > 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)); > - if (evm_hmac_attrs & EVM_ATTR_FSUUID) > + if ((evm_hmac_attrs & EVM_ATTR_FSUUID) && > + type != EVM_XATTR_PORTABLE_DIGSIG) > crypto_shash_update(desc, &inode->i_sb->s_uuid.b[0], > sizeof(inode->i_sb->s_uuid)); > crypto_shash_final(desc, digest); > @@ -189,6 +195,7 @@ static int evm_calc_hmac_or_hash(struct dentry > *dentry, > char *xattr_value = NULL; > int error; > int size; > + bool ima_present = false; > > if (!(inode->i_opflags & IOP_XATTR)) > return -EOPNOTSUPP; > @@ -199,11 +206,18 @@ static int evm_calc_hmac_or_hash(struct dentry > *dentry, > > error = -ENODATA; > for (xattrname = evm_config_xattrnames; *xattrname != NULL; > xattrname++) { > + bool is_ima = false; > + > + if (strcmp(*xattrname, XATTR_NAME_IMA) == 0) > + is_ima = true; > + > if ((req_xattr_name && req_xattr_value) > && !strcmp(*xattrname, req_xattr_name)) { > error = 0; > crypto_shash_update(desc, (const u8 > *)req_xattr_value, > req_xattr_value_len); > + if (is_ima) > + ima_present = true; > continue; > } > size = vfs_getxattr_alloc(dentry, *xattrname, @@ -218,9 > +232,14 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, > error = 0; > xattr_size = size; > crypto_shash_update(desc, (const u8 *)xattr_value, > xattr_size); > + if (is_ima) > + ima_present = true; > } > - hmac_add_misc(desc, inode, digest); > + hmac_add_misc(desc, inode, type, digest); > > + /* Portable EVM signatures must include an IMA hash */ > + if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present) > + return -EPERM; > out: > kfree(xattr_value); > kfree(desc); > @@ -232,15 +251,15 @@ int evm_calc_hmac(struct dentry *dentry, const > char *req_xattr_name, > char *digest) > { > return evm_calc_hmac_or_hash(dentry, req_xattr_name, > req_xattr_value, > - req_xattr_value_len, EVM_XATTR_HMAC, > digest); > + req_xattr_value_len, EVM_XATTR_HMAC, 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) > + char type, 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, type, digest); > } > > /* > @@ -280,7 +299,7 @@ int evm_init_hmac(struct inode *inode, const struct > xattr *lsm_xattr, > } > > crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len); > - hmac_add_misc(desc, inode, hmac_val); > + hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val); > kfree(desc); > return 0; > } > diff --git a/security/integrity/evm/evm_main.c > b/security/integrity/evm/evm_main.c > index 063d38aef64e..7ab633eab576 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -120,7 +120,8 @@ static enum integrity_status evm_verify_hmac(struct > dentry *dentry, > enum integrity_status evm_status = INTEGRITY_PASS; > int rc, xattr_len; > > - if (iint && iint->evm_status == INTEGRITY_PASS) > + if (iint && (iint->evm_status == INTEGRITY_PASS || > + iint->evm_status == INTEGRITY_PASS_IMMUTABLE)) > return iint->evm_status; > > /* if status is not PASS, try to check again - against -ENOMEM */ @@ > -161,22 +162,25 @@ static enum integrity_status evm_verify_hmac(struct > dentry *dentry, > rc = -EINVAL; > break; > case EVM_IMA_XATTR_DIGSIG: > + case EVM_XATTR_PORTABLE_DIGSIG: > rc = evm_calc_hash(dentry, xattr_name, xattr_value, > - xattr_value_len, calc.digest); > + xattr_value_len, xattr_data->type, > + calc.digest); > if (rc) > break; > rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM, > (const char *)xattr_data, xattr_len, > calc.digest, sizeof(calc.digest)); > if (!rc) { > - /* Replace RSA with HMAC if not mounted readonly > and > - * not immutable > - */ > - if (!IS_RDONLY(d_backing_inode(dentry)) && > - !IS_IMMUTABLE(d_backing_inode(dentry))) > + if (xattr_data->type == > EVM_XATTR_PORTABLE_DIGSIG) { > + iint->flags |= EVM_IMMUTABLE_DIGSIG; > + evm_status = INTEGRITY_PASS_IMMUTABLE; > + } else if (!IS_RDONLY(d_backing_inode(dentry)) && > + !IS_IMMUTABLE(d_backing_inode(dentry))) > { > evm_update_evmxattr(dentry, xattr_name, > xattr_value, > xattr_value_len); > + } > } > break; > default: > @@ -292,6 +296,7 @@ static int evm_protect_xattr(struct dentry *dentry, > const char *xattr_name, > return 0; > evm_status = evm_verify_current_integrity(dentry); > if ((evm_status == INTEGRITY_PASS) || > + (evm_status == INTEGRITY_PASS_IMMUTABLE) || > (evm_status == INTEGRITY_NOXATTRS)) > return 0; > goto out; > @@ -345,7 +350,8 @@ int evm_inode_setxattr(struct dentry *dentry, const > char *xattr_name, > if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) { > if (!xattr_value_len) > return -EINVAL; > - if (xattr_data->type != EVM_IMA_XATTR_DIGSIG) > + if (xattr_data->type != EVM_IMA_XATTR_DIGSIG && > + xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) > return -EPERM; > } > return evm_protect_xattr(dentry, xattr_name, xattr_value, @@ - > 432,6 +438,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr > *attr) > return 0; > evm_status = evm_verify_current_integrity(dentry); > if ((evm_status == INTEGRITY_PASS) || > + (evm_status == INTEGRITY_PASS_IMMUTABLE) || > (evm_status == INTEGRITY_NOXATTRS)) > return 0; Something is wrong here? When integrity verification pass, this code WILL ALLOW to change attribute. But it is not possible... next time integrity verification will fail? Or I miss something? > integrity_audit_msg(AUDIT_INTEGRITY_METADATA, > d_backing_inode(dentry), diff --git a/security/integrity/ima/ima_appraise.c > b/security/integrity/ima/ima_appraise.c > index 809ba70fbbbf..6e277c5c7829 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -229,7 +229,9 @@ int ima_appraise_measurement(enum ima_hooks > func, > } > > status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, > iint); > - if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) > { > + if ((status != INTEGRITY_PASS) && > + (status != INTEGRITY_PASS_IMMUTABLE) && > + (status != INTEGRITY_UNKNOWN)) { > if ((status == INTEGRITY_NOLABEL) > || (status == INTEGRITY_NOXATTRS)) > cause = "missing-HMAC"; > @@ -317,7 +319,7 @@ void ima_update_xattr(struct integrity_iint_cache > *iint, struct file *file) > int rc = 0; > > /* do not collect and update hash for digital signatures */ > - if (iint->flags & IMA_DIGSIG) > + if (iint->flags & IMA_DIGSIG || iint->flags & > EVM_IMMUTABLE_DIGSIG) > return; > > rc = ima_collect_measurement(iint, file, NULL, 0, ima_hash_algo); diff > --git a/security/integrity/integrity.h b/security/integrity/integrity.h index > a53e7e4ab06c..cbc7de33fac7 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -33,6 +33,7 @@ > #define IMA_DIGSIG_REQUIRED 0x02000000 > #define IMA_PERMIT_DIRECTIO 0x04000000 > #define IMA_NEW_FILE 0x08000000 > +#define EVM_IMMUTABLE_DIGSIG 0x10000000 > > #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | > IMA_AUDIT | \ > IMA_APPRAISE_SUBMASK) > @@ -58,6 +59,7 @@ enum evm_ima_xattr_type { > EVM_XATTR_HMAC, > EVM_IMA_XATTR_DIGSIG, > IMA_XATTR_DIGEST_NG, > + EVM_XATTR_PORTABLE_DIGSIG, > IMA_XATTR_LAST > }; > > -- > 2.15.0.rc2.357.g7e34df9404-goog
> -----Original Message----- > From: Matthew Garrett [mailto:mjg59@google.com] > Sent: 26 October 2017 11:32 > To: linux-integrity@vger.kernel.org > Cc: zohar@linux.vnet.ibm.com; Matthew Garrett <mjg59@google.com>; > Dmitry Kasatkin <dmitry.kasatkin@huawei.com>; Mikhail Kurinnoi > <viewizard@viewizard.com> > Subject: [RFC] EVM: Add support for portable signature format > > Ok I /think/ I have everything covered here. I'm away from my workstation > this week so can't test this beyond building it right now, but thought I should > send it out so people can check my reasoning. > > The EVM signature includes the inode number and (optionally) the filesystem > UUID, making it impractical to ship EVM signatures in packages. This patch > adds a new portable format intended to allow distributions to include EVM > signatures. It is identical to the existing format but hardcodes the inode and > generation numbers to 0 and does not include the filesystem UUID even if > the kernel is configured to do so. > > Removing the inode means that the metadata and signature from one file > could be copied to another file without invalidating it. This is avoided by > ensuring that an IMA xattr is present during EVM validation. > > Portable signatures are intended to be immutable - ie, they will never be > transformed into HMACs. This is achieved via the following changes: > > 1) An update is not triggered on initial xattr write > 2) evm_verifyxattr returns INTEGRITY_PASS_IMMUTABLE for valid portable > digital signatures and sets the EVM_IMMUTABLE_DIGSIG flag > 3) ima_appraise_measurement() is updated to treat > INTEGRITY_PASS_IMMUTABLE as valid, allowing IMA appraisal to pass > 4) ima_update_xattr() does not update the IMA xattr if > EVM_IMMUTABLE_DIGSIG is set > 5) any other codepath that calls evm_update_evmxattr() treats > INTEGRITY_PASS_IMMUTABLE as a failure, and prevents the write that would > trigger an HMAC writeout > > The only path that will still result in an update will be if IMA is in "fix" mode > while a valid HMAC key is loaded. > > Based on earlier work by Dmitry Kasatkin and Mikhail Kurinnoi. > > Cc: Dmitry Kasatkin <dmitry.kasatkin@huawei.com> > Cc: Mikhail Kurinnoi <viewizard@viewizard.com> > --- > include/linux/integrity.h | 1 + > security/integrity/evm/evm.h | 2 +- > security/integrity/evm/evm_crypto.c | 37 ++++++++++++++++++++++++++--- > ------ > security/integrity/evm/evm_main.c | 23 ++++++++++++++-------- > security/integrity/ima/ima_appraise.c | 6 ++++-- > security/integrity/integrity.h | 2 ++ > 6 files changed, 51 insertions(+), 20 deletions(-) > > diff --git a/include/linux/integrity.h b/include/linux/integrity.h index > c2d6082a1a4c..858d3f4a2241 100644 > --- a/include/linux/integrity.h > +++ b/include/linux/integrity.h > @@ -14,6 +14,7 @@ > > enum integrity_status { > INTEGRITY_PASS = 0, > + INTEGRITY_PASS_IMMUTABLE, > INTEGRITY_FAIL, > INTEGRITY_NOLABEL, > INTEGRITY_NOXATTRS, > diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h > index f5f12727771a..2ff02459fcfd 100644 > --- a/security/integrity/evm/evm.h > +++ b/security/integrity/evm/evm.h > @@ -48,7 +48,7 @@ int evm_calc_hmac(struct dentry *dentry, const char > *req_xattr_name, > 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, char type, char *digest); > int evm_init_hmac(struct inode *inode, const struct xattr *xattr, > char *hmac_val); > int evm_init_secfs(void); > diff --git a/security/integrity/evm/evm_crypto.c > b/security/integrity/evm/evm_crypto.c > index 1d32cd20009a..8855722529d4 100644 > --- a/security/integrity/evm/evm_crypto.c > +++ b/security/integrity/evm/evm_crypto.c > @@ -138,7 +138,7 @@ static struct shash_desc *init_desc(char type) > * protection.) > */ > static void hmac_add_misc(struct shash_desc *desc, struct inode *inode, > - char *digest) > + char type, char *digest) > { > struct h_misc { > unsigned long ino; > @@ -149,8 +149,13 @@ static void hmac_add_misc(struct shash_desc *desc, > struct inode *inode, > } hmac_misc; > > memset(&hmac_misc, 0, sizeof(hmac_misc)); > - hmac_misc.ino = inode->i_ino; > - hmac_misc.generation = inode->i_generation; > + /* Don't include the inode or generation number in portable > + * signatures > + */ > + if (type != EVM_XATTR_PORTABLE_DIGSIG) { > + hmac_misc.ino = inode->i_ino; > + hmac_misc.generation = inode->i_generation; > + } > /* The hmac uid and gid must be encoded in the initial user > * namespace (not the filesystems user namespace) as encoding > * them in the filesystems user namespace allows an attack @@ - > 163,7 +168,8 @@ static void hmac_add_misc(struct shash_desc *desc, struct > inode *inode, > 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)); > - if (evm_hmac_attrs & EVM_ATTR_FSUUID) > + if ((evm_hmac_attrs & EVM_ATTR_FSUUID) && > + type != EVM_XATTR_PORTABLE_DIGSIG) > crypto_shash_update(desc, &inode->i_sb->s_uuid.b[0], > sizeof(inode->i_sb->s_uuid)); > crypto_shash_final(desc, digest); > @@ -189,6 +195,7 @@ static int evm_calc_hmac_or_hash(struct dentry > *dentry, > char *xattr_value = NULL; > int error; > int size; > + bool ima_present = false; > > if (!(inode->i_opflags & IOP_XATTR)) > return -EOPNOTSUPP; > @@ -199,11 +206,18 @@ static int evm_calc_hmac_or_hash(struct dentry > *dentry, > > error = -ENODATA; > for (xattrname = evm_config_xattrnames; *xattrname != NULL; > xattrname++) { > + bool is_ima = false; > + > + if (strcmp(*xattrname, XATTR_NAME_IMA) == 0) > + is_ima = true; > + > if ((req_xattr_name && req_xattr_value) > && !strcmp(*xattrname, req_xattr_name)) { > error = 0; > crypto_shash_update(desc, (const u8 > *)req_xattr_value, > req_xattr_value_len); > + if (is_ima) > + ima_present = true; > continue; > } > size = vfs_getxattr_alloc(dentry, *xattrname, @@ -218,9 > +232,14 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, > error = 0; > xattr_size = size; > crypto_shash_update(desc, (const u8 *)xattr_value, > xattr_size); > + if (is_ima) > + ima_present = true; > } > - hmac_add_misc(desc, inode, digest); > + hmac_add_misc(desc, inode, type, digest); > > + /* Portable EVM signatures must include an IMA hash */ > + if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present) > + return -EPERM; > out: > kfree(xattr_value); > kfree(desc); > @@ -232,15 +251,15 @@ int evm_calc_hmac(struct dentry *dentry, const > char *req_xattr_name, > char *digest) > { > return evm_calc_hmac_or_hash(dentry, req_xattr_name, > req_xattr_value, > - req_xattr_value_len, EVM_XATTR_HMAC, > digest); > + req_xattr_value_len, EVM_XATTR_HMAC, 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) > + char type, 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, type, digest); > } > > /* > @@ -280,7 +299,7 @@ int evm_init_hmac(struct inode *inode, const struct > xattr *lsm_xattr, > } > > crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len); > - hmac_add_misc(desc, inode, hmac_val); > + hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val); > kfree(desc); > return 0; > } > diff --git a/security/integrity/evm/evm_main.c > b/security/integrity/evm/evm_main.c > index 063d38aef64e..7ab633eab576 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -120,7 +120,8 @@ static enum integrity_status evm_verify_hmac(struct > dentry *dentry, > enum integrity_status evm_status = INTEGRITY_PASS; > int rc, xattr_len; > > - if (iint && iint->evm_status == INTEGRITY_PASS) > + if (iint && (iint->evm_status == INTEGRITY_PASS || > + iint->evm_status == INTEGRITY_PASS_IMMUTABLE)) > return iint->evm_status; > > /* if status is not PASS, try to check again - against -ENOMEM */ @@ > -161,22 +162,25 @@ static enum integrity_status evm_verify_hmac(struct > dentry *dentry, > rc = -EINVAL; > break; > case EVM_IMA_XATTR_DIGSIG: > + case EVM_XATTR_PORTABLE_DIGSIG: > rc = evm_calc_hash(dentry, xattr_name, xattr_value, > - xattr_value_len, calc.digest); > + xattr_value_len, xattr_data->type, > + calc.digest); > if (rc) > break; > rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM, > (const char *)xattr_data, xattr_len, > calc.digest, sizeof(calc.digest)); > if (!rc) { > - /* Replace RSA with HMAC if not mounted readonly > and > - * not immutable > - */ > - if (!IS_RDONLY(d_backing_inode(dentry)) && > - !IS_IMMUTABLE(d_backing_inode(dentry))) > + if (xattr_data->type == > EVM_XATTR_PORTABLE_DIGSIG) { > + iint->flags |= EVM_IMMUTABLE_DIGSIG; > + evm_status = INTEGRITY_PASS_IMMUTABLE; > + } else if (!IS_RDONLY(d_backing_inode(dentry)) && > + !IS_IMMUTABLE(d_backing_inode(dentry))) > { > evm_update_evmxattr(dentry, xattr_name, > xattr_value, > xattr_value_len); > + } > } > break; > default: > @@ -292,6 +296,7 @@ static int evm_protect_xattr(struct dentry *dentry, > const char *xattr_name, > return 0; > evm_status = evm_verify_current_integrity(dentry); > if ((evm_status == INTEGRITY_PASS) || > + (evm_status == INTEGRITY_PASS_IMMUTABLE) || > (evm_status == INTEGRITY_NOXATTRS)) > return 0; > goto out; > @@ -345,7 +350,8 @@ int evm_inode_setxattr(struct dentry *dentry, const > char *xattr_name, > if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) { > if (!xattr_value_len) > return -EINVAL; > - if (xattr_data->type != EVM_IMA_XATTR_DIGSIG) > + if (xattr_data->type != EVM_IMA_XATTR_DIGSIG && > + xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) > return -EPERM; > } Also I have an impression that evm_protect_xattr will allow to set security.ima for example, And it will cause to try to re-calculate hmac over immutable signature... > return evm_protect_xattr(dentry, xattr_name, xattr_value, @@ - > 432,6 +438,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr > *attr) > return 0; > evm_status = evm_verify_current_integrity(dentry); > if ((evm_status == INTEGRITY_PASS) || > + (evm_status == INTEGRITY_PASS_IMMUTABLE) || > (evm_status == INTEGRITY_NOXATTRS)) > return 0; > integrity_audit_msg(AUDIT_INTEGRITY_METADATA, > d_backing_inode(dentry), diff --git a/security/integrity/ima/ima_appraise.c > b/security/integrity/ima/ima_appraise.c > index 809ba70fbbbf..6e277c5c7829 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -229,7 +229,9 @@ int ima_appraise_measurement(enum ima_hooks > func, > } > > status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, > iint); > - if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) > { > + if ((status != INTEGRITY_PASS) && > + (status != INTEGRITY_PASS_IMMUTABLE) && > + (status != INTEGRITY_UNKNOWN)) { > if ((status == INTEGRITY_NOLABEL) > || (status == INTEGRITY_NOXATTRS)) > cause = "missing-HMAC"; > @@ -317,7 +319,7 @@ void ima_update_xattr(struct integrity_iint_cache > *iint, struct file *file) > int rc = 0; > > /* do not collect and update hash for digital signatures */ > - if (iint->flags & IMA_DIGSIG) > + if (iint->flags & IMA_DIGSIG || iint->flags & > EVM_IMMUTABLE_DIGSIG) > return; > > rc = ima_collect_measurement(iint, file, NULL, 0, ima_hash_algo); diff > --git a/security/integrity/integrity.h b/security/integrity/integrity.h index > a53e7e4ab06c..cbc7de33fac7 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -33,6 +33,7 @@ > #define IMA_DIGSIG_REQUIRED 0x02000000 > #define IMA_PERMIT_DIRECTIO 0x04000000 > #define IMA_NEW_FILE 0x08000000 > +#define EVM_IMMUTABLE_DIGSIG 0x10000000 > > #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | > IMA_AUDIT | \ > IMA_APPRAISE_SUBMASK) > @@ -58,6 +59,7 @@ enum evm_ima_xattr_type { > EVM_XATTR_HMAC, > EVM_IMA_XATTR_DIGSIG, > IMA_XATTR_DIGEST_NG, > + EVM_XATTR_PORTABLE_DIGSIG, > IMA_XATTR_LAST > }; > > -- > 2.15.0.rc2.357.g7e34df9404-goog
On Thu, Oct 26, 2017 at 8:22 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > On Thu, 2017-10-26 at 12:46 +0300, Mikhail Kurinnoi wrote: >> В Thu, 26 Oct 2017 02:08:25 -0700 >> Matthew Garrett <mjg59@google.com> пишет: >> > That's fine - policy may not care. It's easier to sign all files and >> > then leave enforcement up to the local policy than it is to determine >> > in advance which files will need protection. > > You're both correct. Signing the file data will prevent security.ima > from changing, assuming the file is in policy and there is a > FILE_CHECK rule. We're requiring the signed file-metadata include > security.ima, but it currently doesn't require it to contain a file > signature. Only having a single file signature, was one of Matthew's > requirements. > > IMA accessing the EVM flags crosses the boundary between EVM/IMA. Just > as the LSMs protect their own xattr label, EVM should protect > security.evm, preventing it from changing. There's no need for the > test here in IMA. Ok - so the preferred approach here would be to allow updating of security.ima if it's a hash rather than a digsig, and then have EVM validation fail later? That seems reasonable to me, but it'll mean reworking the EVM side a little in order to ensure that the EVM signature doesn't get rewritten into an HMAC in response. >> Hmm... >> http://www.spinics.net/lists/linux-integrity/msg00151.html >> >> probably, we should decide first, what exactly immutable EVM mean. >> It's hard to propose something or test patch if we still have >> misunderstanding in concept of immutable EVM. > > Agreed. I think EVM should prevent any changes to the file metadata > that would cause the portable/immutable EVM signature to be invalid. > For example, the change in evm_inode_setattr() permits the file > metadata to change. The same is true for removing files or writing > security xattrs. This seems to run counter to there being no linkage between EVM and IMA. If IMA is unaware that there's an EVM digital signature then it has no way of knowing that security.ima shouldn't be updated. The scenario that I want is basically the following: 1) New files with portable signatures can be written to the filesystem even if EVM is active 2) It should be possible to write a policy that allows these files to be arbitrarily modified, including being deleted I'd been interpreting "immutable" in this case to mean "the kernel will never replace the signature with an hmac" rather than "the file and protected information cannot be modified". If you think the latter is necessary then I think we need two separate signature types and to handle the two separately.
On Mon, 2017-10-30 at 10:53 +0000, Matthew Garrett wrote: > On Thu, Oct 26, 2017 at 8:22 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > On Thu, 2017-10-26 at 12:46 +0300, Mikhail Kurinnoi wrote: > >> В Thu, 26 Oct 2017 02:08:25 -0700 > >> Matthew Garrett <mjg59@google.com> пишет: > >> > That's fine - policy may not care. It's easier to sign all files and > >> > then leave enforcement up to the local policy than it is to determine > >> > in advance which files will need protection. > > > > You're both correct. Signing the file data will prevent security.ima > > from changing, assuming the file is in policy and there is a > > FILE_CHECK rule. We're requiring the signed file-metadata include > > security.ima, but it currently doesn't require it to contain a file > > signature. Only having a single file signature, was one of Matthew's > > requirements. > > > > IMA accessing the EVM flags crosses the boundary between EVM/IMA. Just > > as the LSMs protect their own xattr label, EVM should protect > > security.evm, preventing it from changing. There's no need for the > > test here in IMA. > > Ok - so the preferred approach here would be to allow updating of > security.ima if it's a hash rather than a digsig, and then have EVM > validation fail later? That seems reasonable to me, but it'll mean > reworking the EVM side a little in order to ensure that the EVM > signature doesn't get rewritten into an HMAC in response. > > >> Hmm... > >> http://www.spinics.net/lists/linux-integrity/msg00151.html > >> > >> probably, we should decide first, what exactly immutable EVM mean. > >> It's hard to propose something or test patch if we still have > >> misunderstanding in concept of immutable EVM. > > > > Agreed. I think EVM should prevent any changes to the file metadata > > that would cause the portable/immutable EVM signature to be invalid. > > For example, the change in evm_inode_setattr() permits the file > > metadata to change. The same is true for removing files or writing > > security xattrs. > > This seems to run counter to there being no linkage between EVM and > IMA. If IMA is unaware that there's an EVM digital signature then it > has no way of knowing that security.ima shouldn't be updated. This patch should be responsible for security.evm not changing, making it immutable. An additional patch would check the return code from evm_verifyxattr(), as Mikhail said. > The scenario that I want is basically the following: > > 1) New files with portable signatures can be written to the filesystem > even if EVM is active Agreed, that is similar to the existing EVM signature. > 2) It should be possible to write a policy that allows these files to > be arbitrarily modified, including being deleted File deletion is not the problem, but if we allow the file metadata to change, then the file verification will fail. > I'd been interpreting "immutable" in this case to mean "the kernel > will never replace the signature with an hmac" rather than "the file > and protected information cannot be modified". If you think the latter > is necessary then I think we need two separate signature types and to > handle the two separately. EVM, up to now, is reactive to file data and meta-data changes, replacing the file signature with an HMAC. For the new file signature to be immutable it needs to prevent security.evm from changing, which this patch currently does not do. We've already discussed that preventing the file data from changing is an IMA issue, which should be addressed in a separate patch by IMA. That leaves us with file metadata changes. So the question is not whether EVM will re-write security.evm, but whether it returns an error, preventing the file metadata from changing.
On Mon, Oct 30, 2017 at 11:36 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > On Mon, 2017-10-30 at 10:53 +0000, Matthew Garrett wrote: >> 2) It should be possible to write a policy that allows these files to >> be arbitrarily modified, including being deleted > > File deletion is not the problem, but if we allow the file metadata to > change, then the file verification will fail. That seems reasonable? The policy may not be appraising all files, in which case having verification fail isn't a problem. >> I'd been interpreting "immutable" in this case to mean "the kernel >> will never replace the signature with an hmac" rather than "the file >> and protected information cannot be modified". If you think the latter >> is necessary then I think we need two separate signature types and to >> handle the two separately. > > EVM, up to now, is reactive to file data and meta-data changes, > replacing the file signature with an HMAC. For the new file signature > to be immutable it needs to prevent security.evm from changing, which > this patch currently does not do. It prevents the kernel from modifying security.evm, but doesn't prevent userspace from doing so. But if userspace does so, verification will fail.
On Mon, 2017-10-30 at 11:51 +0000, Matthew Garrett wrote: > On Mon, Oct 30, 2017 at 11:36 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > On Mon, 2017-10-30 at 10:53 +0000, Matthew Garrett wrote: > >> 2) It should be possible to write a policy that allows these files to > >> be arbitrarily modified, including being deleted > > > > File deletion is not the problem, but if we allow the file metadata to > > change, then the file verification will fail. > > That seems reasonable? The policy may not be appraising all files, in > which case having verification fail isn't a problem. > > >> I'd been interpreting "immutable" in this case to mean "the kernel > >> will never replace the signature with an hmac" rather than "the file > >> and protected information cannot be modified". If you think the latter > >> is necessary then I think we need two separate signature types and to > >> handle the two separately. > > > > EVM, up to now, is reactive to file data and meta-data changes, > > replacing the file signature with an HMAC. For the new file signature > > to be immutable it needs to prevent security.evm from changing, which > > this patch currently does not do. > > It prevents the kernel from modifying security.evm, but doesn't > prevent userspace from doing so. But if userspace does so, > verification will fail. No, the current code does not prevent the signature from being replaced with an HMAC. Subsequent EVM verification will succeed based on an HMAC.
On Fri, 2017-10-27 at 10:27 +0000, Dmitry Kasatkin wrote: > > @@ -345,7 +350,8 @@ int evm_inode_setxattr(struct dentry *dentry, const > > char *xattr_name, > > if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) { > > if (!xattr_value_len) > > return -EINVAL; > > - if (xattr_data->type != EVM_IMA_XATTR_DIGSIG) > > + if (xattr_data->type != EVM_IMA_XATTR_DIGSIG && > > + xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) > > return -EPERM; > > } > > return evm_protect_xattr(dentry, xattr_name, xattr_value, @@ - > > 432,6 +438,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr > > *attr) > > return 0; > > evm_status = evm_verify_current_integrity(dentry); > > if ((evm_status == INTEGRITY_PASS) || > > + (evm_status == INTEGRITY_PASS_IMMUTABLE) || > > (evm_status == INTEGRITY_NOXATTRS)) > > return 0; > > Something is wrong here? > When integrity verification pass, this code WILL ALLOW to change attribute. > But it is not possible... next time integrity verification will fail? > Or I miss something? Right, it will allow the file metadata change. The bigger problem is that evm_inode_post_setattr() will replace the signature with an HMAC. > > > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
On Fri, 2017-10-27 at 10:41 +0000, Dmitry Kasatkin wrote: > > @@ -345,7 +350,8 @@ int evm_inode_setxattr(struct dentry *dentry, const > > char *xattr_name, > > if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) { > > if (!xattr_value_len) > > return -EINVAL; > > - if (xattr_data->type != EVM_IMA_XATTR_DIGSIG) > > + if (xattr_data->type != EVM_IMA_XATTR_DIGSIG && > > + xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) > > return -EPERM; > > } > > Also I have an impression that evm_protect_xattr will allow to set > security.ima for example, > And it will cause to try to re-calculate hmac over immutable > signature... Right, it will allow evm_inode_post_setxattr() to replace the new file signature with an HMAC. > > > return evm_protect_xattr(dentry, xattr_name, xattr_value, @@ -
On Mon, Oct 30, 2017 at 12:38 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > On Fri, 2017-10-27 at 10:41 +0000, Dmitry Kasatkin wrote: > >> > @@ -345,7 +350,8 @@ int evm_inode_setxattr(struct dentry *dentry, const >> > char *xattr_name, >> > if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) { >> > if (!xattr_value_len) >> > return -EINVAL; >> > - if (xattr_data->type != EVM_IMA_XATTR_DIGSIG) >> > + if (xattr_data->type != EVM_IMA_XATTR_DIGSIG && >> > + xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) >> > return -EPERM; >> > } >> >> Also I have an impression that evm_protect_xattr will allow to set >> security.ima for example, >> And it will cause to try to re-calculate hmac over immutable >> signature... > > Right, it will allow evm_inode_post_setxattr() to replace the new file > signature with an HMAC. evm_inode_setxattr() will call evm_protect_xattr(), which will call evm_verify_xattr(). This will return INTEGRITY_PASS_IMMUTABLE, which will result in evm_protect_xattr() returning -EPERM, so we never get to inode_post_setxattr().
On Mon, Oct 30, 2017 at 12:30 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > On Fri, 2017-10-27 at 10:27 +0000, Dmitry Kasatkin wrote: > >> > @@ -345,7 +350,8 @@ int evm_inode_setxattr(struct dentry *dentry, const >> > char *xattr_name, >> > if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) { >> > if (!xattr_value_len) >> > return -EINVAL; >> > - if (xattr_data->type != EVM_IMA_XATTR_DIGSIG) >> > + if (xattr_data->type != EVM_IMA_XATTR_DIGSIG && >> > + xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) >> > return -EPERM; >> > } >> > return evm_protect_xattr(dentry, xattr_name, xattr_value, @@ - >> > 432,6 +438,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr >> > *attr) >> > return 0; >> > evm_status = evm_verify_current_integrity(dentry); >> > if ((evm_status == INTEGRITY_PASS) || >> > + (evm_status == INTEGRITY_PASS_IMMUTABLE) || >> > (evm_status == INTEGRITY_NOXATTRS)) >> > return 0; >> >> Something is wrong here? >> When integrity verification pass, this code WILL ALLOW to change attribute. >> But it is not possible... next time integrity verification will fail? >> Or I miss something? > > Right, it will allow the file metadata change. The bigger problem is > that evm_inode_post_setattr() will replace the signature with an HMAC. It will allow the metadata update as long as it's not security.evm or one of the evm protected xattrs, which means that evm_inode_post_setxattr() will return rather than calling evm_update_evmxattr().
On Mon, 2017-10-30 at 13:21 +0000, Matthew Garrett wrote: > On Mon, Oct 30, 2017 at 12:30 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > On Fri, 2017-10-27 at 10:27 +0000, Dmitry Kasatkin wrote: > > > >> > @@ -345,7 +350,8 @@ int evm_inode_setxattr(struct dentry *dentry, const > >> > char *xattr_name, > >> > if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) { > >> > if (!xattr_value_len) > >> > return -EINVAL; > >> > - if (xattr_data->type != EVM_IMA_XATTR_DIGSIG) > >> > + if (xattr_data->type != EVM_IMA_XATTR_DIGSIG && > >> > + xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) > >> > return -EPERM; > >> > } > >> > return evm_protect_xattr(dentry, xattr_name, xattr_value, @@ - > >> > 432,6 +438,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr > >> > *attr) > >> > return 0; > >> > evm_status = evm_verify_current_integrity(dentry); > >> > if ((evm_status == INTEGRITY_PASS) || > >> > + (evm_status == INTEGRITY_PASS_IMMUTABLE) || > >> > (evm_status == INTEGRITY_NOXATTRS)) > >> > return 0; > >> > >> Something is wrong here? > >> When integrity verification pass, this code WILL ALLOW to change attribute. > >> But it is not possible... next time integrity verification will fail? > >> Or I miss something? > > > > Right, it will allow the file metadata change. The bigger problem is > > that evm_inode_post_setattr() will replace the signature with an HMAC. > > It will allow the metadata update as long as it's not security.evm or > one of the evm protected xattrs, which means that > evm_inode_post_setxattr() will return rather than calling > evm_update_evmxattr(). We're discussing setattr, not setxattr here. Any file metadata change (eg. chmod, chown, etc) will result in the file signature being converted to an HMAC.
On Mon, Oct 30, 2017 at 1:58 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > We're discussing setattr, not setxattr here. Any file metadata change > (eg. chmod, chown, etc) will result in the file signature being > converted to an HMAC. Gah. Sorry, you're completely correct. Ok, I'll think about the best way to approach this.
On Mon, 2017-10-30 at 13:17 +0000, Matthew Garrett wrote: > On Mon, Oct 30, 2017 at 12:38 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > On Fri, 2017-10-27 at 10:41 +0000, Dmitry Kasatkin wrote: > > > >> > @@ -345,7 +350,8 @@ int evm_inode_setxattr(struct dentry *dentry, const > >> > char *xattr_name, > >> > if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) { > >> > if (!xattr_value_len) > >> > return -EINVAL; > >> > - if (xattr_data->type != EVM_IMA_XATTR_DIGSIG) > >> > + if (xattr_data->type != EVM_IMA_XATTR_DIGSIG && > >> > + xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) > >> > return -EPERM; > >> > } > >> > >> Also I have an impression that evm_protect_xattr will allow to set > >> security.ima for example, > >> And it will cause to try to re-calculate hmac over immutable > >> signature... > > > > Right, it will allow evm_inode_post_setxattr() to replace the new file > > signature with an HMAC. > > evm_inode_setxattr() will call evm_protect_xattr(), which will call > evm_verify_xattr(). This will return INTEGRITY_PASS_IMMUTABLE, which > will result in evm_protect_xattr() returning -EPERM, so we never get > to inode_post_setxattr(). Oh, I missed that.
On Mon, Oct 30, 2017 at 3:31 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Oh, I missed that.
No problem, I've confused myself enough with this!
Ok, thinking about this some more, I think I'm conflating two independent things here. Whether the extended attributes and file contents are modifiable is a matter of policy rather than something that should be tied to the signature format, so I think the approach that makes sense is to make the portable signatures immutable (as previously discussed) and then allow userland to define a policy that permits modification of the protected metadata. I'll split this up and retest. On Mon, Oct 30, 2017 at 8:36 AM, Matthew Garrett <mjg59@google.com> wrote: > On Mon, Oct 30, 2017 at 3:31 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: >> Oh, I missed that. > > No problem, I've confused myself enough with this!
On Wed, 2017-11-01 at 10:54 -0700, Matthew Garrett wrote: > Ok, thinking about this some more, I think I'm conflating two > independent things here. Whether the extended attributes and file > contents are modifiable is a matter of policy rather than something > that should be tied to the signature format, so I think the approach > that makes sense is to make the portable signatures immutable (as > previously discussed) So updating any file metadata, including security xattrs, included in the signature will not cause the security.evm to change. The file metadata will be permited to change. > and then allow userland to define a policy that > permits modification of the protected metadata. I'll split this up and > retest. This subsequent patch will define a method for preventing the file metadata included in the signature from changing as well. Where is this policy? Do you mean a build or runtime configuration?
diff --git a/include/linux/integrity.h b/include/linux/integrity.h index c2d6082a1a4c..858d3f4a2241 100644 --- a/include/linux/integrity.h +++ b/include/linux/integrity.h @@ -14,6 +14,7 @@ enum integrity_status { INTEGRITY_PASS = 0, + INTEGRITY_PASS_IMMUTABLE, INTEGRITY_FAIL, INTEGRITY_NOLABEL, INTEGRITY_NOXATTRS, diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h index f5f12727771a..2ff02459fcfd 100644 --- a/security/integrity/evm/evm.h +++ b/security/integrity/evm/evm.h @@ -48,7 +48,7 @@ int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name, 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, char type, char *digest); int evm_init_hmac(struct inode *inode, const struct xattr *xattr, char *hmac_val); int evm_init_secfs(void); diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 1d32cd20009a..8855722529d4 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -138,7 +138,7 @@ static struct shash_desc *init_desc(char type) * protection.) */ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode, - char *digest) + char type, char *digest) { struct h_misc { unsigned long ino; @@ -149,8 +149,13 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode, } hmac_misc; memset(&hmac_misc, 0, sizeof(hmac_misc)); - hmac_misc.ino = inode->i_ino; - hmac_misc.generation = inode->i_generation; + /* Don't include the inode or generation number in portable + * signatures + */ + if (type != EVM_XATTR_PORTABLE_DIGSIG) { + hmac_misc.ino = inode->i_ino; + hmac_misc.generation = inode->i_generation; + } /* The hmac uid and gid must be encoded in the initial user * namespace (not the filesystems user namespace) as encoding * them in the filesystems user namespace allows an attack @@ -163,7 +168,8 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode, 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)); - if (evm_hmac_attrs & EVM_ATTR_FSUUID) + if ((evm_hmac_attrs & EVM_ATTR_FSUUID) && + type != EVM_XATTR_PORTABLE_DIGSIG) crypto_shash_update(desc, &inode->i_sb->s_uuid.b[0], sizeof(inode->i_sb->s_uuid)); crypto_shash_final(desc, digest); @@ -189,6 +195,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, char *xattr_value = NULL; int error; int size; + bool ima_present = false; if (!(inode->i_opflags & IOP_XATTR)) return -EOPNOTSUPP; @@ -199,11 +206,18 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, error = -ENODATA; for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) { + bool is_ima = false; + + if (strcmp(*xattrname, XATTR_NAME_IMA) == 0) + is_ima = true; + if ((req_xattr_name && req_xattr_value) && !strcmp(*xattrname, req_xattr_name)) { error = 0; crypto_shash_update(desc, (const u8 *)req_xattr_value, req_xattr_value_len); + if (is_ima) + ima_present = true; continue; } size = vfs_getxattr_alloc(dentry, *xattrname, @@ -218,9 +232,14 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, error = 0; xattr_size = size; crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size); + if (is_ima) + ima_present = true; } - hmac_add_misc(desc, inode, digest); + hmac_add_misc(desc, inode, type, digest); + /* Portable EVM signatures must include an IMA hash */ + if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present) + return -EPERM; out: kfree(xattr_value); kfree(desc); @@ -232,15 +251,15 @@ int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name, char *digest) { return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value, - req_xattr_value_len, EVM_XATTR_HMAC, digest); + req_xattr_value_len, EVM_XATTR_HMAC, 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) + char type, 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, type, digest); } /* @@ -280,7 +299,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr, } crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len); - hmac_add_misc(desc, inode, hmac_val); + hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val); kfree(desc); return 0; } diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 063d38aef64e..7ab633eab576 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -120,7 +120,8 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, enum integrity_status evm_status = INTEGRITY_PASS; int rc, xattr_len; - if (iint && iint->evm_status == INTEGRITY_PASS) + if (iint && (iint->evm_status == INTEGRITY_PASS || + iint->evm_status == INTEGRITY_PASS_IMMUTABLE)) return iint->evm_status; /* if status is not PASS, try to check again - against -ENOMEM */ @@ -161,22 +162,25 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, rc = -EINVAL; break; case EVM_IMA_XATTR_DIGSIG: + case EVM_XATTR_PORTABLE_DIGSIG: rc = evm_calc_hash(dentry, xattr_name, xattr_value, - xattr_value_len, calc.digest); + xattr_value_len, xattr_data->type, + calc.digest); if (rc) break; rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM, (const char *)xattr_data, xattr_len, calc.digest, sizeof(calc.digest)); if (!rc) { - /* Replace RSA with HMAC if not mounted readonly and - * not immutable - */ - if (!IS_RDONLY(d_backing_inode(dentry)) && - !IS_IMMUTABLE(d_backing_inode(dentry))) + if (xattr_data->type == EVM_XATTR_PORTABLE_DIGSIG) { + iint->flags |= EVM_IMMUTABLE_DIGSIG; + evm_status = INTEGRITY_PASS_IMMUTABLE; + } else if (!IS_RDONLY(d_backing_inode(dentry)) && + !IS_IMMUTABLE(d_backing_inode(dentry))) { evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len); + } } break; default: @@ -292,6 +296,7 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name, return 0; evm_status = evm_verify_current_integrity(dentry); if ((evm_status == INTEGRITY_PASS) || + (evm_status == INTEGRITY_PASS_IMMUTABLE) || (evm_status == INTEGRITY_NOXATTRS)) return 0; goto out; @@ -345,7 +350,8 @@ int evm_inode_setxattr(struct dentry *dentry, const char *xattr_name, if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) { if (!xattr_value_len) return -EINVAL; - if (xattr_data->type != EVM_IMA_XATTR_DIGSIG) + if (xattr_data->type != EVM_IMA_XATTR_DIGSIG && + xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) return -EPERM; } return evm_protect_xattr(dentry, xattr_name, xattr_value, @@ -432,6 +438,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr) return 0; evm_status = evm_verify_current_integrity(dentry); if ((evm_status == INTEGRITY_PASS) || + (evm_status == INTEGRITY_PASS_IMMUTABLE) || (evm_status == INTEGRITY_NOXATTRS)) return 0; integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 809ba70fbbbf..6e277c5c7829 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -229,7 +229,9 @@ int ima_appraise_measurement(enum ima_hooks func, } status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint); - if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) { + if ((status != INTEGRITY_PASS) && + (status != INTEGRITY_PASS_IMMUTABLE) && + (status != INTEGRITY_UNKNOWN)) { if ((status == INTEGRITY_NOLABEL) || (status == INTEGRITY_NOXATTRS)) cause = "missing-HMAC"; @@ -317,7 +319,7 @@ void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file) int rc = 0; /* do not collect and update hash for digital signatures */ - if (iint->flags & IMA_DIGSIG) + if (iint->flags & IMA_DIGSIG || iint->flags & EVM_IMMUTABLE_DIGSIG) return; rc = ima_collect_measurement(iint, file, NULL, 0, ima_hash_algo); diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index a53e7e4ab06c..cbc7de33fac7 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -33,6 +33,7 @@ #define IMA_DIGSIG_REQUIRED 0x02000000 #define IMA_PERMIT_DIRECTIO 0x04000000 #define IMA_NEW_FILE 0x08000000 +#define EVM_IMMUTABLE_DIGSIG 0x10000000 #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ IMA_APPRAISE_SUBMASK) @@ -58,6 +59,7 @@ enum evm_ima_xattr_type { EVM_XATTR_HMAC, EVM_IMA_XATTR_DIGSIG, IMA_XATTR_DIGEST_NG, + EVM_XATTR_PORTABLE_DIGSIG, IMA_XATTR_LAST };