Re: [RFC][PATCH v2 06/12] diglim: Interfaces - digest_list_add, digest_list_del

Hi Roberto,

On Fri, 2021-07-30 at 07:16 +0000, Roberto Sassu wrote:

quoted

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:

quoted

/*
+ * 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().

quoted

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