[PATCH] connection: Detect overflows in length field.

[Michal Srb]

The length field can be any uint32 value. Two kinds of overflows may
happen on 32 bit systems:

1) If the value is in range [UINT32_MAX-3, UINT32_MAX], the DIV_ROUNDUP
will turn it into 0. Then `next` equals `p` and so the big `length` is not
detected. But the wl_array will contain the original big value. Probably
leading to crashes it is used later.

2) The pointer may overflow if the `length` is sufficiently big. In that
case `next` will point to some memory before the beginning of the buffer
and the the check `next > end` is not triggered. Using `next` later can
crash.

Signed-off-by: Michal Srb <msrb at suse.com>
---

Note that the problem happens only on 32bit systems.

 src/connection.c        | 19 +++++++++++++++++--
 tests/connection-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index 294c521..b48f3a4 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -46,6 +46,21 @@
 
 #define DIV_ROUNDUP(n, a) ( ((n) + ((a) - 1)) / (a) )
 
+inline uint32_t *
+ptr_add_saturating(uint32_t *p, uint32_t length) {
+	// Make sure that rounding length up will not overflow.
+	if (length > UINT32_MAX - sizeof(uint32_t))
+		return (uint32_t*) INTPTR_MAX;
+
+	length = DIV_ROUNDUP(length, sizeof(uint32_t));
+
+	// Make sure that adding length to p will not overflow.
+	if (length * sizeof(uint32_t) > UINTPTR_MAX - (uintptr_t) p)
+		return (uint32_t*) INTPTR_MAX;
+
+	return p + length;
+}
+
 struct wl_buffer {
 	char data[4096];
 	uint32_t head, tail;
@@ -734,7 +749,7 @@ wl_connection_demarshal(struct wl_connection *connection,
 				break;
 			}
 
-			next = p + DIV_ROUNDUP(length, sizeof *p);
+			next = ptr_add_saturating(p, length);
 			if (next > end) {
 				wl_log("message too short, "
 				       "object (%d), message %s(%s)\n",
@@ -793,7 +808,7 @@ wl_connection_demarshal(struct wl_connection *connection,
 		case 'a':
 			length = *p++;
 
-			next = p + DIV_ROUNDUP(length, sizeof *p);
+			next = ptr_add_saturating(p, length);
 			if (next > end) {
 				wl_log("message too short, "
 				       "object (%d), message %s(%s)\n",
diff --git a/tests/connection-test.c b/tests/connection-test.c
index 157e1bc..ff064cf 100644
--- a/tests/connection-test.c
+++ b/tests/connection-test.c
@@ -456,6 +456,52 @@ TEST(connection_demarshal)
 	release_marshal_data(&data);
 }
 
+static void
+expected_fail_demarshal(struct marshal_data *data, const char *format, uint32_t *msg, int expected_error)
+{
+	struct wl_message message = { "test", format, NULL };
+	struct wl_closure *closure;
+	struct wl_map objects;
+	int size = msg[1];
+
+	assert(write(data->s[1], msg, size) == size);
+	assert(wl_connection_read(data->read_connection) == size);
+
+	wl_map_init(&objects, WL_MAP_SERVER_SIDE);
+	closure = wl_connection_demarshal(data->read_connection,
+					  size, &objects, &message);
+
+	assert(closure == NULL);
+	assert(errno == expected_error);
+}
+
+TEST(connection_demarshal_failures)
+{
+	struct marshal_data data;
+	uint32_t msg[10];
+
+	setup_marshal_data(&data);
+
+	// These need careful handling on 32bit systems.
+	uint32_t overflowing_values[] = {
+		0xffffffff, 0xfffffffe, 0xfffffffd, 0xfffffffc,
+		0xfffff000, 0xffffd000, 0xffffc000, 0xffffb000
+	};
+	for (unsigned int i = 0; i < ARRAY_LENGTH(overflowing_values); i++) {
+		msg[0] = 400200;
+		msg[1] = 24;
+		msg[2] = overflowing_values[i];
+		expected_fail_demarshal(&data, "s", msg, EINVAL);
+
+		msg[0] = 400200;
+		msg[1] = 24;
+		msg[2] = overflowing_values[i];
+		expected_fail_demarshal(&data, "a", msg, EINVAL);
+	}
+
+	release_marshal_data(&data);
+}
+
 static void
 marshal_demarshal(struct marshal_data *data,
 		  void (*func)(void), int size, const char *format, ...)
-- 
2.16.4