| Message ID | 20210726163700.2092768-7-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | integrity: Introduce DIGLIM | expand |
Em Mon, 26 Jul 2021 18:36:54 +0200 Roberto Sassu <roberto.sassu@huawei.com> escreveu: > Introduce <securityfs>/integrity/diglim/digest_list_add, which can be used > to upload a digest list and add the digests to the hash table; passed data > are interpreted as file path if the first byte is / or as the digest list > itself otherwise. ima_measure_critical_data() is called to calculate the > digest of the digest list and eventually, if an appropriate rule is set in > the IMA policy, to measure it. > > Also introduce <securityfs>/integrity/diglim/digest_list_del, which can be > used to upload a digest list and delete the digests from the hash table; > data are interpreted in the same way as described for digest_list_add. LGTM. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > .../security/diglim/implementation.rst | 9 + > MAINTAINERS | 1 + > include/linux/kernel_read_file.h | 1 + > security/integrity/diglim/Makefile | 2 +- > security/integrity/diglim/diglim.h | 2 + > security/integrity/diglim/fs.c | 239 ++++++++++++++++++ > security/integrity/integrity.h | 4 + > 7 files changed, 257 insertions(+), 1 deletion(-) > create mode 100644 security/integrity/diglim/fs.c > > diff --git a/Documentation/security/diglim/implementation.rst b/Documentation/security/diglim/implementation.rst > index 9d679567a037..fc0cd8810a80 100644 > --- a/Documentation/security/diglim/implementation.rst > +++ b/Documentation/security/diglim/implementation.rst > @@ -244,3 +244,12 @@ back when one of the steps fails. > > #. digest_list_parse() deletes the struct digest_list_item on unsuccessful > add or successful delete. > + > + > +Interfaces > +---------- > + > +This section introduces the interfaces in > +``<securityfs>/integrity/diglim`` necessary to interact with DIGLIM. > + > +.. kernel-doc:: security/integrity/diglim/fs.c > diff --git a/MAINTAINERS b/MAINTAINERS > index 77c3613c600a..0672128fae7f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5464,6 +5464,7 @@ F: Documentation/security/diglim/introduction.rst > F: include/linux/diglim.h > F: include/uapi/linux/diglim.h > F: security/integrity/diglim/diglim.h > +F: security/integrity/diglim/fs.c > F: security/integrity/diglim/methods.c > F: security/integrity/diglim/parser.c > > diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h > index 575ffa1031d3..636ecdfdc616 100644 > --- a/include/linux/kernel_read_file.h > +++ b/include/linux/kernel_read_file.h > @@ -14,6 +14,7 @@ > id(KEXEC_INITRAMFS, kexec-initramfs) \ > id(POLICY, security-policy) \ > id(X509_CERTIFICATE, x509-certificate) \ > + id(DIGEST_LIST, digest-list) \ > id(MAX_ID, ) > > #define __fid_enumify(ENUM, dummy) READING_ ## ENUM, > diff --git a/security/integrity/diglim/Makefile b/security/integrity/diglim/Makefile > index 34e4e154fff3..ac345afdf5dd 100644 > --- a/security/integrity/diglim/Makefile > +++ b/security/integrity/diglim/Makefile > @@ -5,4 +5,4 @@ > > obj-$(CONFIG_DIGLIM) += diglim.o > > -diglim-y := methods.o parser.o > +diglim-y := methods.o parser.o fs.o > diff --git a/security/integrity/diglim/diglim.h b/security/integrity/diglim/diglim.h > index 3adc218a0325..819703175eda 100644 > --- a/security/integrity/diglim/diglim.h > +++ b/security/integrity/diglim/diglim.h > @@ -22,6 +22,8 @@ > #include <linux/hash_info.h> > #include <linux/diglim.h> > > +#include "../integrity.h" > + > #define MAX_DIGEST_SIZE 64 > #define HASH_BITS 10 > #define DIGLIM_HTABLE_SIZE (1 << HASH_BITS) > diff --git a/security/integrity/diglim/fs.c b/security/integrity/diglim/fs.c > new file mode 100644 > index 000000000000..07a0d75a0e33 > --- /dev/null > +++ b/security/integrity/diglim/fs.c > @@ -0,0 +1,239 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2005,2006,2007,2008 IBM Corporation > + * Copyright (C) 2017-2021 Huawei Technologies Duesseldorf GmbH > + * > + * Author: Roberto Sassu <roberto.sassu@huawei.com> > + * > + * Functions for the interfaces exposed in securityfs. > + */ > + > +#include <linux/fcntl.h> > +#include <linux/kernel_read_file.h> > +#include <linux/slab.h> > +#include <linux/init.h> > +#include <linux/seq_file.h> > +#include <linux/rculist.h> > +#include <linux/rcupdate.h> > +#include <linux/parser.h> > +#include <linux/vmalloc.h> > +#include <linux/namei.h> > +#include <linux/ima.h> > + > +#include "diglim.h" > + > +#define MAX_DIGEST_LIST_SIZE (64 * 1024 * 1024 - 1) > + > +static struct dentry *diglim_dir; > +/** > + * DOC: digest_list_add > + * > + * digest_list_add can be used to upload a digest list and add the digests > + * to the hash table; passed data are interpreted as file path if the first > + * byte is ``/`` or as the digest list itself otherwise. > + * > + * ima_measure_critical_data() is called to calculate the digest of the > + * digest list and eventually, if an appropriate rule is set in the IMA > + * policy, to measure it. > + */ > +static struct dentry *digest_list_add_dentry; > +/** > + * DOC: digest_list_del > + * > + * digest_list_del can be used to upload a digest list and delete the > + * digests from the hash table; data are interpreted in the same way as > + * described for digest_list_add. > + */ > +static struct dentry *digest_list_del_dentry; > +char digest_label[NAME_MAX + 1]; > + > +/* > + * digest_list_read: read and parse the digest list from the path > + */ > +static ssize_t digest_list_read(char *path, enum ops op) > +{ > + void *data = NULL; > + char *datap; > + size_t size; > + u8 actions = 0; > + struct file *file; > + char event_name[NAME_MAX + 9 + 1]; > + u8 digest[IMA_MAX_DIGEST_SIZE] = { 0 }; > + enum hash_algo algo; > + int rc, pathlen = strlen(path); > + > + /* Remove \n. */ > + datap = path; > + strsep(&datap, "\n"); > + > + file = filp_open(path, O_RDONLY, 0); > + if (IS_ERR(file)) { > + pr_err("unable to open file: %s (%ld)", path, PTR_ERR(file)); > + return PTR_ERR(file); > + } > + > + rc = kernel_read_file(file, 0, &data, INT_MAX, NULL, > + READING_DIGEST_LIST); > + if (rc < 0) { > + pr_err("unable to read file: %s (%d)", path, rc); > + goto out; > + } > + > + size = rc; > + > + snprintf(event_name, sizeof(event_name), "%s_file_%s", > + op == DIGEST_LIST_ADD ? "add" : "del", > + file_dentry(file)->d_name.name); > + > + rc = ima_measure_critical_data("diglim", event_name, data, size, false, > + digest, sizeof(digest)); > + if (rc < 0 && rc != -EEXIST) > + goto out_vfree; > + > + algo = ima_get_current_hash_algo(); > + > + if (!rc || rc == -EEXIST) > + actions |= (1 << COMPACT_ACTION_IMA_MEASURED); > + > + rc = digest_list_parse(size, data, op, actions, digest, algo, ""); > + if (rc < 0) > + pr_err("unable to upload digest list %s (%d)\n", path, rc); > +out_vfree: > + vfree(data); > +out: > + fput(file); > + > + if (rc < 0) > + return rc; > + > + return pathlen; > +} > + > +/* > + * digest_list_write: write the digest list path or the digest list itself > + */ > +static ssize_t digest_list_write(struct file *file, const char __user *buf, > + size_t datalen, loff_t *ppos) > +{ > + char *data; > + ssize_t result; > + enum ops op = DIGEST_LIST_ADD; > + struct dentry *dentry = file_dentry(file); > + u8 digest[IMA_MAX_DIGEST_SIZE]; > + char event_name[NAME_MAX + 11 + 1]; > + enum hash_algo algo; > + u8 actions = 0; > + > + /* No partial writes. */ > + result = -EINVAL; > + if (*ppos != 0) > + goto out; > + > + result = -EFBIG; > + if (datalen > MAX_DIGEST_LIST_SIZE) > + goto out; > + > + data = memdup_user_nul(buf, datalen); > + if (IS_ERR(data)) { > + result = PTR_ERR(data); > + goto out; > + } > + > + if (dentry == digest_list_del_dentry) > + op = DIGEST_LIST_DEL; > + > + result = -EPERM; > + > + if (data[0] == '/') { > + result = digest_list_read(data, op); > + } else { > + snprintf(event_name, sizeof(event_name), "%s_buffer_%s", > + op == DIGEST_LIST_ADD ? "add" : "del", digest_label); > + > + result = ima_measure_critical_data("diglim", event_name, data, > + datalen, false, digest, > + sizeof(digest)); > + if (result < 0 && result != -EEXIST) { > + pr_err("failed to measure buffer digest (%ld)\n", > + result); > + goto out_kfree; > + } > + > + algo = ima_get_current_hash_algo(); > + > + if (!result || result == -EEXIST) > + actions |= (1 << COMPACT_ACTION_IMA_MEASURED); > + > + result = digest_list_parse(datalen, data, op, actions, digest, > + algo, ""); > + if (result != datalen) { > + pr_err("unable to upload generated digest list\n"); > + result = -EINVAL; > + } > + } > +out_kfree: > + kfree(data); > +out: > + return result; > +} > + > +static unsigned long flags; > + > +/* > + * digest_list_open: sequentialize access to the add/del files > + */ > +static int digest_list_open(struct inode *inode, struct file *filp) > +{ > + if ((filp->f_flags & O_ACCMODE) != O_WRONLY) > + return -EACCES; > + > + if (test_and_set_bit(0, &flags)) > + return -EBUSY; > + > + return 0; > +} > + > +/* > + * digest_list_release - release the add/del files > + */ > +static int digest_list_release(struct inode *inode, struct file *file) > +{ > + clear_bit(0, &flags); > + return 0; > +} > + > +static const struct file_operations digest_list_upload_ops = { > + .open = digest_list_open, > + .write = digest_list_write, > + .read = seq_read, > + .release = digest_list_release, > + .llseek = generic_file_llseek, > +}; > + > +static int __init diglim_fs_init(void) > +{ > + diglim_dir = securityfs_create_dir("diglim", integrity_dir); > + if (IS_ERR(diglim_dir)) > + return -1; > + > + digest_list_add_dentry = securityfs_create_file("digest_list_add", 0200, > + diglim_dir, NULL, > + &digest_list_upload_ops); > + if (IS_ERR(digest_list_add_dentry)) > + goto out; > + > + digest_list_del_dentry = securityfs_create_file("digest_list_del", 0200, > + diglim_dir, NULL, > + &digest_list_upload_ops); > + if (IS_ERR(digest_list_del_dentry)) > + goto out; > + > + return 0; > +out: > + securityfs_remove(digest_list_del_dentry); > + securityfs_remove(digest_list_add_dentry); > + securityfs_remove(diglim_dir); > + return -1; > +} > + > +late_initcall(diglim_fs_init); > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index 547425c20e11..ac45e1599c2d 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -6,6 +6,9 @@ > * Mimi Zohar <zohar@us.ibm.com> > */ > > +#ifndef __INTEGRITY_H > +#define __INTEGRITY_H > + > #ifdef pr_fmt > #undef pr_fmt > #endif > @@ -283,3 +286,4 @@ static inline void __init add_to_platform_keyring(const char *source, > { > } > #endif > +#endif /*__INTEGRITY_H*/
Hi Roberto, On Mon, 2021-07-26 at 18:36 +0200, Roberto Sassu wrote: > /* > + * digest_list_read: read and parse the digest list from the path > + */ > +static ssize_t digest_list_read(char *path, enum ops op) > +{ > + void *data = NULL; > + char *datap; > + size_t size; > + u8 actions = 0; > + struct file *file; > + char event_name[NAME_MAX + 9 + 1]; > + u8 digest[IMA_MAX_DIGEST_SIZE] = { 0 }; > + enum hash_algo algo; > + int rc, pathlen = strlen(path); > + > + /* Remove \n. */ > + datap = path; > + strsep(&datap, "\n"); > + > + file = filp_open(path, O_RDONLY, 0); > + if (IS_ERR(file)) { > + pr_err("unable to open file: %s (%ld)", path, PTR_ERR(file)); > + return PTR_ERR(file); > + } > + > + rc = kernel_read_file(file, 0, &data, INT_MAX, NULL, > + READING_DIGEST_LIST); > + if (rc < 0) { > + pr_err("unable to read file: %s (%d)", path, rc); > + goto out; > + } > + > + size = rc; > + > + snprintf(event_name, sizeof(event_name), "%s_file_%s", > + op == DIGEST_LIST_ADD ? "add" : "del", > + file_dentry(file)->d_name.name); > + > + rc = ima_measure_critical_data("diglim", event_name, data, size, false, > + digest, sizeof(digest)); > + if (rc < 0 && rc != -EEXIST) > + goto out_vfree; The digest lists could easily be measured while reading the digest list file above in kernel_read_file(). What makes it "critical-data"? In the SELinux case, the in memory SELinux policy is being measured and re-measured to make sure it hasn't been modified. Is the digest list file data being measured more than once? I understand that with your changes to ima_measure_critical_data(), which are now in next-integrity-testing branch, allow IMA to calculate the file data hash. thanks, Mimi > + > + algo = ima_get_current_hash_algo(); > +
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Thursday, July 29, 2021 11:21 PM > Hi Roberto, > > On Mon, 2021-07-26 at 18:36 +0200, Roberto Sassu wrote: > > /* > > + * digest_list_read: read and parse the digest list from the path > > + */ > > +static ssize_t digest_list_read(char *path, enum ops op) > > +{ > > + void *data = NULL; > > + char *datap; > > + size_t size; > > + u8 actions = 0; > > + struct file *file; > > + char event_name[NAME_MAX + 9 + 1]; > > + u8 digest[IMA_MAX_DIGEST_SIZE] = { 0 }; > > + enum hash_algo algo; > > + int rc, pathlen = strlen(path); > > + > > + /* Remove \n. */ > > + datap = path; > > + strsep(&datap, "\n"); > > + > > + file = filp_open(path, O_RDONLY, 0); > > + if (IS_ERR(file)) { > > + pr_err("unable to open file: %s (%ld)", path, PTR_ERR(file)); > > + return PTR_ERR(file); > > + } > > + > > + rc = kernel_read_file(file, 0, &data, INT_MAX, NULL, > > + READING_DIGEST_LIST); > > + if (rc < 0) { > > + pr_err("unable to read file: %s (%d)", path, rc); > > + goto out; > > + } > > + > > + size = rc; > > + > > + snprintf(event_name, sizeof(event_name), "%s_file_%s", > > + op == DIGEST_LIST_ADD ? "add" : "del", > > + file_dentry(file)->d_name.name); > > + > > + rc = ima_measure_critical_data("diglim", event_name, data, size, false, > > + digest, sizeof(digest)); > > + if (rc < 0 && rc != -EEXIST) > > + goto out_vfree; > > The digest lists could easily be measured while reading the digest list > file above in kernel_read_file(). What makes it "critical-data"? In > the SELinux case, the in memory SELinux policy is being measured and > re-measured to make sure it hasn't been modified. Is the digest list > file data being measured more than once? Hi Mimi yes, the digest lists can be measured with kernel_read_file(). I didn't send the change yet, but I added a DIGEST_LIST_CHECK hook mapped to READING_DIGEST_LIST, so that digest lists can be easily measured or appraised. The point was that the digest of the digest list must be always calculated, as it is added to the hash table. Instead of duplicating the code, I preferred to use ima_measure_critical_data(). The advantage is also that, if the use case is to just measure digest lists, ima_measure_critical_data() could do both at the same time. Digest lists can be seen as "critical data" in the sense that they can affect the security decision on whether to grant access to a file or not, assuming that an appropriate rule is added in the IMA policy. > I understand that with your changes to ima_measure_critical_data(), > which are now in next-integrity-testing branch, allow IMA to calculate > the file data hash. Yes, correct. But actually there is another useful use case. If digest lists are not in the format supported by the kernel, the user space parser has to convert them before uploading them to the kernel. ima_measure_critical_data() would in this case measure the converted digest list (it is written directly, without sending the file path). It is easier to attest the result, instead of determining whether the user space parser produced the expected result (by checking the files it read). Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > thanks, > > Mimi > > > + > > + algo = ima_get_current_hash_algo(); > > + >
Hi Roberto, On Fri, 2021-07-30 at 07:16 +0000, Roberto Sassu wrote: > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > Sent: Thursday, July 29, 2021 11:21 PM > > > > On Mon, 2021-07-26 at 18:36 +0200, Roberto Sassu wrote: > > > /* > > > + * digest_list_read: read and parse the digest list from the path > > > + */ > > > +static ssize_t digest_list_read(char *path, enum ops op) > > > +{ > > > + void *data = NULL; > > > + char *datap; > > > + size_t size; > > > + u8 actions = 0; > > > + struct file *file; > > > + char event_name[NAME_MAX + 9 + 1]; > > > + u8 digest[IMA_MAX_DIGEST_SIZE] = { 0 }; > > > + enum hash_algo algo; > > > + int rc, pathlen = strlen(path); > > > + > > > + /* Remove \n. */ > > > + datap = path; > > > + strsep(&datap, "\n"); > > > + > > > + file = filp_open(path, O_RDONLY, 0); > > > + if (IS_ERR(file)) { > > > + pr_err("unable to open file: %s (%ld)", path, PTR_ERR(file)); > > > + return PTR_ERR(file); > > > + } > > > + > > > + rc = kernel_read_file(file, 0, &data, INT_MAX, NULL, > > > + READING_DIGEST_LIST); > > > + if (rc < 0) { > > > + pr_err("unable to read file: %s (%d)", path, rc); > > > + goto out; > > > + } > > > + > > > + size = rc; > > > + > > > + snprintf(event_name, sizeof(event_name), "%s_file_%s", > > > + op == DIGEST_LIST_ADD ? "add" : "del", > > > + file_dentry(file)->d_name.name); > > > + > > > + rc = ima_measure_critical_data("diglim", event_name, data, size, false, > > > + digest, sizeof(digest)); > > > + if (rc < 0 && rc != -EEXIST) > > > + goto out_vfree; > > > > The digest lists could easily be measured while reading the digest list > > file above in kernel_read_file(). What makes it "critical-data"? In > > the SELinux case, the in memory SELinux policy is being measured and > > re-measured to make sure it hasn't been modified. Is the digest list > > file data being measured more than once? > > Hi Mimi > > yes, the digest lists can be measured with kernel_read_file(). > I didn't send the change yet, but I added a DIGEST_LIST_CHECK > hook mapped to READING_DIGEST_LIST, so that digest lists > can be easily measured or appraised. > > The point was that the digest of the digest list must be always > calculated, as it is added to the hash table. Instead of duplicating > the code, I preferred to use ima_measure_critical_data(). > > The advantage is also that, if the use case is to just measure > digest lists, ima_measure_critical_data() could do both at the > same time. > > Digest lists can be seen as "critical data" in the sense that > they can affect the security decision on whether to grant > access to a file or not, assuming that an appropriate rule is > added in the IMA policy. Of course the integrity of files containing the digest lists is important, but that doesn't make them "critical data". If the integrity of these files is important, then the digest lists not only need to be measured, but they need to be signed and the resulting signature verified. Without signature verification, there is no basis on which to trust the digest lists data. Adding the kernel_read_file() "READING_DIGEST_LIST" support in IMA does not seem to be optional. IMA would then be calculating the digest list file hash twice, once in kernel_read_file() and then, again, in ima_measure_critical_data(). > > > I understand that with your changes to ima_measure_critical_data(), > > which are now in next-integrity-testing branch, allow IMA to calculate > > the file data hash. > > Yes, correct. But actually there is another useful use case. > If digest lists are not in the format supported by the kernel, > the user space parser has to convert them before uploading > them to the kernel. > > ima_measure_critical_data() would in this case measure > the converted digest list (it is written directly, without > sending the file path). It is easier to attest the result, > instead of determining whether the user space parser > produced the expected result (by checking the files it > read). The application to properly convert the digest list file data into the appropriate format would need to be trusted. I'm concerned that not requiring the converted data to be signed and the signature verified is introducing a new integrity gap. Perhaps between an LSM policy, limiting which files may be read by the application, and an IMA policy, requiring all files read by this application to be measured and the signature verified, this integrity gap could be averted. "critical data", in this context, should probably be used for verifying the in memory file digests and other state information haven't been compromised. thanks, Mimi
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Friday, July 30, 2021 2:40 PM > Hi Roberto, > > On Fri, 2021-07-30 at 07:16 +0000, Roberto Sassu wrote: > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > Sent: Thursday, July 29, 2021 11:21 PM > > > > > > On Mon, 2021-07-26 at 18:36 +0200, Roberto Sassu wrote: > > > > /* > > > > + * digest_list_read: read and parse the digest list from the path > > > > + */ > > > > +static ssize_t digest_list_read(char *path, enum ops op) > > > > +{ > > > > + void *data = NULL; > > > > + char *datap; > > > > + size_t size; > > > > + u8 actions = 0; > > > > + struct file *file; > > > > + char event_name[NAME_MAX + 9 + 1]; > > > > + u8 digest[IMA_MAX_DIGEST_SIZE] = { 0 }; > > > > + enum hash_algo algo; > > > > + int rc, pathlen = strlen(path); > > > > + > > > > + /* Remove \n. */ > > > > + datap = path; > > > > + strsep(&datap, "\n"); > > > > + > > > > + file = filp_open(path, O_RDONLY, 0); > > > > + if (IS_ERR(file)) { > > > > + pr_err("unable to open file: %s (%ld)", path, PTR_ERR(file)); > > > > + return PTR_ERR(file); > > > > + } > > > > + > > > > + rc = kernel_read_file(file, 0, &data, INT_MAX, NULL, > > > > + READING_DIGEST_LIST); > > > > + if (rc < 0) { > > > > + pr_err("unable to read file: %s (%d)", path, rc); > > > > + goto out; > > > > + } > > > > + > > > > + size = rc; > > > > + > > > > + snprintf(event_name, sizeof(event_name), "%s_file_%s", > > > > + op == DIGEST_LIST_ADD ? "add" : "del", > > > > + file_dentry(file)->d_name.name); > > > > + > > > > + rc = ima_measure_critical_data("diglim", event_name, data, size, > false, > > > > + digest, sizeof(digest)); > > > > + if (rc < 0 && rc != -EEXIST) > > > > + goto out_vfree; > > > > > > The digest lists could easily be measured while reading the digest list > > > file above in kernel_read_file(). What makes it "critical-data"? In > > > the SELinux case, the in memory SELinux policy is being measured and > > > re-measured to make sure it hasn't been modified. Is the digest list > > > file data being measured more than once? > > > > Hi Mimi > > > > yes, the digest lists can be measured with kernel_read_file(). > > I didn't send the change yet, but I added a DIGEST_LIST_CHECK > > hook mapped to READING_DIGEST_LIST, so that digest lists > > can be easily measured or appraised. > > > > The point was that the digest of the digest list must be always > > calculated, as it is added to the hash table. Instead of duplicating > > the code, I preferred to use ima_measure_critical_data(). > > > > The advantage is also that, if the use case is to just measure > > digest lists, ima_measure_critical_data() could do both at the > > same time. > > > > Digest lists can be seen as "critical data" in the sense that > > they can affect the security decision on whether to grant > > access to a file or not, assuming that an appropriate rule is > > added in the IMA policy. > > Of course the integrity of files containing the digest lists is > important, but that doesn't make them "critical data". If the > integrity of these files is important, then the digest lists not only > need to be measured, but they need to be signed and the resulting > signature verified. Without signature verification, there is no basis > on which to trust the digest lists data. The reason of storing the actions performed by IMA on the digest lists helps to determine for which purpose they can be used. If digest lists are used only for measurement purpose, it should be sufficient that digest lists are measured. The same applies for appraisal. > Adding the kernel_read_file() "READING_DIGEST_LIST" support in IMA does > not seem to be optional. IMA would then be calculating the digest list > file hash twice, once in kernel_read_file() and then, again, in > ima_measure_critical_data(). I didn't include also this part: I retrieve the integrity_iint_cache for the opened file descriptor and I get the flags from there. If the IMA_MEASURED flag is set, it is not necessary to call also ima_measure_critical_data(). > > > I understand that with your changes to ima_measure_critical_data(), > > > which are now in next-integrity-testing branch, allow IMA to calculate > > > the file data hash. > > > > Yes, correct. But actually there is another useful use case. > > If digest lists are not in the format supported by the kernel, > > the user space parser has to convert them before uploading > > them to the kernel. > > > > ima_measure_critical_data() would in this case measure > > the converted digest list (it is written directly, without > > sending the file path). It is easier to attest the result, > > instead of determining whether the user space parser > > produced the expected result (by checking the files it > > read). > > The application to properly convert the digest list file data into the > appropriate format would need to be trusted. I'm concerned that not > requiring the converted data to be signed and the signature verified is > introducing a new integrity gap. Perhaps between an LSM policy, > limiting which files may be read by the application, and an IMA policy, > requiring all files read by this application to be measured and the > signature verified, this integrity gap could be averted. It is the weakest point in the chain, yes. Relying on existing LSMs didn't seem to me a good idea, as: - a new policy must be installed - we must be sure that the policy is really enforced - we need to support different LSMs (SELinux for Fedora, Apparmor for SUSE) - there might be no LSM we can rely on For these reasons, I developed a new LSM. Its purpose is to identify the user space parser and for each file it opens, ensure that the file has been measured or appraised by IMA. If one of these actions are missing, it will not be set in the digest list the user space parser uploads to the kernel (which means that IMA will ignore the digest list for that specific action). > "critical data", in this context, should probably be used for verifying > the in memory file digests and other state information haven't been > compromised. Actually, this is what we are doing currently. To keep the implementation simple, once the file or the buffer are uploaded to the kernel, they will not be modified, just accessed through the indexes. I could send the second part of the patch set, so that it becomes more clear how digest lists are used by IMA and how the integrity gap is filled. Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > thanks, > > Mimi
Hi Roberto, On Fri, 2021-07-30 at 13:16 +0000, Roberto Sassu wrote: > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > Sent: Friday, July 30, 2021 2:40 PM > > "critical data", in this context, should probably be used for verifying > > the in memory file digests and other state information haven't been > > compromised. > > Actually, this is what we are doing currently. To keep the > implementation simple, once the file or the buffer are uploaded > to the kernel, they will not be modified, just accessed through > the indexes. My main concern about digest lists is their integrity, from loading the digest lists to their being stored in memory. A while back, there was some work on defining a write once memory allocator. I don't recall whatever happened to it. This would be a perfect usecase for that memory allocator. thanks, Mimi
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Friday, July 30, 2021 4:03 PM > Hi Roberto, > > On Fri, 2021-07-30 at 13:16 +0000, Roberto Sassu wrote: > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > Sent: Friday, July 30, 2021 2:40 PM > > > > "critical data", in this context, should probably be used for verifying > > > the in memory file digests and other state information haven't been > > > compromised. > > > > Actually, this is what we are doing currently. To keep the > > implementation simple, once the file or the buffer are uploaded > > to the kernel, they will not be modified, just accessed through > > the indexes. > > My main concern about digest lists is their integrity, from loading the > digest lists to their being stored in memory. A while back, there was > some work on defining a write once memory allocator. I don't recall > whatever happened to it. This would be a perfect usecase for that > memory allocator. Adding Igor in CC. Regarding loading, everything uploaded to the kernel is carefully evaluated. This should not be a concern. Regarding making them read-only, probably if you can subvert digest lists you can also remove the read-only protection (unless you use an hypervisor). Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > thanks, > > Mimi
> From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > Sent: Friday, July 30, 2021 4:25 PM > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > Sent: Friday, July 30, 2021 4:03 PM > > Hi Roberto, > > > > On Fri, 2021-07-30 at 13:16 +0000, Roberto Sassu wrote: > > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > > Sent: Friday, July 30, 2021 2:40 PM > > > > > > "critical data", in this context, should probably be used for verifying > > > > the in memory file digests and other state information haven't been > > > > compromised. > > > > > > Actually, this is what we are doing currently. To keep the > > > implementation simple, once the file or the buffer are uploaded > > > to the kernel, they will not be modified, just accessed through > > > the indexes. > > > > My main concern about digest lists is their integrity, from loading the > > digest lists to their being stored in memory. A while back, there was > > some work on defining a write once memory allocator. I don't recall > > whatever happened to it. This would be a perfect usecase for that > > memory allocator. > > Adding Igor in CC. > > Regarding loading, everything uploaded to the kernel is carefully > evaluated. This should not be a concern. Regarding making them > read-only, probably if you can subvert digest lists you can also > remove the read-only protection (unless you use an hypervisor). I briefly talked with Igor. He also agreed with that, and added that it could make it more difficult for an attacker to also disable the protection. However, he is not planning to submit an update soon, so I wouldn't consider this an option for now. Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > Thanks > > Roberto > > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 > Managing Director: Li Peng, Li Jian, Shi Yanli > > > thanks, > > > > Mimi
Hi Roberto, On Fri, 2021-07-30 at 13:16 +0000, Roberto Sassu wrote: > The reason of storing the actions performed by IMA on the > digest lists helps to determine for which purpose they can be > used. If digest lists are used only for measurement purpose, > it should be sufficient that digest lists are measured. The > same applies for appraisal. Is that assumption correct? How would you know if the digests lists are only being used one way and not the other. For example, assuming that the digest lists are stored on protected media, the digest lists could be measured, but would not necessarily be appraised. > > Adding the kernel_read_file() "READING_DIGEST_LIST" support in IMA does > > not seem to be optional. IMA would then be calculating the digest list > > file hash twice, once in kernel_read_file() and then, again, in > > ima_measure_critical_data(). > > I didn't include also this part: I retrieve the integrity_iint_cache for > the opened file descriptor and I get the flags from there. If the > IMA_MEASURED flag is set, it is not necessary to call also > ima_measure_critical_data(). Right, assuming the file is in policy, the digest would already be stored in the iint cache. > > > > I understand that with your changes to ima_measure_critical_data(), > > > > which are now in next-integrity-testing branch, allow IMA to calculate > > > > the file data hash. > > > > > > Yes, correct. But actually there is another useful use case. > > > If digest lists are not in the format supported by the kernel, > > > the user space parser has to convert them before uploading > > > them to the kernel. > > > > > > ima_measure_critical_data() would in this case measure > > > the converted digest list (it is written directly, without > > > sending the file path). It is easier to attest the result, > > > instead of determining whether the user space parser > > > produced the expected result (by checking the files it > > > read). > > > > The application to properly convert the digest list file data into the > > appropriate format would need to be trusted. I'm concerned that not > > requiring the converted data to be signed and the signature verified is > > introducing a new integrity gap. Perhaps between an LSM policy, > > limiting which files may be read by the application, and an IMA policy, > > requiring all files read by this application to be measured and the > > signature verified, this integrity gap could be averted. > > It is the weakest point in the chain, yes. Relying on existing LSMs > didn't seem to me a good idea, as: > - a new policy must be installed > - we must be sure that the policy is really enforced > - we need to support different LSMs (SELinux for Fedora, > Apparmor for SUSE) > - there might be no LSM we can rely on > > For these reasons, I developed a new LSM. Its purpose is to > identify the user space parser and for each file it opens, ensure > that the file has been measured or appraised by IMA. If one of > these actions are missing, it will not be set in the digest list the > user space parser uploads to the kernel (which means that IMA > will ignore the digest list for that specific action). Properly identifying (all) user space parser(s) would be critical. It would be simpler and safer to require the converted data be signed. thanks, Mimi
On Mon, 2021-08-02 at 08:14 +0000, Roberto Sassu wrote: > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > Sent: Friday, July 30, 2021 4:25 PM > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > Sent: Friday, July 30, 2021 4:03 PM > > > Hi Roberto, > > > > > > On Fri, 2021-07-30 at 13:16 +0000, Roberto Sassu wrote: > > > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > > > Sent: Friday, July 30, 2021 2:40 PM > > > > > > > > "critical data", in this context, should probably be used for verifying > > > > > the in memory file digests and other state information haven't been > > > > > compromised. > > > > > > > > Actually, this is what we are doing currently. To keep the > > > > implementation simple, once the file or the buffer are uploaded > > > > to the kernel, they will not be modified, just accessed through > > > > the indexes. > > > > > > My main concern about digest lists is their integrity, from loading the > > > digest lists to their being stored in memory. A while back, there was > > > some work on defining a write once memory allocator. I don't recall > > > whatever happened to it. This would be a perfect usecase for that > > > memory allocator. > > > > Adding Igor in CC. > > > > Regarding loading, everything uploaded to the kernel is carefully > > evaluated. This should not be a concern. Regarding making them > > read-only, probably if you can subvert digest lists you can also > > remove the read-only protection (unless you use an hypervisor). > > I briefly talked with Igor. He also agreed with that, and added that > it could make it more difficult for an attacker to also disable the > protection. However, he is not planning to submit an update soon, > so I wouldn't consider this an option for now. Hi Roberto, Greg, As long as others understand and agree to the risk, the IMA details can be worked out. thanks, Mimi
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Monday, August 2, 2021 4:42 PM > Hi Roberto, > > On Fri, 2021-07-30 at 13:16 +0000, Roberto Sassu wrote: > > > The reason of storing the actions performed by IMA on the > > digest lists helps to determine for which purpose they can be > > used. If digest lists are used only for measurement purpose, > > it should be sufficient that digest lists are measured. The > > same applies for appraisal. > > Is that assumption correct? How would you know if the digests lists > are only being used one way and not the other. For example, assuming > that the digest lists are stored on protected media, the digest lists > could be measured, but would not necessarily be appraised. Hi Mimi the actions performed by IMA on the digest lists are recorded in the digest_list_item structure. These can be retrieved when IMA calls diglim_digest_get_info() (actually it is the OR of the actions for the digest lists that contain the digest passed as a query). At the moment, DIGLIM can only know whether a digest list has been measured or not (with the return value of ima_measure_critical_data()). In the next patch set, I add the changes to get the actions from the integrity_iint_cache(). > > > Adding the kernel_read_file() "READING_DIGEST_LIST" support in IMA > does > > > not seem to be optional. IMA would then be calculating the digest list > > > file hash twice, once in kernel_read_file() and then, again, in > > > ima_measure_critical_data(). > > > > I didn't include also this part: I retrieve the integrity_iint_cache for > > the opened file descriptor and I get the flags from there. If the > > IMA_MEASURED flag is set, it is not necessary to call also > > ima_measure_critical_data(). > > Right, assuming the file is in policy, the digest would already be > stored in the iint cache. > > > > > > I understand that with your changes to ima_measure_critical_data(), > > > > > which are now in next-integrity-testing branch, allow IMA to calculate > > > > > the file data hash. > > > > > > > > Yes, correct. But actually there is another useful use case. > > > > If digest lists are not in the format supported by the kernel, > > > > the user space parser has to convert them before uploading > > > > them to the kernel. > > > > > > > > ima_measure_critical_data() would in this case measure > > > > the converted digest list (it is written directly, without > > > > sending the file path). It is easier to attest the result, > > > > instead of determining whether the user space parser > > > > produced the expected result (by checking the files it > > > > read). > > > > > > The application to properly convert the digest list file data into the > > > appropriate format would need to be trusted. I'm concerned that not > > > requiring the converted data to be signed and the signature verified is > > > introducing a new integrity gap. Perhaps between an LSM policy, > > > limiting which files may be read by the application, and an IMA policy, > > > requiring all files read by this application to be measured and the > > > signature verified, this integrity gap could be averted. > > > > It is the weakest point in the chain, yes. Relying on existing LSMs > > didn't seem to me a good idea, as: > > - a new policy must be installed > > - we must be sure that the policy is really enforced > > - we need to support different LSMs (SELinux for Fedora, > > Apparmor for SUSE) > > - there might be no LSM we can rely on > > > > For these reasons, I developed a new LSM. Its purpose is to > > identify the user space parser and for each file it opens, ensure > > that the file has been measured or appraised by IMA. If one of > > these actions are missing, it will not be set in the digest list the > > user space parser uploads to the kernel (which means that IMA > > will ignore the digest list for that specific action). > > Properly identifying (all) user space parser(s) would be critical. It > would be simpler and safer to require the converted data be signed. I agree, it would be much easier. However, it would require changes to the building infrastructure of Linux distribution vendors, which might limit the applicability of DIGLIM. With the user space parser taking care of the conversion, distributions can do appraisal of executables and shared libraries with an update of: - the kernel: to add DIGLIM - dracut: to add required digest lists in the initial ram disk - rpm (plugin): to extract the RPM header and its signature and write them to a file that is uploaded to the kernel by the user space parser I'm planning to append the signature at the end of the RPM header (and use appraise_type=modsig) to avoid the dependency on the 'initramfs: add support for xattrs in the initial ram disk' patch set (which I might try to resume in the future). Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > thanks, > > Mimi
> From: Roberto Sassu > Sent: Monday, August 2, 2021 5:13 PM > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > Sent: Monday, August 2, 2021 4:42 PM > > Hi Roberto, > > > > On Fri, 2021-07-30 at 13:16 +0000, Roberto Sassu wrote: > > > > > The reason of storing the actions performed by IMA on the > > > digest lists helps to determine for which purpose they can be > > > used. If digest lists are used only for measurement purpose, > > > it should be sufficient that digest lists are measured. The > > > same applies for appraisal. > > > > Is that assumption correct? How would you know if the digests lists > > are only being used one way and not the other. For example, assuming > > that the digest lists are stored on protected media, the digest lists > > could be measured, but would not necessarily be appraised. > > Hi Mimi > > the actions performed by IMA on the digest lists are recorded > in the digest_list_item structure. These can be retrieved when > IMA calls diglim_digest_get_info() (actually it is the OR of the > actions for the digest lists that contain the digest passed as a > query). > > At the moment, DIGLIM can only know whether a digest list > has been measured or not (with the return value of > ima_measure_critical_data()). In the next patch set, I add the > changes to get the actions from the integrity_iint_cache(). > > > > > Adding the kernel_read_file() "READING_DIGEST_LIST" support in IMA > > does > > > > not seem to be optional. IMA would then be calculating the digest list > > > > file hash twice, once in kernel_read_file() and then, again, in > > > > ima_measure_critical_data(). > > > > > > I didn't include also this part: I retrieve the integrity_iint_cache for > > > the opened file descriptor and I get the flags from there. If the > > > IMA_MEASURED flag is set, it is not necessary to call also > > > ima_measure_critical_data(). > > > > Right, assuming the file is in policy, the digest would already be > > stored in the iint cache. > > > > > > > > I understand that with your changes to ima_measure_critical_data(), > > > > > > which are now in next-integrity-testing branch, allow IMA to calculate > > > > > > the file data hash. > > > > > > > > > > Yes, correct. But actually there is another useful use case. > > > > > If digest lists are not in the format supported by the kernel, > > > > > the user space parser has to convert them before uploading > > > > > them to the kernel. > > > > > > > > > > ima_measure_critical_data() would in this case measure > > > > > the converted digest list (it is written directly, without > > > > > sending the file path). It is easier to attest the result, > > > > > instead of determining whether the user space parser > > > > > produced the expected result (by checking the files it > > > > > read). > > > > > > > > The application to properly convert the digest list file data into the > > > > appropriate format would need to be trusted. I'm concerned that not > > > > requiring the converted data to be signed and the signature verified is > > > > introducing a new integrity gap. Perhaps between an LSM policy, > > > > limiting which files may be read by the application, and an IMA policy, > > > > requiring all files read by this application to be measured and the > > > > signature verified, this integrity gap could be averted. > > > > > > It is the weakest point in the chain, yes. Relying on existing LSMs > > > didn't seem to me a good idea, as: > > > - a new policy must be installed > > > - we must be sure that the policy is really enforced > > > - we need to support different LSMs (SELinux for Fedora, > > > Apparmor for SUSE) > > > - there might be no LSM we can rely on > > > > > > For these reasons, I developed a new LSM. Its purpose is to > > > identify the user space parser and for each file it opens, ensure > > > that the file has been measured or appraised by IMA. If one of > > > these actions are missing, it will not be set in the digest list the > > > user space parser uploads to the kernel (which means that IMA > > > will ignore the digest list for that specific action). > > > > Properly identifying (all) user space parser(s) would be critical. It > > would be simpler and safer to require the converted data be signed. When a process directly uploads a buffer to the kernel, the actions are added to a digest list depending on the result of ima_measure_critical_data() and from the actions attached to the process credentials and set by the new LSM. If a process fails the identification, the actions in the process credentials remain zero and the digest lists the process uploads will be ignored by IMA. The actions in the process credentials are set with the actions performed on the executable by IMA, only if the digest of the executable is found in a digest list and the digest list type is COMPACT_PARSER. The parser is statically linked. The digest list for the parser can be generated at the end of the building process and signed similarly to kernel modules (for SUSE, with pesign-obs-integration). This is the only exception to handle, other packages are not affected. After the parser has been identified, each file operation is monitored. The LSM has to explicitly perform a second open to ensure that the file is measured/appraised before the integrity_iint_cache structure is retrieved (because IMA is called after all LSMs). If an action is missing from the integrity_iint_cache structure, it will be cleared by the LSM in the actions attached to the process credentials, and will not be added to the digest list being uploaded. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > I agree, it would be much easier. However, it would require changes > to the building infrastructure of Linux distribution vendors, which > might limit the applicability of DIGLIM. > > With the user space parser taking care of the conversion, distributions > can do appraisal of executables and shared libraries with an update of: > - the kernel: to add DIGLIM > - dracut: to add required digest lists in the initial ram disk > - rpm (plugin): to extract the RPM header and its signature and write > them to a file that is uploaded to the kernel by the user space parser > > I'm planning to append the signature at the end of the RPM header > (and use appraise_type=modsig) to avoid the dependency on the > 'initramfs: add support for xattrs in the initial ram disk' patch set > (which I might try to resume in the future). > > Thanks > > Roberto > > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 > Managing Director: Li Peng, Li Jian, Shi Yanli > > > thanks, > > > > Mimi
[Cc'ing Eric Snowberg] Hi Roberto, On Mon, 2021-08-02 at 16:54 +0000, Roberto Sassu wrote: > > > Properly identifying (all) user space parser(s) would be critical. It > > > would be simpler and safer to require the converted data be signed. > > When a process directly uploads a buffer to the kernel, the actions are > added to a digest list depending on the result of ima_measure_critical_data() > and from the actions attached to the process credentials and set by the > new LSM. > > If a process fails the identification, the actions in the process credentials > remain zero and the digest lists the process uploads will be ignored by IMA. > > The actions in the process credentials are set with the actions performed > on the executable by IMA, only if the digest of the executable is found in > a digest list and the digest list type is COMPACT_PARSER. The parser is > statically linked. > > The digest list for the parser can be generated at the end of the > building process and signed similarly to kernel modules (for SUSE, > with pesign-obs-integration). This is the only exception to handle, > other packages are not affected. Ok, so to boot strap the set of permitted digest list parsers, the digest list signature is an appended signature, generated by the build process. The key needed for verifying the signature would already be loaded on the builtin keyring. > > After the parser has been identified, each file operation is monitored. Does the new LSM need to monitor all file opens? > The LSM has to explicitly perform a second open to ensure that > the file is measured/appraised before the integrity_iint_cache structure > is retrieved (because IMA is called after all LSMs). > > If an action is missing from the integrity_iint_cache structure, it > will be cleared by the LSM in the actions attached to the process > credentials, and will not be added to the digest list being uploaded. > > > I agree, it would be much easier. However, it would require changes > > to the building infrastructure of Linux distribution vendors, which > > might limit the applicability of DIGLIM. > > I understand, but instead of the distros signing the compact digest lists, with Eric's "Enroll kernel keys thru MOK" patch set, the customer/end user could have more control over which file digests are permitted on a per system basis. > > With the user space parser taking care of the conversion, distributions > > can do appraisal of executables and shared libraries with an update of: > > - the kernel: to add DIGLIM > > - dracut: to add required digest lists in the initial ram disk > > - rpm (plugin): to extract the RPM header and its signature and write > > them to a file that is uploaded to the kernel by the user space parser > > > > I'm planning to append the signature at the end of the RPM header > > (and use appraise_type=modsig) to avoid the dependency on the > > 'initramfs: add support for xattrs in the initial ram disk' patch set > > (which I might try to resume in the future). Based on your explanation above, I surmised as much. thanks, Mimi
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Thursday, August 5, 2021 5:38 PM > [Cc'ing Eric Snowberg] > > Hi Roberto, > > On Mon, 2021-08-02 at 16:54 +0000, Roberto Sassu wrote: > > > > > Properly identifying (all) user space parser(s) would be critical. It > > > > would be simpler and safer to require the converted data be signed. > > > > When a process directly uploads a buffer to the kernel, the actions are > > added to a digest list depending on the result of ima_measure_critical_data() > > and from the actions attached to the process credentials and set by the > > new LSM. > > > > If a process fails the identification, the actions in the process credentials > > remain zero and the digest lists the process uploads will be ignored by IMA. > > > > The actions in the process credentials are set with the actions performed > > on the executable by IMA, only if the digest of the executable is found in > > a digest list and the digest list type is COMPACT_PARSER. The parser is > > statically linked. > > > > The digest list for the parser can be generated at the end of the > > building process and signed similarly to kernel modules (for SUSE, > > with pesign-obs-integration). This is the only exception to handle, > > other packages are not affected. > > Ok, so to boot strap the set of permitted digest list parsers, the > digest list signature is an appended signature, generated by the build > process. The key needed for verifying the signature would already be > loaded on the builtin keyring. Hi Mimi yes. RPM headers will have an appended signature too, so that appraisal will work. > > After the parser has been identified, each file operation is monitored. > > Does the new LSM need to monitor all file opens? I would say yes. In the threat model, root is untrusted and can potentially interfere with the conversion of the original digest lists. Other than monitoring file operations, I'm also denying ptraces on the parser. Hopefully this would be sufficient, but any suggestion is more than welcome. The good thing is that the policy of the new LSM is applied to the processes that are successfully identified as parser. Other processes are mostly unaffected. The only limitation the new LSM would introduce is that the files being used by the parser are write-locked until the parser releases them (if files are already opened for writing by other processes, the LSM would mark the parser as untrusted and will not apply any IMA actions to the digest lists the parser uploads). It is probably a good idea to send the patch, after I finish testing it. I will send also another patch for loading digest lists during kernel initialization (with the two new patches the non-IMA part would be complete). > > The LSM has to explicitly perform a second open to ensure that > > the file is measured/appraised before the integrity_iint_cache structure > > is retrieved (because IMA is called after all LSMs). > > > > If an action is missing from the integrity_iint_cache structure, it > > will be cleared by the LSM in the actions attached to the process > > credentials, and will not be added to the digest list being uploaded. > > > > > I agree, it would be much easier. However, it would require changes > > > to the building infrastructure of Linux distribution vendors, which > > > might limit the applicability of DIGLIM. > > > > > I understand, but instead of the distros signing the compact digest > lists, with Eric's "Enroll kernel keys thru MOK" patch set, the > customer/end user could have more control over which file digests are > permitted on a per system basis. Yes, generating custom digest lists is supported and needed if users want to run their own applications, when appraisal is enforced. But I like the idea that, if users simply want to just run anything the distribution provides, they have everything they need. Theoretically, users will be able to run appraisal in enforcing mode at the first boot after installation. Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > > > With the user space parser taking care of the conversion, distributions > > > can do appraisal of executables and shared libraries with an update of: > > > - the kernel: to add DIGLIM > > > - dracut: to add required digest lists in the initial ram disk > > > - rpm (plugin): to extract the RPM header and its signature and write > > > them to a file that is uploaded to the kernel by the user space parser > > > > > > I'm planning to append the signature at the end of the RPM header > > > (and use appraise_type=modsig) to avoid the dependency on the > > > 'initramfs: add support for xattrs in the initial ram disk' patch set > > > (which I might try to resume in the future). > > Based on your explanation above, I surmised as much. > > thanks, > > Mimi
diff --git a/Documentation/security/diglim/implementation.rst b/Documentation/security/diglim/implementation.rst index 9d679567a037..fc0cd8810a80 100644 --- a/Documentation/security/diglim/implementation.rst +++ b/Documentation/security/diglim/implementation.rst @@ -244,3 +244,12 @@ back when one of the steps fails. #. digest_list_parse() deletes the struct digest_list_item on unsuccessful add or successful delete. + + +Interfaces +---------- + +This section introduces the interfaces in +``<securityfs>/integrity/diglim`` necessary to interact with DIGLIM. + +.. kernel-doc:: security/integrity/diglim/fs.c diff --git a/MAINTAINERS b/MAINTAINERS index 77c3613c600a..0672128fae7f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5464,6 +5464,7 @@ F: Documentation/security/diglim/introduction.rst F: include/linux/diglim.h F: include/uapi/linux/diglim.h F: security/integrity/diglim/diglim.h +F: security/integrity/diglim/fs.c F: security/integrity/diglim/methods.c F: security/integrity/diglim/parser.c diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h index 575ffa1031d3..636ecdfdc616 100644 --- a/include/linux/kernel_read_file.h +++ b/include/linux/kernel_read_file.h @@ -14,6 +14,7 @@ id(KEXEC_INITRAMFS, kexec-initramfs) \ id(POLICY, security-policy) \ id(X509_CERTIFICATE, x509-certificate) \ + id(DIGEST_LIST, digest-list) \ id(MAX_ID, ) #define __fid_enumify(ENUM, dummy) READING_ ## ENUM, diff --git a/security/integrity/diglim/Makefile b/security/integrity/diglim/Makefile index 34e4e154fff3..ac345afdf5dd 100644 --- a/security/integrity/diglim/Makefile +++ b/security/integrity/diglim/Makefile @@ -5,4 +5,4 @@ obj-$(CONFIG_DIGLIM) += diglim.o -diglim-y := methods.o parser.o +diglim-y := methods.o parser.o fs.o diff --git a/security/integrity/diglim/diglim.h b/security/integrity/diglim/diglim.h index 3adc218a0325..819703175eda 100644 --- a/security/integrity/diglim/diglim.h +++ b/security/integrity/diglim/diglim.h @@ -22,6 +22,8 @@ #include <linux/hash_info.h> #include <linux/diglim.h> +#include "../integrity.h" + #define MAX_DIGEST_SIZE 64 #define HASH_BITS 10 #define DIGLIM_HTABLE_SIZE (1 << HASH_BITS) diff --git a/security/integrity/diglim/fs.c b/security/integrity/diglim/fs.c new file mode 100644 index 000000000000..07a0d75a0e33 --- /dev/null +++ b/security/integrity/diglim/fs.c @@ -0,0 +1,239 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2005,2006,2007,2008 IBM Corporation + * Copyright (C) 2017-2021 Huawei Technologies Duesseldorf GmbH + * + * Author: Roberto Sassu <roberto.sassu@huawei.com> + * + * Functions for the interfaces exposed in securityfs. + */ + +#include <linux/fcntl.h> +#include <linux/kernel_read_file.h> +#include <linux/slab.h> +#include <linux/init.h> +#include <linux/seq_file.h> +#include <linux/rculist.h> +#include <linux/rcupdate.h> +#include <linux/parser.h> +#include <linux/vmalloc.h> +#include <linux/namei.h> +#include <linux/ima.h> + +#include "diglim.h" + +#define MAX_DIGEST_LIST_SIZE (64 * 1024 * 1024 - 1) + +static struct dentry *diglim_dir; +/** + * DOC: digest_list_add + * + * digest_list_add can be used to upload a digest list and add the digests + * to the hash table; passed data are interpreted as file path if the first + * byte is ``/`` or as the digest list itself otherwise. + * + * ima_measure_critical_data() is called to calculate the digest of the + * digest list and eventually, if an appropriate rule is set in the IMA + * policy, to measure it. + */ +static struct dentry *digest_list_add_dentry; +/** + * DOC: digest_list_del + * + * digest_list_del can be used to upload a digest list and delete the + * digests from the hash table; data are interpreted in the same way as + * described for digest_list_add. + */ +static struct dentry *digest_list_del_dentry; +char digest_label[NAME_MAX + 1]; + +/* + * digest_list_read: read and parse the digest list from the path + */ +static ssize_t digest_list_read(char *path, enum ops op) +{ + void *data = NULL; + char *datap; + size_t size; + u8 actions = 0; + struct file *file; + char event_name[NAME_MAX + 9 + 1]; + u8 digest[IMA_MAX_DIGEST_SIZE] = { 0 }; + enum hash_algo algo; + int rc, pathlen = strlen(path); + + /* Remove \n. */ + datap = path; + strsep(&datap, "\n"); + + file = filp_open(path, O_RDONLY, 0); + if (IS_ERR(file)) { + pr_err("unable to open file: %s (%ld)", path, PTR_ERR(file)); + return PTR_ERR(file); + } + + rc = kernel_read_file(file, 0, &data, INT_MAX, NULL, + READING_DIGEST_LIST); + if (rc < 0) { + pr_err("unable to read file: %s (%d)", path, rc); + goto out; + } + + size = rc; + + snprintf(event_name, sizeof(event_name), "%s_file_%s", + op == DIGEST_LIST_ADD ? "add" : "del", + file_dentry(file)->d_name.name); + + rc = ima_measure_critical_data("diglim", event_name, data, size, false, + digest, sizeof(digest)); + if (rc < 0 && rc != -EEXIST) + goto out_vfree; + + algo = ima_get_current_hash_algo(); + + if (!rc || rc == -EEXIST) + actions |= (1 << COMPACT_ACTION_IMA_MEASURED); + + rc = digest_list_parse(size, data, op, actions, digest, algo, ""); + if (rc < 0) + pr_err("unable to upload digest list %s (%d)\n", path, rc); +out_vfree: + vfree(data); +out: + fput(file); + + if (rc < 0) + return rc; + + return pathlen; +} + +/* + * digest_list_write: write the digest list path or the digest list itself + */ +static ssize_t digest_list_write(struct file *file, const char __user *buf, + size_t datalen, loff_t *ppos) +{ + char *data; + ssize_t result; + enum ops op = DIGEST_LIST_ADD; + struct dentry *dentry = file_dentry(file); + u8 digest[IMA_MAX_DIGEST_SIZE]; + char event_name[NAME_MAX + 11 + 1]; + enum hash_algo algo; + u8 actions = 0; + + /* No partial writes. */ + result = -EINVAL; + if (*ppos != 0) + goto out; + + result = -EFBIG; + if (datalen > MAX_DIGEST_LIST_SIZE) + goto out; + + data = memdup_user_nul(buf, datalen); + if (IS_ERR(data)) { + result = PTR_ERR(data); + goto out; + } + + if (dentry == digest_list_del_dentry) + op = DIGEST_LIST_DEL; + + result = -EPERM; + + if (data[0] == '/') { + result = digest_list_read(data, op); + } else { + snprintf(event_name, sizeof(event_name), "%s_buffer_%s", + op == DIGEST_LIST_ADD ? "add" : "del", digest_label); + + result = ima_measure_critical_data("diglim", event_name, data, + datalen, false, digest, + sizeof(digest)); + if (result < 0 && result != -EEXIST) { + pr_err("failed to measure buffer digest (%ld)\n", + result); + goto out_kfree; + } + + algo = ima_get_current_hash_algo(); + + if (!result || result == -EEXIST) + actions |= (1 << COMPACT_ACTION_IMA_MEASURED); + + result = digest_list_parse(datalen, data, op, actions, digest, + algo, ""); + if (result != datalen) { + pr_err("unable to upload generated digest list\n"); + result = -EINVAL; + } + } +out_kfree: + kfree(data); +out: + return result; +} + +static unsigned long flags; + +/* + * digest_list_open: sequentialize access to the add/del files + */ +static int digest_list_open(struct inode *inode, struct file *filp) +{ + if ((filp->f_flags & O_ACCMODE) != O_WRONLY) + return -EACCES; + + if (test_and_set_bit(0, &flags)) + return -EBUSY; + + return 0; +} + +/* + * digest_list_release - release the add/del files + */ +static int digest_list_release(struct inode *inode, struct file *file) +{ + clear_bit(0, &flags); + return 0; +} + +static const struct file_operations digest_list_upload_ops = { + .open = digest_list_open, + .write = digest_list_write, + .read = seq_read, + .release = digest_list_release, + .llseek = generic_file_llseek, +}; + +static int __init diglim_fs_init(void) +{ + diglim_dir = securityfs_create_dir("diglim", integrity_dir); + if (IS_ERR(diglim_dir)) + return -1; + + digest_list_add_dentry = securityfs_create_file("digest_list_add", 0200, + diglim_dir, NULL, + &digest_list_upload_ops); + if (IS_ERR(digest_list_add_dentry)) + goto out; + + digest_list_del_dentry = securityfs_create_file("digest_list_del", 0200, + diglim_dir, NULL, + &digest_list_upload_ops); + if (IS_ERR(digest_list_del_dentry)) + goto out; + + return 0; +out: + securityfs_remove(digest_list_del_dentry); + securityfs_remove(digest_list_add_dentry); + securityfs_remove(diglim_dir); + return -1; +} + +late_initcall(diglim_fs_init); diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 547425c20e11..ac45e1599c2d 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -6,6 +6,9 @@ * Mimi Zohar <zohar@us.ibm.com> */ +#ifndef __INTEGRITY_H +#define __INTEGRITY_H + #ifdef pr_fmt #undef pr_fmt #endif @@ -283,3 +286,4 @@ static inline void __init add_to_platform_keyring(const char *source, { } #endif +#endif /*__INTEGRITY_H*/
Introduce <securityfs>/integrity/diglim/digest_list_add, which can be used to upload a digest list and add the digests to the hash table; passed data are interpreted as file path if the first byte is / or as the digest list itself otherwise. ima_measure_critical_data() is called to calculate the digest of the digest list and eventually, if an appropriate rule is set in the IMA policy, to measure it. Also introduce <securityfs>/integrity/diglim/digest_list_del, which can be used to upload a digest list and delete the digests from the hash table; data are interpreted in the same way as described for digest_list_add. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- .../security/diglim/implementation.rst | 9 + MAINTAINERS | 1 + include/linux/kernel_read_file.h | 1 + security/integrity/diglim/Makefile | 2 +- security/integrity/diglim/diglim.h | 2 + security/integrity/diglim/fs.c | 239 ++++++++++++++++++ security/integrity/integrity.h | 4 + 7 files changed, 257 insertions(+), 1 deletion(-) create mode 100644 security/integrity/diglim/fs.c