connection: Detect overflows in length field.

Submitted by Michal Srb on July 30, 2018, 3:45 p.m.

Details

Message ID 20180730154517.19886-1-msrb@suse.com
State New
Series "connection: Detect overflows in length field."
Headers show

Commit Message

Michal Srb July 30, 2018, 3:45 p.m.
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@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(-)

Patch hide | download patch | download mbox

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, ...)

Comments

Jann Horn Aug. 2, 2018, 6:43 p.m.
On Thu, Aug 2, 2018 at 8:37 PM Michal Srb <msrb@suse.com> wrote:
>
> 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@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;
> +}

This patch probably fixes the problem in practice, but AFAICS the
patched version still, in theory, contains undefined behavior. The C99
standard says: "If both the pointer operand and the result point to
elements of the same array object, or one past the last element of the
array object, the evaluation shall not produce an overflow; otherwise,
the behavior is undefined."
Can you please change the patch so that it compares lengths instead of
comparing potentially out-of-bounds pointers, thereby avoiding
undefined behavior?
Michal Srb Aug. 14, 2018, 11:07 a.m.
Hi,

Sorry for the long delay. I rewrote it using different method. IMO it is
cleaner now too.

There were two kinds of overflows - integer overflow when rounding the
length and pointer/integer overflow when adding the length to the `p`.
So I split it into two patches + one with tests.

Michal Srb (3):
  tests: Demarshalling of very long array/string lengths.
  connection: Prevent integer overflow in DIV_ROUNDUP.
  connection: Prevent pointer overflow from large lengths.

 src/connection.c        | 31 +++++++++++++++++++------------
 tests/connection-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 12 deletions(-)