[PATCH] libblkid: bounds-check volume-supplied offsets in NTFS/exFAT probes

[PATCH] libblkid: bounds-check volume-supplied offsets in NTFS/exFAT probes

[Akira Patafio]

probe_ntfs() and probe_exfat() use four fields read straight from the
volume without validating them, and each one produces an out-of-bounds
read. blkid is normally the first thing to touch a newly inserted
filesystem - udisks2 runs it on hotplug, and on Android vold runs it on
removable media before any fsck and before any mount, under a dedicated
blkid_untrusted SELinux domain - so the input is attacker-supplied
whenever the medium is.

1. mft_record_size, probe.c:709

   cluster_per_mft_record is a signed byte from the boot sector. When
   negative it means the record size is 2^-value, computed as
   1 << (0 - cluster_per_mft_record), so the shift exponent can reach
   128. Shifting an int by >= 32 is undefined; in practice the result
   comes out below 4, get_buffer() allocates exactly that many bytes,
   and the memcmp(buf_mft, "FILE", 4) that follows reads past it.

   Reject an exponent that cannot fit, and require the record to be at
   least as large as the header that every later read indexes into.

2. The attribute walk, probe.c:754

   attr_off starts at mft->attrs_offset, a 16-bit volume field, and is
   advanced by attr->len each iteration. The loop dereferences the
   attribute first and checks attr_off against mft_record_size only
   afterwards, so the very first read can be up to 64 KiB past the
   record buffer.

   Move the check to the top of the loop, before the dereference.

3. The NTFS volume label, probe.c:774

   val_len is clamped to sizeof(label_str), but val_off - the
   attribute's value_offset, equally volume-supplied - is not, so the
   copy loop can start at an arbitrary offset past the record. What it
   reads is written into label_str and published as the LABEL tag, so
   this site does not just read out of bounds, it surfaces the result.

   Bound value_offset + value_len against the record size.

4. The exFAT volume label, probe.c:1523

   label->length is a single byte and can ask for up to 255 characters,
   510 bytes, out of a 30-byte name field. exFAT permits at most 11, so
   clamp to that. Note this is a behaviour change for a malformed
   volume claiming a longer label: it is now truncated to 11 characters
   rather than read past the field. util-linux's copy of this code
   clamps the same way.

None of these is a write, and none crashes an unsanitized build; the
practical worst case is heap adjacent to the probe buffer appearing in
a volume label.

Found by fuzzing the blkid entry point with libFuzzer under ASan and
UBSan on aarch64, then reduced to four hand-built images of 64-256 KiB.
Tested on this tree by driving blkid_get_dev() over each image the way
a caller does: 13 AddressSanitizer heap-buffer-overflow reports before
(1, 8, 2 and 2 across the four), none after. A well-formed exFAT
superblock through the same path is unaffected, still reporting
LABEL="MYSD" UUID="1234-5678" TYPE="exfat". Verified on both x86_64 and
aarch64.

Signed-off-by: Akira Patafio <kokokoala4211@gmail.com>
---
 lib/blkid/probe.c | 57 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 4 deletions(-)

diff --git a/lib/blkid/probe.c b/lib/blkid/probe.c
index 6a3bb24..7986bb2 100644
--- a/lib/blkid/probe.c
+++ b/lib/blkid/probe.c
@@ -705,11 +705,27 @@ static int probe_ntfs(struct blkid_probe *probe,
 	if ((bytes_per_sector < 512) || (sectors_per_cluster == 0))
 		return 1;
 
-	if (ns->cluster_per_mft_record < 0)
+	if (ns->cluster_per_mft_record < 0) {
+		/*
+		 * A negative value means the record size is 2^-value bytes.
+		 * cluster_per_mft_record is read straight from the volume, so
+		 * without this check the shift exponent can reach 128 and the
+		 * shift is undefined.
+		 */
+		if (ns->cluster_per_mft_record < -30)
+			return 1;
 		mft_record_size = 1 << (0-ns->cluster_per_mft_record);
-	else
+	} else
 		mft_record_size = ns->cluster_per_mft_record *
 			sectors_per_cluster * bytes_per_sector;
+
+	/*
+	 * Everything below reads inside a buffer of mft_record_size bytes: the
+	 * "FILE" magic, the record header, and the attribute walk. A record too
+	 * small to hold the header is not a valid NTFS volume.
+	 */
+	if (mft_record_size < (int) sizeof(struct master_file_table_record))
+		return 1;
 	nr_clusters = blkid_le64(ns->number_of_sectors) / sectors_per_cluster;
 
 	if ((blkid_le64(ns->mft_cluster_location) > nr_clusters) ||
@@ -751,7 +767,20 @@ static int probe_ntfs(struct blkid_probe *probe,
 	label_str[0] = 0;
 
 	while (1) {
-		attr = (struct file_attribute *) (buf_mft + attr_off);
+		int cur_off = attr_off;
+
+		/*
+		 * attrs_offset, and every attr->len that advances attr_off,
+		 * come straight from the volume. The existing check below
+		 * runs AFTER the attribute has already been dereferenced, so
+		 * bound the offset here, before it is used.
+		 */
+		if (cur_off < 0 ||
+		    cur_off + (int) sizeof(struct file_attribute) >
+							mft_record_size)
+			break;
+
+		attr = (struct file_attribute *) (buf_mft + cur_off);
 		attr_len = blkid_le16(attr->len);
 		attr_type = blkid_le32(attr->type);
 		val_off = blkid_le16(attr->value_offset);
@@ -770,6 +799,16 @@ static int probe_ntfs(struct blkid_probe *probe,
 			if (val_len > sizeof(label_str))
 				val_len = sizeof(label_str)-1;
 
+			/*
+			 * value_offset is relative to the attribute and is also
+			 * volume-supplied. val_len is clamped just above but
+			 * val_off was not, so the copy below could start at an
+			 * arbitrary offset past the record.
+			 */
+			if (val_off < 0 ||
+			    cur_off + val_off + (int) val_len > mft_record_size)
+				break;
+
 			for (i=0, cp=label_str; i < val_len; i+=2,cp++) {
 				val = ((__u8 *) attr) + val_off + i;
 				*cp = val[0];
@@ -1520,7 +1559,17 @@ static int probe_exfat(struct blkid_probe *probe,
     label = find_exfat_entry_label(probe, sb);
     if (label) {
         unsigned char utf8_label[128];
-        unicode_16le_to_utf8(utf8_label, sizeof(utf8_label), label->name, label->length * 2);
+        /*
+         * label->length is a single volume-supplied byte, so it can claim up
+         * to 255 characters - 510 bytes - from a 30-byte name field. exFAT
+         * permits at most 11.
+         */
+        unsigned int label_len = label->length;
+
+        if (label_len > 11)
+            label_len = 11;
+        unicode_16le_to_utf8(utf8_label, sizeof(utf8_label), label->name,
+                             label_len * 2);
         blkid_set_tag(probe->dev, "LABEL", (char *) utf8_label, 0);
     } else {
         blkid_set_tag(probe->dev, "LABEL", "disk", 4);
-- 
2.50.1