Re: [Linux-ima-devel] [PATCH v6 02/10] ima: on soft reboot, restore the measurement list

On Tue, 2016-11-08 at 15:47 -0500, Mimi Zohar wrote:

On Tue, 2016-11-08 at 21:46 +0200, Dmitry Kasatkin wrote:

quoted

On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermann

quoted

quoted

+/* Restore the serialized binary measurement list without extending PCRs. */
+int ima_restore_measurement_list(loff_t size, void *buf)
+{
+       struct binary_hdr_v1 {
+               u32 pcr;
+               u8 digest[TPM_DIGEST_SIZE];
+               u32 template_name_len;
+               char template_name[0];
+       } __packed;
+       char template_name[MAX_TEMPLATE_NAME_LEN];
+
+       struct binary_data_v1 {
+               u32 template_data_size;
+               char template_data[0];
+       } __packed;
+
+       struct ima_kexec_hdr *khdr = buf;
+       struct binary_hdr_v1 *hdr_v1;
+       struct binary_data_v1 *data_v1;
+
+       void *bufp = buf + sizeof(*khdr);
+       void *bufendp = buf + khdr->buffer_size;
+       struct ima_template_entry *entry;
+       struct ima_template_desc *template_desc;
+       unsigned long count = 0;
+       int ret = 0;
+
+       if (!buf || size < sizeof(*khdr))
+               return 0;
+
+       if (khdr->version != 1) {
+               pr_err("attempting to restore a incompatible measurement list");
+               return 0;
+       }
+
+       /*
+        * ima kexec buffer prefix: version, buffer size, count
+        * v1 format: pcr, digest, template-name-len, template-name,
+        *            template-data-size, template-data
+        */
+       while ((bufp < bufendp) && (count++ < khdr->count)) {
+               if (count > ULONG_MAX - 1) {
+                       pr_err("attempting to restore too many measurements");
+                       ret = -EINVAL;
+               }
+
+               hdr_v1 = bufp;
+               if ((hdr_v1->template_name_len >= MAX_TEMPLATE_NAME_LEN) ||
+                   ((bufp + hdr_v1->template_name_len) > bufendp)) {

based on following code  template_name_len does not include header
(sizeof(*hdr_v1))?
If so the check is wrong???

Yes, good catch.  In addition, before assigning hdr_v1 there should be a
size check as well. 

quoted

quoted

+                       pr_err("attempting to restore a template name \
+                               that is too long\n");
+                       ret = -EINVAL;
+                       break;
+               }
+               bufp += sizeof(*hdr_v1);

This line should have been before the test above.

quoted

quoted

+
+               /* template name is not null terminated */
+               memcpy(template_name, bufp, hdr_v1->template_name_len);
+               template_name[hdr_v1->template_name_len] = 0;
+
+               if (strcmp(template_name, "ima") == 0) {
+                       pr_err("attempting to restore an unsupported \
+                               template \"%s\" failed\n", template_name);
+                       ret = -EINVAL;
+                       break;
+               }
+               data_v1 = bufp += (u_int8_t)hdr_v1->template_name_len;
+
+               /* get template format */
+               template_desc = lookup_template_desc(template_name);
+               if (!template_desc) {
+                       pr_err("template \"%s\" not found\n", template_name);
+                       ret = -EINVAL;
+                       break;
+               }
+
+               if (bufp > (bufendp - sizeof(data_v1->template_data_size))) {
+                       pr_err("restoring the template data size failed\n");
+                       ret = -EINVAL;
+                       break;
+               }
+               bufp += (u_int8_t) sizeof(data_v1->template_data_size);
+
+               if (bufp > (bufendp - data_v1->template_data_size)) {
+                       pr_err("restoring the template data failed\n");
+                       ret = -EINVAL;
+                       break;
+               }
+

It looks like a similar problem... sizeof(struct binary_data_v1) is
missing in the check...

Following the template name, is the template data length and the
template data. 

quoted

quoted

+               ret = ima_restore_template_data(template_desc,
+                                               data_v1->template_data,
+                                               data_v1->template_data_size,
+                                               &entry);
+               if (ret < 0)
+                       break;
+
+               memcpy(entry->digest, hdr_v1->digest, TPM_DIGEST_SIZE);
+               entry->pcr = hdr_v1->pcr;
+               ret = ima_restore_measurement_entry(entry);
+               if (ret < 0)
+                       break;
+
+               bufp += data_v1->template_data_size;
+       }
+       return ret;
+}
--
2.7.4

In overall it is a bit hard to read this function somehow..

Ok, I'll see if there is any way of simplifying/cleaning up walking the
measurement list some more.

I moved some code around so that the bufp pointer update is immediately
after the  buffer overflow tests and moved one check outside the while
loop.  I tried defining a buffer overflow macro, but that didn't seem to
help.

The updated patches are available in the next-kexec-restore branch of:
git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git

Mimi