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>

1 files changed, 90 insertions, 69 deletions

diff --git a/libblkid/src/superblocks/zfs.c b/libblkid/src/superblocks/zfs.c
index 02fa95675..136087c23 100644
--- 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);
 	}
 }