current/SOURCES/buffer-issue.patch

From: Phillip Lougher <phillip@squashfs.org.uk>
Date: Sat, 24 Nov 2012 02:34:48 +0000 (+0000)
Subject: unsquashfs: fix CVE-2012-4025
X-Git-Url: http://squashfs.git.sourceforge.net/git/gitweb.cgi?p=squashfs%2Fsquashfs;a=commitdiff_plain;h=8515b3d420f502c5c0236b86e2d6d7e3b23c190e;hp=19c38fba0be1ce949ab44310d7f49887576cc123

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>
---

diff -Nurp squashfs4.2-fix1/squashfs-tools/squashfs_fs.h squashfs4.2-fix2/squashfs-tools/squashfs_fs.h
--- squashfs4.2-fix1/squashfs-tools/squashfs_fs.h       2011-02-11 17:49:24.000000000 +0200
+++ squashfs4.2-fix2/squashfs-tools/squashfs_fs.h       2013-01-03 18:26:42.966614515 +0200
@@ -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
diff -Nurp squashfs4.2-fix1/squashfs-tools/unsquashfs.c squashfs4.2-fix2/squashfs-tools/unsquashfs.c
--- squashfs4.2-fix1/squashfs-tools/unsquashfs.c        2013-01-03 18:25:58.835643093 +0200
+++ squashfs4.2-fix2/squashfs-tools/unsquashfs.c        2013-01-03 18:26:42.986614959 +0200
@@ -31,6 +31,7 @@
 
 #include <sys/sysinfo.h>
 #include <sys/types.h>
+#include <limits.h>
 
 struct cache *fragment_cache, *data_cache;
 struct queue *to_reader, *to_deflate, *to_writer, *from_writer;
@@ -136,6 +137,24 @@ 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));
@@ -143,6 +162,10 @@ struct queue *queue_init(int size)
        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");
@@ -1814,7 +1837,7 @@ void initialise_threads(int fragment_buf
 {
        int i;
        sigset_t sigmask, old_mask;
-       int all_buffers_size = fragment_buffer_size + data_buffer_size;
+       int all_buffers_size;
 
        sigemptyset(&sigmask);
        sigaddset(&sigmask, SIGINT);
@@ -1850,6 +1873,15 @@ void initialise_threads(int fragment_buf
                EXIT_UNSQUASH("Out of memory allocating thread descriptors\n");
        deflator_thread = &thread[3];
 
+        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);
        to_writer = queue_init(1000);
@@ -1949,6 +1981,31 @@ void progress_bar(long long current, lon
        fflush(stdout);
 }
 
+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-CVS (2011/04/12)\n");\
@@ -2031,8 +2088,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);
@@ -2045,8 +2102,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);
@@ -2170,8 +2227,41 @@ options:
        block_size = sBlk.s.block_size;
        block_log = sBlk.s.block_log;
 
-       fragment_buffer_size <<= 20 - block_log;
-       data_buffer_size <<= 20 - 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.
+        *
+        * In doing so, check that the user supplied values do not
+        * overflow a signed int
+        */
+       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);
1	From: Phillip Lougher <phillip@squashfs.org.uk>
2	Date: Sat, 24 Nov 2012 02:34:48 +0000 (+0000)
3	Subject: unsquashfs: fix CVE-2012-4025
4	X-Git-Url: http://squashfs.git.sourceforge.net/git/gitweb.cgi?p=squashfs%2Fsquashfs;a=commitdiff_plain;h=8515b3d420f502c5c0236b86e2d6d7e3b23c190e;hp=19c38fba0be1ce949ab44310d7f49887576cc123
5
6	unsquashfs: fix CVE-2012-4025
7
8	Fix integer overflow exploit in queue_init() leading to heap overflow.
9
10	This was a complex exploit to fix because analysis of the code showed
11	there were multiple ways this exploit could be triggered, in addition
12	to the exploit vector documented in the CVE.
13
14	Exploit:
15
16	Data_buffer_size and fragment_buffer_size are added together and
17	passed to the queue_init function defined as:
18
19	struct queue *queue_init(int size)
20
21	Unfortunately, data_buffer_size and fragment_buffer_size are ints, and
22	contain values supplied by the user. The addition of these values can
23	overflow, leading to a negative size passed to queue_init()
24
25	In queue_init, space is allocated by
26
27	queue->data = malloc(sizeof(void ) (size + 1));
28
29	Given a sufficiently large negative size, (size + 1) * sizeof(void *)
30	can overflow leading to a small positive number, leading to a small
31	amount of buffer space being created.
32
33	In queue_get, the read pointer is moved through the buffer space by
34
35	queue->readp = (queue->readp + 1) % queue->size;
36
37	The negative sign of size is ignored, leading to readp going beyond the
38	allocated buffer space.
39
40	Analysis of exploit:
41
42	The exploit above is subtle, and centres around passing a negative
43	number to queue_init() due to integer overflow. This exploit can be
44	fixed by adding a check for negative size passed into queue_init().
45
46	However, further analysis of the exploit shows it can be triggered much
47	more easily. Data_buffer_size and fragment_buffer_size added together
48	can produce a value less than MAX_INT, which when passed into
49	queue_init() is not negative. However, this less than MAX_INT value can
50	overflow in the malloc calculation when multipled by sizeof(void *),
51	leading to a small positive integer. This leads to the situation where
52	the buffer allocated is smaller than size, and subsequent heap overflow.
53
54	Checking for a negative number in queue_init() is insufficient to
55	determine if overflow has or will occur. In fact any one time
56	"spot checks" on size is fraught with the problem it can fail to detect
57	previous overflow (i.e. previous overflow which has resulted in a small
58	positive number is impossible to distinguish from a valid small positive
59	number), and it cannot detect if overflow will occur later.
60
61	Due to this the approach to the fix is to check every operation
62	performed on data_buffer_size, fragment_buffer_size and any values
63	derived from them. All calculations are checked for potential overflow
64	before they are performed.
65
66	This has the following advantages:
67
68	1. It allows the code to trap exactly where the overflow takes place,
69	leading to more descriptive error messages.
70
71	2. It ensures overflow situations do not feed through to later
72	calculations making it more difficult to determine if overflow has
73	occurred.
74
75	3. It ensures too large values for data and fragment buffer sizes are
76	correctly rejected even if they don't trigger the exploits.
77
78	Signed-off-by: Phillip Lougher <phillip@squashfs.org.uk>
79	---
80
81	diff -Nurp squashfs4.2-fix1/squashfs-tools/squashfs_fs.h squashfs4.2-fix2/squashfs-tools/squashfs_fs.h
82	--- squashfs4.2-fix1/squashfs-tools/squashfs_fs.h 2011-02-11 17:49:24.000000000 +0200
83	+++ squashfs4.2-fix2/squashfs-tools/squashfs_fs.h 2013-01-03 18:26:42.966614515 +0200
84	@@ -39,6 +39,7 @@
85	#define SQUASHFS_FILE_LOG 17
86
87	#define SQUASHFS_FILE_MAX_SIZE 1048576
88	+#define SQUASHFS_FILE_MAX_LOG 20
89
90	/* Max number of uids and gids */
91	#define SQUASHFS_IDS 65536
92	diff -Nurp squashfs4.2-fix1/squashfs-tools/unsquashfs.c squashfs4.2-fix2/squashfs-tools/unsquashfs.c
93	--- squashfs4.2-fix1/squashfs-tools/unsquashfs.c 2013-01-03 18:25:58.835643093 +0200
94	+++ squashfs4.2-fix2/squashfs-tools/unsquashfs.c 2013-01-03 18:26:42.986614959 +0200
95	@@ -31,6 +31,7 @@
96
97	#include <sys/sysinfo.h>
98	#include <sys/types.h>
99	+#include <limits.h>
100
101	struct cache fragment_cache, data_cache;
102	struct queue to_reader, to_deflate, to_writer, from_writer;
103	@@ -136,6 +137,24 @@ void sigalrm_handler()
104	}
105
106
107	+int add_overflow(int a, int b)
108	+{
109	+ return (INT_MAX - a) < b;
110	+}
111	+
112	+
113	+int shift_overflow(int a, int shift)
114	+{
115	+ return (INT_MAX >> shift) < a;
116	+}
117	+
118	+
119	+int multiply_overflow(int a, int multiplier)
120	+{
121	+ return (INT_MAX / multiplier) < a;
122	+}
123	+
124	+
125	struct queue *queue_init(int size)
126	{
127	struct queue *queue = malloc(sizeof(struct queue));
128	@@ -143,6 +162,10 @@ struct queue *queue_init(int size)
129	if(queue == NULL)
130	EXIT_UNSQUASH("Out of memory in queue_init\n");
131
132	+ if(add_overflow(size, 1) \|\|
133	+ multiply_overflow(size + 1, sizeof(void *)))
134	+ EXIT_UNSQUASH("Size too large in queue_init\n");
135	+
136	queue->data = malloc(sizeof(void ) (size + 1));
137	if(queue->data == NULL)
138	EXIT_UNSQUASH("Out of memory in queue_init\n");
139	@@ -1814,7 +1837,7 @@ void initialise_threads(int fragment_buf
140	{
141	int i;
142	sigset_t sigmask, old_mask;
143	- int all_buffers_size = fragment_buffer_size + data_buffer_size;
144	+ int all_buffers_size;
145
146	sigemptyset(&sigmask);
147	sigaddset(&sigmask, SIGINT);
148	@@ -1850,6 +1873,15 @@ void initialise_threads(int fragment_buf
149	EXIT_UNSQUASH("Out of memory allocating thread descriptors\n");
150	deflator_thread = &thread[3];
151
152	+ if(add_overflow(fragment_buffer_size, data_buffer_size))
153	+ EXIT_UNSQUASH("Data and fragment queues combined are"
154	+ " too large\n");
155	+
156	+ all_buffers_size = fragment_buffer_size + data_buffer_size;
157	+
158	+ if(add_overflow(all_buffers_size, all_buffers_size))
159	+ EXIT_UNSQUASH("Data and fragment queues combined are"
160	+ " too large\n");
161	to_reader = queue_init(all_buffers_size);
162	to_deflate = queue_init(all_buffers_size);
163	to_writer = queue_init(1000);
164	@@ -1949,6 +1981,31 @@ void progress_bar(long long current, lon
165	fflush(stdout);
166	}
167
168	+int parse_number(char arg, int res)
169	+{
170	+ char *b;
171	+ long number = strtol(arg, &b, 10);
172	+
173	+ /* check for trailing junk after number */
174	+ if(*b != '\0')
175	+ return 0;
176	+
177	+ /* check for strtol underflow or overflow in conversion */
178	+ if(number == LONG_MIN \|\| number == LONG_MAX)
179	+ return 0;
180	+
181	+ /* reject negative numbers as invalid */
182	+ if(number < 0)
183	+ return 0;
184	+
185	+ /* check if long result will overflow signed int */
186	+ if(number > INT_MAX)
187	+ return 0;
188	+
189	+ *res = number;
190	+ return 1;
191	+}
192	+
193
194	#define VERSION() \
195	printf("unsquashfs version 4.2-CVS (2011/04/12)\n");\
196	@@ -2031,8 +2088,8 @@ int main(int argc, char *argv[])
197	} else if(strcmp(argv[i], "-data-queue") == 0 \|\|
198	strcmp(argv[i], "-da") == 0) {
199	if((++i == argc) \|\|
200	- (data_buffer_size = strtol(argv[i], &b,
201	- 10), *b != '\0')) {
202	+ !parse_number(argv[i],
203	+ &data_buffer_size)) {
204	ERROR("%s: -data-queue missing or invalid "
205	"queue size\n", argv[0]);
206	exit(1);
207	@@ -2045,8 +2102,8 @@ int main(int argc, char *argv[])
208	} else if(strcmp(argv[i], "-frag-queue") == 0 \|\|
209	strcmp(argv[i], "-fr") == 0) {
210	if((++i == argc) \|\|
211	- (fragment_buffer_size = strtol(argv[i],
212	- &b, 10), *b != '\0')) {
213	+ !parse_number(argv[i],
214	+ &fragment_buffer_size)) {
215	ERROR("%s: -frag-queue missing or invalid "
216	"queue size\n", argv[0]);
217	exit(1);
218	@@ -2170,8 +2227,41 @@ options:
219	block_size = sBlk.s.block_size;
220	block_log = sBlk.s.block_log;
221
222	- fragment_buffer_size <<= 20 - block_log;
223	- data_buffer_size <<= 20 - block_log;
224	+ /*
225	+ * Sanity check block size and block log.
226	+ *
227	+ * Check they're within correct limits
228	+ */
229	+ if(block_size > SQUASHFS_FILE_MAX_SIZE \|\|
230	+ block_log > SQUASHFS_FILE_MAX_LOG)
231	+ EXIT_UNSQUASH("Block size or block_log too large."
232	+ " File system is corrupt.\n");
233	+
234	+ /*
235	+ * Check block_size and block_log match
236	+ */
237	+ if(block_size != (1 << block_log))
238	+ EXIT_UNSQUASH("Block size and block_log do not match."
239	+ " File system is corrupt.\n");
240	+
241	+ /*
242	+ * convert from queue size in Mbytes to queue size in
243	+ * blocks.
244	+ *
245	+ * In doing so, check that the user supplied values do not
246	+ * overflow a signed int
247	+ */
248	+ if(shift_overflow(fragment_buffer_size, 20 - block_log))
249	+ EXIT_UNSQUASH("Fragment queue size is too large\n");
250	+ else
251	+ fragment_buffer_size <<= 20 - block_log;
252	+
253	+ if(shift_overflow(data_buffer_size, 20 - block_log))
254	+ EXIT_UNSQUASH("Data queue size is too large\n");
255	+ else
256	+ data_buffer_size <<= 20 - block_log;
257	+
258	+
259	initialise_threads(fragment_buffer_size, data_buffer_size);
260
261	fragment_data = malloc(block_size);