Do bounds checking in metadata reader

In all cases where metadata blocks are read, we can roughly (in some
cases even preciesly) say in what range those metadata blocks will be,
so it makes sense to throw an error if an attempt is made to wander
outside this range.

Furthermore, when reading from an uncompressed block, it is more reasonable
to check against the actual block bounds than to padd it with 0 bytes.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>

8 files changed, 88 insertions, 31 deletions

diff --git a/include/highlevel.h b/include/highlevel.h
index f8c5bfb..a455234 100644
--- a/include/highlevel.h
+++ b/include/highlevel.h
@@ -50,7 +50,8 @@ int sqfs_write_table(int outfd, sqfs_super_t *super, compressor_t *cmp,
 		     const void *data, size_t table_size, uint64_t *start);
 
 void *sqfs_read_table(int fd, compressor_t *cmp, size_t table_size,
-		      uint64_t location);
+		      uint64_t location, uint64_t lower_limit,
+		      uint64_t upper_limit);
 
 /*
   High level helper function to serialize an entire file system tree to
diff --git a/include/meta_reader.h b/include/meta_reader.h
index 8369ad3..d5628af 100644
--- a/include/meta_reader.h
+++ b/include/meta_reader.h
@@ -15,8 +15,13 @@
 typedef struct meta_reader_t meta_reader_t;
 
 /* Create a meta data reader using a given compressor to extract data.
-   Internally prints error message to stderr on failure. */
-meta_reader_t *meta_reader_create(int fd, compressor_t *cmp);
+   Internally prints error message to stderr on failure.
+
+   Start offset and limit can be specified to do bounds checking against
+   a subregion of the filesystem image.
+*/
+meta_reader_t *meta_reader_create(int fd, compressor_t *cmp,
+				  uint64_t start, uint64_t limit);
 
 void meta_reader_destroy(meta_reader_t *m);
 
diff --git a/lib/sqfs/data_reader.c b/lib/sqfs/data_reader.c
index 9d24745..4ad6266 100644
--- a/lib/sqfs/data_reader.c
+++ b/lib/sqfs/data_reader.c
@@ -143,6 +143,8 @@ data_reader_t *data_reader_create(int fd, sqfs_super_t *super,
 	}
 
 	data->frag = sqfs_read_table(fd, cmp, size,
+				     super->fragment_table_start,
+				     super->directory_table_start,
 				     super->fragment_table_start);
 	if (data->frag == NULL) {
 		free(data);
diff --git a/lib/sqfs/deserialize_fstree.c b/lib/sqfs/deserialize_fstree.c
index 050a1df..11670e1 100644
--- a/lib/sqfs/deserialize_fstree.c
+++ b/lib/sqfs/deserialize_fstree.c
@@ -201,19 +201,26 @@ static int fill_dir(meta_reader_t *ir, meta_reader_t *dr, tree_node_t *root,
 int deserialize_fstree(fstree_t *out, sqfs_super_t *super, compressor_t *cmp,
 		       int fd, int flags)
 {
+	uint64_t block_start, limit;
 	sqfs_inode_generic_t *root;
 	meta_reader_t *ir, *dr;
-	uint64_t block_start;
 	xattr_reader_t *xr;
 	id_table_t idtbl;
 	int status = -1;
 	size_t offset;
 
-	ir = meta_reader_create(fd, cmp);
+	ir = meta_reader_create(fd, cmp, super->inode_table_start,
+				super->directory_table_start);
 	if (ir == NULL)
 		return -1;
 
-	dr = meta_reader_create(fd, cmp);
+	limit = super->id_table_start;
+	if (super->export_table_start < limit)
+		limit = super->export_table_start;
+	if (super->fragment_table_start < limit)
+		limit = super->fragment_table_start;
+
+	dr = meta_reader_create(fd, cmp, super->directory_table_start, limit);
 	if (dr == NULL)
 		goto out_ir;
 
diff --git a/lib/sqfs/id_table_read.c b/lib/sqfs/id_table_read.c
index a310fd4..ccb0fc8 100644
--- a/lib/sqfs/id_table_read.c
+++ b/lib/sqfs/id_table_read.c
@@ -15,6 +15,7 @@
 int id_table_read(id_table_t *tbl, int fd, sqfs_super_t *super,
 		  compressor_t *cmp)
 {
+	uint64_t upper_limit, lower_limit;
 	size_t i;
 
 	if (tbl->ids != NULL) {
@@ -29,10 +30,24 @@ int id_table_read(id_table_t *tbl, int fd, sqfs_super_t *super,
 		return -1;
 	}
 
+	upper_limit = super->id_table_start;
+	lower_limit = super->directory_table_start;
+
+	if (super->fragment_table_start > lower_limit &&
+	    super->fragment_table_start < upper_limit) {
+		lower_limit = super->fragment_table_start;
+	}
+
+	if (super->export_table_start > lower_limit &&
+	    super->export_table_start < upper_limit) {
+		lower_limit = super->export_table_start;
+	}
+
 	tbl->num_ids = super->id_count;
 	tbl->max_ids = super->id_count;
 	tbl->ids = sqfs_read_table(fd, cmp, tbl->num_ids * sizeof(uint32_t),
-				   super->id_table_start);
+				   super->id_table_start, lower_limit,
+				   upper_limit);
 	if (tbl->ids == NULL)
 		return -1;
 
diff --git a/lib/sqfs/meta_reader.c b/lib/sqfs/meta_reader.c
index 711e8b2..5e71951 100644
--- a/lib/sqfs/meta_reader.c
+++ b/lib/sqfs/meta_reader.c
@@ -15,6 +15,10 @@
 #include <stdio.h>
 
 struct meta_reader_t {
+	uint64_t start;
+	uint64_t limit;
+	size_t data_used;
+
 	/* The location of the current block in the image */
 	uint64_t block_offset;
 
@@ -37,7 +41,8 @@ struct meta_reader_t {
 	uint8_t scratch[SQFS_META_BLOCK_SIZE];
 };
 
-meta_reader_t *meta_reader_create(int fd, compressor_t *cmp)
+meta_reader_t *meta_reader_create(int fd, compressor_t *cmp,
+				  uint64_t start, uint64_t limit)
 {
 	meta_reader_t *m = calloc(1, sizeof(*m));
 
@@ -46,6 +51,8 @@ meta_reader_t *meta_reader_create(int fd, compressor_t *cmp)
 		return NULL;
 	}
 
+	m->start = start;
+	m->limit = limit;
 	m->fd = fd;
 	m->cmp = cmp;
 	return m;
@@ -63,12 +70,13 @@ int meta_reader_seek(meta_reader_t *m, uint64_t block_start, size_t offset)
 	ssize_t ret;
 	size_t size;
 
-	if (offset >= sizeof(m->data)) {
-		fputs("Tried to seek past end of meta data block.\n", stderr);
-		return -1;
-	}
+	if (block_start < m->start || block_start >= m->limit)
+		goto fail_range;
 
 	if (block_start == m->block_offset) {
+		if (offset >= m->data_used)
+			goto fail_offset;
+
 		m->offset = offset;
 		return 0;
 	}
@@ -82,17 +90,11 @@ int meta_reader_seek(meta_reader_t *m, uint64_t block_start, size_t offset)
 	compressed = (header & 0x8000) == 0;
 	size = header & 0x7FFF;
 
-	if (size > sizeof(m->data)) {
-		fputs("found meta data block larger than maximum size\n",
-		      stderr);
-		return -1;
-	}
+	if (size > sizeof(m->data))
+		goto fail_too_large;
 
-	m->block_offset = block_start;
-	m->offset = offset;
-	m->next_block = block_start + size + 2;
-
-	memset(m->data, 0, sizeof(m->data));
+	if ((block_start + 2 + size) > m->limit)
+		goto fail_block_bounds;
 
 	if (read_data_at("reading meta data block", block_start + 2,
 			 m->fd, m->data, size)) {
@@ -109,12 +111,32 @@ int meta_reader_seek(meta_reader_t *m, uint64_t block_start, size_t offset)
 		}
 
 		memcpy(m->data, m->scratch, ret);
-
-		if ((size_t)ret < sizeof(m->data))
-			memset(m->data + ret, 0, sizeof(m->data) - ret);
+		m->data_used = ret;
+	} else {
+		m->data_used = size;
 	}
 
+	if (offset >= m->data_used)
+		goto fail_offset;
+
+	m->block_offset = block_start;
+	m->next_block = block_start + size + 2;
+	m->offset = offset;
 	return 0;
+fail_block_bounds:
+	fputs("found metadata block that exceeds filesystem bounds.\n",
+	      stderr);
+	return -1;
+fail_too_large:
+	fputs("found metadata block larger than maximum size.\n", stderr);
+	return -1;
+fail_offset:
+	fputs("Tried to seek past end of metadata block.\n", stderr);
+	return -1;
+fail_range:
+	fputs("Tried to read meta data block past filesystem bounds.\n",
+	      stderr);
+	return -1;
 }
 
 void meta_reader_get_position(meta_reader_t *m, uint64_t *block_start,
@@ -129,12 +151,12 @@ int meta_reader_read(meta_reader_t *m, void *data, size_t size)
 	size_t diff;
 
 	while (size != 0) {
-		diff = sizeof(m->data) - m->offset;
+		diff = m->data_used - m->offset;
 
 		if (diff == 0) {
 			if (meta_reader_seek(m, m->next_block, 0))
 				return -1;
-			diff = sizeof(m->data);
+			diff = m->data_used;
 		}
 
 		if (diff > size)
diff --git a/lib/sqfs/read_table.c b/lib/sqfs/read_table.c
index 12e06b4..6efd401 100644
--- a/lib/sqfs/read_table.c
+++ b/lib/sqfs/read_table.c
@@ -15,7 +15,8 @@
 #include <stdio.h>
 
 void *sqfs_read_table(int fd, compressor_t *cmp, size_t table_size,
-		      uint64_t location)
+		      uint64_t location, uint64_t lower_limit,
+		      uint64_t upper_limit)
 {
 	size_t diff, block_count, list_size, blk_idx = 0;
 	uint64_t start, *locations;
@@ -48,7 +49,7 @@ void *sqfs_read_table(int fd, compressor_t *cmp, size_t table_size,
 	}
 
 	/* Read the actual data */
-	m = meta_reader_create(fd, cmp);
+	m = meta_reader_create(fd, cmp, lower_limit, upper_limit);
 	if (m == NULL)
 		goto fail_idx;
 
diff --git a/lib/sqfs/xattr_reader.c b/lib/sqfs/xattr_reader.c
index 99bd389..7a881c7 100644
--- a/lib/sqfs/xattr_reader.c
+++ b/lib/sqfs/xattr_reader.c
@@ -359,11 +359,15 @@ xattr_reader_t *xattr_reader_create(int sqfsfd, sqfs_super_t *super,
 	if (get_id_block_locations(xr, sqfsfd, super))
 		goto fail;
 
-	xr->idrd = meta_reader_create(sqfsfd, cmp);
+	xr->idrd = meta_reader_create(sqfsfd, cmp,
+				      super->id_table_start,
+				      super->bytes_used);
 	if (xr->idrd == NULL)
 		goto fail;
 
-	xr->kvrd = meta_reader_create(sqfsfd, cmp);
+	xr->kvrd = meta_reader_create(sqfsfd, cmp,
+				      super->id_table_start,
+				      super->bytes_used);
 	if (xr->kvrd == NULL)
 		goto fail;