unsquashfs: fix CVE-2012-4025

Fix integer overflow exploit in queue_init() leading to heap overflow.

This was a complex exploit to fix because analysis of the code showed
there were multiple ways this exploit could be triggered, in addition
to the exploit vector documented in the CVE.

Exploit:

Data_buffer_size and fragment_buffer_size are added together and
passed to the queue_init function defined as:

struct queue *queue_init(int size)

Unfortunately, data_buffer_size and fragment_buffer_size are ints, and
contain values supplied by the user.  The addition of these values can
overflow, leading to a negative size passed to queue_init()

In queue_init, space is allocated by

queue->data = malloc(sizeof(void *) * (size + 1));

Given a sufficiently large negative size, (size + 1) * sizeof(void *)
can overflow leading to a small positive number, leading to a small
amount of buffer space being created.

In queue_get, the read pointer is moved through the buffer space by

queue->readp = (queue->readp + 1) % queue->size;

The negative sign of size is ignored, leading to readp going beyond the
allocated buffer space.

Analysis of exploit:

The exploit above is subtle, and centres around passing a negative
number to queue_init() due to integer overflow.  This exploit can be
fixed by adding a check for negative size passed into queue_init().

However, further analysis of the exploit shows it can be triggered much
more easily.  Data_buffer_size and fragment_buffer_size added together
can produce a value less than MAX_INT, which when passed into
queue_init() is not negative.  However, this less than MAX_INT value can
overflow in the malloc calculation when multipled by sizeof(void *),
leading to a small positive integer.  This leads to the situation where
the buffer allocated is smaller than size, and subsequent heap overflow.

Checking for a negative number in queue_init() is insufficient to
determine if overflow has or will occur.  In fact any one time
"spot checks" on size is fraught with the problem it can fail to detect
previous overflow (i.e. previous overflow which has resulted in a small
positive number is impossible to distinguish from a valid small positive
number), and it cannot detect if overflow will occur later.

Due to this the approach to the fix is to check every operation
performed on data_buffer_size, fragment_buffer_size and any values
derived from them.  All calculations are checked for potential overflow
before they are performed.

This has the following advantages:

1. It allows the code to trap exactly where the overflow takes place,
   leading to more descriptive error messages.

2. It ensures overflow situations do not feed through to later
   calculations making it more difficult to determine if overflow has
   occurred.

3. It ensures too large values for data and fragment buffer sizes are
   correctly rejected even if they don't trigger the exploits.

Signed-off-by: Phillip Lougher <phillip@squashfs.org.uk>

squashfs-tools/squashfs_fs.h

@@ -39,6 +39,7 @@
 #define SQUASHFS_FILE_LOG		17
 
 #define SQUASHFS_FILE_MAX_SIZE		1048576
+#define SQUASHFS_FILE_MAX_LOG		20
 
 /* Max number of uids and gids */
 #define SQUASHFS_IDS			65536

squashfs-tools/unsquashfs.c

@@ -34,6 +34,7 @@
 #include <sys/types.h>
 #include <sys/time.h>
 #include <sys/resource.h>
+#include <limits.h>
 
 struct cache *fragment_cache, *data_cache;
 struct queue *to_reader, *to_deflate, *to_writer, *from_writer;
@@ -139,13 +140,35 @@ void sigalrm_handler()
 }
 
 
+int add_overflow(int a, int b)
+{
+	return (INT_MAX - a) < b;
+}
+
+
+int shift_overflow(int a, int shift)
+{
+	return (INT_MAX >> shift) < a;
+}
+
+
+int multiply_overflow(int a, int multiplier)
+{
+	return (INT_MAX / multiplier) < a;
+}
+
+
 struct queue *queue_init(int size)
 {
 	struct queue *queue = malloc(sizeof(struct queue));
 
 	if(queue == NULL)
 		EXIT_UNSQUASH("Out of memory in queue_init\n");
 
+	if(add_overflow(size, 1) ||
+				multiply_overflow(size + 1, sizeof(void *)))
+		EXIT_UNSQUASH("Size too large in queue_init\n");
+
 	queue->data = malloc(sizeof(void *) * (size + 1));
 	if(queue->data == NULL)
 		EXIT_UNSQUASH("Out of memory in queue_init\n");
@@ -2015,13 +2038,30 @@ void initialise_threads(int fragment_buffer_size, int data_buffer_size)
 	 * allocate to_reader, to_deflate and to_writer queues.  Set based on
 	 * open file limit and cache size, unless open file limit is unlimited,
 	 * in which case set purely based on cache limits
+	 *
+	 * In doing so, check that the user supplied values do not overflow
+	 * a signed int
 	 */
 	if (max_files != -1) {
+		if(add_overflow(data_buffer_size, max_files) ||
+				add_overflow(data_buffer_size, max_files * 2))
+			EXIT_UNSQUASH("Data queue size is too large\n");
+
 		to_reader = queue_init(max_files + data_buffer_size);
 		to_deflate = queue_init(max_files + data_buffer_size);
 		to_writer = queue_init(max_files * 2 + data_buffer_size);
 	} else {
-		int all_buffers_size = fragment_buffer_size + data_buffer_size;
+		int all_buffers_size;
+
+		if(add_overflow(fragment_buffer_size, data_buffer_size))
+			EXIT_UNSQUASH("Data and fragment queues combined are"
+							" too large\n");
+
+		all_buffers_size = fragment_buffer_size + data_buffer_size;
+
+		if(add_overflow(all_buffers_size, all_buffers_size))
+			EXIT_UNSQUASH("Data and fragment queues combined are"
+							" too large\n");
 
 		to_reader = queue_init(all_buffers_size);
 		to_deflate = queue_init(all_buffers_size);
@@ -2126,8 +2166,34 @@ void progress_bar(long long current, long long max, int columns)
 }
 
 
+int parse_number(char *arg, int *res)
+{
+	char *b;
+	long number = strtol(arg, &b, 10);
+
+	/* check for trailing junk after number */
+	if(*b != '\0')
+		return 0;
+
+	/* check for strtol underflow or overflow in conversion */
+	if(number == LONG_MIN || number == LONG_MAX)
+		return 0;
+
+	/* reject negative numbers as invalid */
+	if(number < 0)
+		return 0;
+
+	/* check if long result will overflow signed int */
+	if(number > INT_MAX)
+		return 0;
+
+	*res = number;
+	return 1;
+}
+
+
 #define VERSION() \
-	printf("unsquashfs version 4.2-git (2012/11/21)\n");\
+	printf("unsquashfs version 4.2-git (2012/11/24)\n");\
 	printf("copyright (C) 2012 Phillip Lougher "\
 		"<phillip@squashfs.org.uk>\n\n");\
     	printf("This program is free software; you can redistribute it and/or"\
@@ -2207,8 +2273,8 @@ int main(int argc, char *argv[])
 		} else if(strcmp(argv[i], "-data-queue") == 0 ||
 					 strcmp(argv[i], "-da") == 0) {
 			if((++i == argc) ||
-					(data_buffer_size = strtol(argv[i], &b,
-					 10), *b != '\0')) {
+					!parse_number(argv[i],
+						&data_buffer_size)) {
 				ERROR("%s: -data-queue missing or invalid "
 					"queue size\n", argv[0]);
 				exit(1);
@@ -2221,8 +2287,8 @@ int main(int argc, char *argv[])
 		} else if(strcmp(argv[i], "-frag-queue") == 0 ||
 					strcmp(argv[i], "-fr") == 0) {
 			if((++i == argc) ||
-					(fragment_buffer_size = strtol(argv[i],
-					 &b, 10), *b != '\0')) {
+					!parse_number(argv[i],
+						&fragment_buffer_size)) {
 				ERROR("%s: -frag-queue missing or invalid "
 					"queue size\n", argv[0]);
 				exit(1);
@@ -2346,12 +2412,40 @@ int main(int argc, char *argv[])
 	block_size = sBlk.s.block_size;
 	block_log = sBlk.s.block_log;
 
+	/*
+	 * Sanity check block size and block log.
+	 *
+	 * Check they're within correct limits
+	 */
+	if(block_size > SQUASHFS_FILE_MAX_SIZE ||
+					block_log > SQUASHFS_FILE_MAX_LOG)
+		EXIT_UNSQUASH("Block size or block_log too large."
+			"  File system is corrupt.\n");
+
+	/*
+	 * Check block_size and block_log match
+	 */
+	if(block_size != (1 << block_log))
+		EXIT_UNSQUASH("Block size and block_log do not match."
+			"  File system is corrupt.\n");
+
 	/*
 	 * convert from queue size in Mbytes to queue size in
-	 * blocks
+	 * blocks.
+	 *
+	 * In doing so, check that the user supplied values do not
+	 * overflow a signed int
 	 */
-	fragment_buffer_size <<= 20 - block_log;
-	data_buffer_size <<= 20 - block_log;
+	if(shift_overflow(fragment_buffer_size, 20 - block_log))
+		EXIT_UNSQUASH("Fragment queue size is too large\n");
+	else
+		fragment_buffer_size <<= 20 - block_log;
+
+	if(shift_overflow(data_buffer_size, 20 - block_log))
+		EXIT_UNSQUASH("Data queue size is too large\n");
+	else
+		data_buffer_size <<= 20 - block_log;
+
 	initialise_threads(fragment_buffer_size, data_buffer_size);
 
 	fragment_data = malloc(block_size);