[PATCH v1 0/2] fs/squashfs: bounds checks on image-controlled offsets

[PATCH v1 0/2] fs/squashfs: bounds checks on image-controlled offsets

[Pranav Rajendran]

Two out-of-bounds reads reachable from a crafted SquashFS image, found
while auditing fs/squashfs for image-controlled values used as buffer
offsets without validation. Both were reported to the list earlier; these
are the fixes.

Patch 1 covers sqfs_frag_lookup(), where the fragment index is checked
only against a superblock field that is itself part of the image, and
every subsequent access derived from it is unchecked against the buffer
that was actually read.

Patch 2 covers sqfs_dir_offset(). Commit 57e0bb7bf00d ("fs/squashfs: add
sqfs_dir_offset() error checks") addressed the negative return value; the
positive range is still unbounded, and the callers use it to index the
directory table.

Neither patch changes behaviour for well-formed images: the rejected
cases all describe inodes that reference data outside the tables the
superblock declares.

checkpatch-clean, builds for sandbox with no new warnings at W=1, and
test_sqfs_ls and test_sqfs_load both pass against images generated by
mksquashfs 4.6.1 (the default plus the three lzo fragment variants).

Review of the exact bounds is welcome, in particular whether patch 2 is
too strict in rejecting a directory header that would start within the
last SQFS_DIR_HEADER_SIZE bytes of the directory table.

Pranav Rajendran (2):
  fs/squashfs: bound fragment table accesses in sqfs_frag_lookup()
  fs/squashfs: bound the offset returned by sqfs_dir_offset()

 fs/squashfs/sqfs.c     | 50 ++++++++++++++++++++++++++++++++++++++++--
 fs/squashfs/sqfs_dir.c | 42 ++++++++++++++++++++++++++++++-----
 2 files changed, 84 insertions(+), 8 deletions(-)

-- 
2.50.1 (Apple Git-155)

[PATCH v1 1/2] fs/squashfs: bound fragment table accesses in sqfs_frag_lookup()

[Pranav Rajendran]

sqfs_frag_lookup() validates its fragment index only against
sblk->fragments, which is read from the image superblock and is
therefore under the control of whoever supplies the image. Every
buffer access derived from that index is then made without checking
it against the size of the buffer actually read from the device:

 - the fragment index table entry at 'table_offset + block *
   sizeof(u64)' can be read past the end of 'table', as 'block' is
   inode_fragment_index / SQFS_MAX_ENTRIES and has no upper bound;

 - the metadata block header and payload are read from
   'metadata_buffer' at 'table_offset', but that buffer is sized from
   start_block, which comes from the unvalidated index table entry
   above. A start_block just below sblk->fragment_table_start yields a
   single-block buffer while SQFS_METADATA_SIZE(header) may be up to
   SQFS_METADATA_BLOCK_SIZE, so both the decompression source and the
   memcpy() source can run past the end of the buffer;

 - 'entries' is allocated with SQFS_METADATA_BLOCK_SIZE bytes but only
   partially filled, so entries[offset] can read uninitialised heap
   memory when the metadata block holds fewer than offset + 1 entries.

Compute the size of both buffers explicitly, rejecting the
multiplication overflow the way sqfs_read_directory_table() already
does, and check each access against it. Also track how much of
'entries' was populated and reject an index beyond that.

A crafted SquashFS image can trigger all three cases, either crashing
U-Boot or feeding adjacent heap contents into the fragment entry that
the following data read is based on.

Fixes: c51006130370 ("fs/squashfs: new filesystem")
Signed-off-by: Pranav Rajendran <pranavkasthuri@gmail.com>
---
 fs/squashfs/sqfs.c | 50 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
index 55fbe1bcc1a..cd88923521e 100644
--- a/fs/squashfs/sqfs.c
+++ b/fs/squashfs/sqfs.c
@@ -109,6 +109,7 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 	unsigned char *metadata_buffer, *metadata, *table;
 	struct squashfs_fragment_block_entry *entries;
 	struct squashfs_super_block *sblk = ctxt.sblk;
+	size_t table_size, metadata_size, valid_len;
 	unsigned long dest_len;
 	int block, offset, ret;
 	u16 header;
@@ -133,7 +134,12 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 	start /= ctxt.cur_dev->blksz;
 
 	/* Allocate a proper sized buffer to store the fragment index table */
-	table = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz);
+	if (__builtin_mul_overflow(n_blks, ctxt.cur_dev->blksz, &table_size)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	table = malloc_cache_aligned(table_size);
 	if (!table) {
 		ret = -ENOMEM;
 		goto out;
@@ -147,6 +153,16 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 	block = SQFS_FRAGMENT_INDEX(inode_fragment_index);
 	offset = SQFS_FRAGMENT_INDEX_OFFSET(inode_fragment_index);
 
+	/*
+	 * 'inode_fragment_index' is only checked against sblk->fragments, which
+	 * is itself read from the image, so the resulting index may point past
+	 * the fragment index table that was actually read from the device.
+	 */
+	if (table_offset + ((u64)block + 1) * sizeof(u64) > table_size) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/*
 	 * Get the start offset of the metadata block that contains the right
 	 * fragment block entry
@@ -158,7 +174,13 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 	n_blks = sqfs_calc_n_blks(cpu_to_le64(start_block),
 				  sblk->fragment_table_start, &table_offset);
 
-	metadata_buffer = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz);
+	if (__builtin_mul_overflow(n_blks, ctxt.cur_dev->blksz,
+				   &metadata_size)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	metadata_buffer = malloc_cache_aligned(metadata_size);
 	if (!metadata_buffer) {
 		ret = -ENOMEM;
 		goto out;
@@ -170,6 +192,11 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 	}
 
 	/* Every metadata block starts with a 16-bit header */
+	if (table_offset + SQFS_HEADER_SIZE > metadata_size) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	header = get_unaligned_le16(metadata_buffer + table_offset);
 	metadata = metadata_buffer + table_offset + SQFS_HEADER_SIZE;
 
@@ -183,6 +210,16 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 		goto out;
 	}
 
+	/*
+	 * The metadata block's payload is read straight out of
+	 * 'metadata_buffer', so it has to fit in what was read from the device.
+	 */
+	if (table_offset + SQFS_HEADER_SIZE + SQFS_METADATA_SIZE(header) >
+	    metadata_size) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	entries = malloc(SQFS_METADATA_BLOCK_SIZE);
 	if (!entries) {
 		ret = -ENOMEM;
@@ -198,8 +235,17 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 			ret = -EINVAL;
 			goto out;
 		}
+
+		valid_len = dest_len;
 	} else {
 		memcpy(entries, metadata, SQFS_METADATA_SIZE(header));
+		valid_len = SQFS_METADATA_SIZE(header);
+	}
+
+	/* Only the part of 'entries' that was actually filled in is usable */
+	if (((u64)offset + 1) * sizeof(*entries) > valid_len) {
+		ret = -EINVAL;
+		goto out;
 	}
 
 	*e = entries[offset];
-- 
2.50.1 (Apple Git-155)

[PATCH v1 2/2] fs/squashfs: bound the offset returned by sqfs_dir_offset()

[Pranav Rajendran]

Commit 57e0bb7bf00d ("fs/squashfs: add sqfs_dir_offset() error checks")
made sqfs_search_dir() reject negative returns from sqfs_dir_offset(),
but the positive range is still unbounded. Both parts of the returned
offset come from the image: 'offset' is a 16-bit inode field used
verbatim, and the matched metadata block index may be the last one in
m_list, in which case the returned block (j + 1) is one past the end
of the directory table.

The callers use the result to index dirs->dir_table[], which
sqfs_read_directory_table() allocates as m_count metadata blocks, and
then memcpy() a directory header out of it. A crafted image can
therefore read up to 64 KiB past the end of that allocation.

Reject an inode offset that cannot address a decompressed metadata
block, and verify that the resulting directory header lies entirely
within the directory table.

The existing 'offset < 0' test is dropped: 'offset' is assigned from
get_unaligned_le16() and so is never negative, meaning the test never
fired. The new upper bound covers what it was meant to catch.

Fixes: c51006130370 ("fs/squashfs: new filesystem")
Signed-off-by: Pranav Rajendran <pranavkasthuri@gmail.com>
---
 fs/squashfs/sqfs_dir.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/fs/squashfs/sqfs_dir.c b/fs/squashfs/sqfs_dir.c
index ed83c90682f..3908d1380b3 100644
--- a/fs/squashfs/sqfs_dir.c
+++ b/fs/squashfs/sqfs_dir.c
@@ -32,6 +32,7 @@ int sqfs_dir_offset(void *dir_i, u32 *m_list, int m_count)
 	struct squashfs_base_inode *base = dir_i;
 	struct squashfs_ldir_inode *ldir;
 	struct squashfs_dir_inode *dir;
+	u64 table_size, res;
 	u32 start_block;
 	int j, offset;
 
@@ -51,20 +52,49 @@ int sqfs_dir_offset(void *dir_i, u32 *m_list, int m_count)
 		return -EINVAL;
 	}
 
-	if (offset < 0)
+	/*
+	 * 'offset' is an offset into a decompressed metadata block, so it can
+	 * never address past the end of one.
+	 */
+	if (offset >= SQFS_METADATA_BLOCK_SIZE)
 		return -EINVAL;
 
+	if (m_count < 1)
+		return -EINVAL;
+
+	/* The caller's directory table holds m_count decompressed blocks. */
+	table_size = (u64)m_count * SQFS_METADATA_BLOCK_SIZE;
+
 	for (j = 0; j < m_count; j++) {
 		if (m_list[j] == start_block)
-			return (++j * SQFS_METADATA_BLOCK_SIZE) + offset;
+			break;
 	}
 
-	if (start_block == 0)
-		return offset;
+	if (j < m_count) {
+		/*
+		 * m_list[j] is the position of the metadata block following
+		 * block j, so a match means the directory starts in block
+		 * j + 1.
+		 */
+		res = (u64)(j + 1) * SQFS_METADATA_BLOCK_SIZE + offset;
+	} else if (start_block == 0) {
+		res = offset;
+	} else {
+		printf("Error: invalid inode reference to directory table.\n");
+		return -EINVAL;
+	}
 
-	printf("Error: invalid inode reference to directory table.\n");
+	/*
+	 * Callers use the return value to index the directory table and read a
+	 * directory header from it, so the whole header must lie inside the
+	 * table.
+	 */
+	if (res + SQFS_DIR_HEADER_SIZE > table_size) {
+		printf("Error: inode points past the end of the directory table.\n");
+		return -EINVAL;
+	}
 
-	return -EINVAL;
+	return res;
 }
 
 bool sqfs_is_empty_dir(void *dir_i)
-- 
2.50.1 (Apple Git-155)