libblkid: Fix out of bounds byte swaps in ZFS handling

A corrupted ZFS filesystem can trigger 32-bit endian-conversions of
unintended memory locations in zfs_extract_guid_name(), in several ways:

* The variable "left" (number of bytes remaining in the buffer) does not
  account for the 12 bytes of the nvlist header.

* The field nvp->nvp_namelen (name length in name/value pair) is rounded
  up to the nearest multiple of 4, but only the unrounded size is checked.

* The fields nvs->nvs_type, nvs_strlen, etc. are modified _before_ checking
  if they are within bounds.

* A negative value of nvp->nvp_namelen will bypass the check that
  nvp->nvp_namelen fits into nvp->nvp_size (size of name/value pair).

This allows for mangling of locations up to 12 + 3 + 8 == 23
bytes beyond the end of stack-based buff[4096], and up to 2**31 bytes
before its beginning.

Furthermore some debugging messages are printed from unchecked memory
locations, possibly resulting in OOB reads or setuid programs leaking
sensitive data when LIBBLKID_DEBUG is set.

This fix attempts to correct all of these problems. It also eliminates the
stack-based buffer (in case anything else was missed) and refactors things
a bit to (hopefully) make it easier to spot any mistakes.

Signed-off-by: Alden Tondettar <alden.tondettar@gmail.com>

diff --git a/libblkid/src/superblocks/zfs.c b/libblkid/src/superblocks/zfs.c
index 02fa95675e75a12ba9fb7e55c3f5f2030bd3c528..136087c235baf6675c745acb3ca684ba3c546844 100644 (file)
--- a/libblkid/src/superblocks/zfs.c
+++ b/libblkid/src/superblocks/zfs.c
@@ -65,12 +65,74 @@ struct nvlist {
        struct nvpair   nvl_nvpair;
 };
 
+static int zfs_process_value(blkid_probe pr, char *name, size_t namelen,
+                            void *value, size_t max_value_size)
+{
+       if (strncmp(name, "name", namelen) == 0 &&
+           sizeof(struct nvstring) <= max_value_size) {
+               struct nvstring *nvs = value;
+               uint32_t nvs_type = be32_to_cpu(nvs->nvs_type);
+               uint32_t nvs_strlen = be32_to_cpu(nvs->nvs_strlen);
+
+               if (nvs_type != DATA_TYPE_STRING ||
+                   (uint64_t)nvs_strlen + sizeof(*nvs) > max_value_size)
+                       return 0;
+
+               DBG(LOWPROBE, ul_debug("nvstring: type %u string %*s\n",
+                                      nvs_type, nvs_strlen, nvs->nvs_string));
+
+               blkid_probe_set_label(pr, nvs->nvs_string, nvs_strlen);
+
+               return 1;
+       } else if (strncmp(name, "guid", namelen) == 0 &&
+                  sizeof(struct nvuint64) <= max_value_size) {
+               struct nvuint64 *nvu = value;
+               uint32_t nvu_type = be32_to_cpu(nvu->nvu_type);
+               uint64_t nvu_value;
+
+               memcpy(&nvu_value, &nvu->nvu_value, sizeof(nvu_value));
+               nvu_value = be64_to_cpu(nvu_value);
+
+               if (nvu_type != DATA_TYPE_UINT64)
+                       return 0;
+
+               DBG(LOWPROBE, ul_debug("nvuint64: type %u value %"PRIu64"\n",
+                                      nvu_type, nvu_value));
+
+               blkid_probe_sprintf_value(pr, "UUID_SUB",
+                                         "%"PRIu64, nvu_value);
+
+               return 1;
+       } else if (strncmp(name, "pool_guid", namelen) == 0 &&
+                  sizeof(struct nvuint64) <= max_value_size) {
+               struct nvuint64 *nvu = value;
+               uint32_t nvu_type = be32_to_cpu(nvu->nvu_type);
+               uint64_t nvu_value;
+
+               memcpy(&nvu_value, &nvu->nvu_value, sizeof(nvu_value));
+               nvu_value = be64_to_cpu(nvu_value);
+
+               if (nvu_type != DATA_TYPE_UINT64)
+                       return 0;
+
+               DBG(LOWPROBE, ul_debug("nvuint64: type %u value %"PRIu64"\n",
+                                      nvu_type, nvu_value));
+
+               blkid_probe_sprintf_uuid(pr, (unsigned char *) &nvu_value,
+                                        sizeof(nvu_value),
+                                        "%"PRIu64, nvu_value);
+               return 1;
+       }
+
+       return 0;
+}
+
 static void zfs_extract_guid_name(blkid_probe pr, loff_t offset)
 {
-       unsigned char *p, buff[4096];
+       unsigned char *p;
        struct nvlist *nvl;
        struct nvpair *nvp;
-       size_t left = sizeof(buff);
+       size_t left = 4096;
        int found = 0;
 
        offset = (offset & ~(VDEV_LABEL_SIZE - 1)) + VDEV_LABEL_NVPAIR;
@@ -83,82 +145,41 @@ static void zfs_extract_guid_name(blkid_probe pr, loff_t offset)
        if (!p)
                return;
 
-       /* libblkid buffers are strictly readonly, but the code below modifies nvpair etc. */
-       memcpy(buff, p, sizeof(buff));
-       nvl = (struct nvlist *) buff;
-
-       DBG(LOWPROBE, ul_debug("zfs_extract: nvlist offset %jd\n", (intmax_t)offset));
+       DBG(LOWPROBE, ul_debug("zfs_extract: nvlist offset %jd\n",
+                              (intmax_t)offset));
 
+       nvl = (struct nvlist *) p;
        nvp = &nvl->nvl_nvpair;
+       left -= (unsigned char *)nvp - p; /* Already used up 12 bytes */
+
        while (left > sizeof(*nvp) && nvp->nvp_size != 0 && found < 3) {
-               int avail;   /* tracks that name/value data fits in nvp_size */
-               int namesize;
+               uint32_t nvp_size = be32_to_cpu(nvp->nvp_size);
+               uint32_t nvp_namelen = be32_to_cpu(nvp->nvp_namelen);
+               uint64_t namesize = ((uint64_t)nvp_namelen + 3) & ~3;
+               size_t max_value_size;
+               void *value;
 
-               nvp->nvp_size = be32_to_cpu(nvp->nvp_size);
-               nvp->nvp_namelen = be32_to_cpu(nvp->nvp_namelen);
-               avail = nvp->nvp_size - nvp->nvp_namelen - sizeof(*nvp);
+               DBG(LOWPROBE, ul_debug("left %zd nvp_size %u\n",
+                                      left, nvp_size));
 
-               DBG(LOWPROBE, ul_debug("left %zd nvp_size %u\n", left, nvp->nvp_size));
-               if (left < nvp->nvp_size || avail < 0)
+               /* nvpair fits in buffer and name fits in nvpair? */
+               if (nvp_size > left || namesize + sizeof(*nvp) > nvp_size)
                        break;
 
-               namesize = (nvp->nvp_namelen + 3) & ~3;
+               DBG(LOWPROBE,
+                   ul_debug("nvlist: size %u, namelen %u, name %*s\n",
+                            nvp_size, nvp_namelen, nvp_namelen,
+                            nvp->nvp_name));
 
-               DBG(LOWPROBE, ul_debug("nvlist: size %u, namelen %u, name %*s\n",
-                       nvp->nvp_size, nvp->nvp_namelen, nvp->nvp_namelen,
-                       nvp->nvp_name));
-               if (strncmp(nvp->nvp_name, "name", nvp->nvp_namelen) == 0) {
-                       struct nvstring *nvs = (void *)(nvp->nvp_name+namesize);
+               max_value_size = nvp_size - (namesize + sizeof(*nvp));
+               value = nvp->nvp_name + namesize;
 
-                       nvs->nvs_type = be32_to_cpu(nvs->nvs_type);
-                       nvs->nvs_strlen = be32_to_cpu(nvs->nvs_strlen);
-                       if (nvs->nvs_strlen > INT_MAX - sizeof(*nvs))
-                               break;
-                       avail -= nvs->nvs_strlen + sizeof(*nvs);
-                       DBG(LOWPROBE, ul_debug("nvstring: type %u string %*s\n", nvs->nvs_type,
-                               nvs->nvs_strlen, nvs->nvs_string));
-                       if (nvs->nvs_type == DATA_TYPE_STRING && avail >= 0)
-                               blkid_probe_set_label(pr, nvs->nvs_string,
-                                                     nvs->nvs_strlen);
-                       found++;
-               } else if (strncmp(nvp->nvp_name, "guid",
-                                  nvp->nvp_namelen) == 0) {
-                       struct nvuint64 *nvu = (void *)(nvp->nvp_name+namesize);
-                       uint64_t nvu_value;
-
-                       memcpy(&nvu_value, &nvu->nvu_value, sizeof(nvu_value));
-                       nvu->nvu_type = be32_to_cpu(nvu->nvu_type);
-                       nvu_value = be64_to_cpu(nvu_value);
-                       avail -= sizeof(*nvu);
-                       DBG(LOWPROBE, ul_debug("nvuint64: type %u value %"PRIu64"\n",
-                               nvu->nvu_type, nvu_value));
-                       if (nvu->nvu_type == DATA_TYPE_UINT64 && avail >= 0)
-                               blkid_probe_sprintf_value(pr, "UUID_SUB",
-                                                         "%"PRIu64, nvu_value);
-                       found++;
-               } else if (strncmp(nvp->nvp_name, "pool_guid",
-                                  nvp->nvp_namelen) == 0) {
-                       struct nvuint64 *nvu = (void *)(nvp->nvp_name+namesize);
-                       uint64_t nvu_value;
-
-                       memcpy(&nvu_value, &nvu->nvu_value, sizeof(nvu_value));
-                       nvu->nvu_type = be32_to_cpu(nvu->nvu_type);
-                       nvu_value = be64_to_cpu(nvu_value);
-                       avail -= sizeof(*nvu);
-                       DBG(LOWPROBE, ul_debug("nvuint64: type %u value %"PRIu64"\n",
-                               nvu->nvu_type, nvu_value));
-                       if (nvu->nvu_type == DATA_TYPE_UINT64 && avail >= 0)
-                               blkid_probe_sprintf_uuid(pr, (unsigned char *)
-                                                        &nvu_value,
-                                                        sizeof(nvu_value),
-                                                        "%"PRIu64, nvu_value);
-                       found++;
-               }
-               if (left > nvp->nvp_size)
-                       left -= nvp->nvp_size;
-               else
-                       left = 0;
-               nvp = (struct nvpair *)((char *)nvp + nvp->nvp_size);
+               found += zfs_process_value(pr, nvp->nvp_name, nvp_namelen,
+                                          value, max_value_size);
+
+               left -= nvp_size;
+
+               nvp = (struct nvpair *)((char *)nvp + nvp_size);
        }
 }