[v2,1/3] tests: Demarshalling of very long array/string lengths.

Submitted by Michal Srb on Aug. 21, 2018, 8:47 a.m.

Details

Message ID 20180821084729.32443-1-msrb@suse.com
State Accepted
Commit 581c62841f2215ee12a8d9af4e4c05d052c6a204
Headers show
Series "connection: Detect overflows in length field." ( rev: 2 ) in Wayland

Not browsing as part of any series.

Commit Message

Michal Srb Aug. 21, 2018, 8:47 a.m.
Attempting to demarshal message with array or string longer than its
body should return failure. Handling the length correctly is tricky when
it gets to near-UINT32_MAX values. Unexpected overflows can cause
crashes and other security issues.

These tests verify that demarshalling such message gives failure instead
of crash.

v2: Added consts, serialized opcode and size properly, updated style.
---
 tests/connection-test.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

Patch hide | download patch | download mbox

diff --git a/tests/connection-test.c b/tests/connection-test.c
index 157e1bc..018e2ac 100644
--- a/tests/connection-test.c
+++ b/tests/connection-test.c
@@ -533,6 +533,69 @@  TEST(connection_marshal_demarshal)
 	release_marshal_data(&data);
 }
 
+static void
+expected_fail_demarshal(struct marshal_data *data, const char *format,
+                        const 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] >> 16);
+
+	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);
+}
+
+/* These tests are verifying that the demarshaling code will gracefuly handle
+ * clients lying about string and array lengths and giving values near
+ * UINT32_MAX. Before fixes f7fdface and f5b9e3b9 this test would crash on
+ * 32bit systems.
+ */
+TEST(connection_demarshal_failures)
+{
+	struct marshal_data data;
+	unsigned int i;
+	uint32_t msg[3];
+
+	const uint32_t overflowing_values[] = {
+		/* Values very close to UINT32_MAX. Before f5b9e3b9 these
+		 * would cause integer overflow in DIV_ROUNDUP. */
+		0xffffffff, 0xfffffffe, 0xfffffffd, 0xfffffffc,
+
+		/* Values at various offsets from UINT32_MAX. Before f7fdface
+		 * these would overflow the "p" pointer on 32bit systems,
+		 * effectively subtracting the offset from it. It had good
+		 * chance to cause crash depending on what was stored at that
+		 * offset before "p". */
+		0xfffff000, 0xffffd000, 0xffffc000, 0xffffb000
+	};
+
+	setup_marshal_data(&data);
+
+	/* sender_id, does not matter */
+	msg[0] = 0;
+
+	/* (size << 16 | opcode), opcode is 0, does not matter */
+	msg[1] = sizeof(msg) << 16;
+
+	for (i = 0; i < ARRAY_LENGTH(overflowing_values); i++) {
+		/* length of the string or array */
+		msg[2] = overflowing_values[i];
+
+		expected_fail_demarshal(&data, "s", msg, EINVAL);
+		expected_fail_demarshal(&data, "a", msg, EINVAL);
+	}
+
+	release_marshal_data(&data);
+}
+
 TEST(connection_marshal_alot)
 {
 	struct marshal_data data;

Comments

On Tue, 21 Aug 2018 10:47:29 +0200
Michal Srb <msrb@suse.com> wrote:

> Attempting to demarshal message with array or string longer than its
> body should return failure. Handling the length correctly is tricky when
> it gets to near-UINT32_MAX values. Unexpected overflows can cause
> crashes and other security issues.
> 
> These tests verify that demarshalling such message gives failure instead
> of crash.
> 
> v2: Added consts, serialized opcode and size properly, updated style.
> ---
>  tests/connection-test.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 

Hi,

looks good!

Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

I also tested 32-bit builds in CI with and without the fixes, and those
worked as expected too.


Thanks,
pq

> diff --git a/tests/connection-test.c b/tests/connection-test.c
> index 157e1bc..018e2ac 100644
> --- a/tests/connection-test.c
> +++ b/tests/connection-test.c
> @@ -533,6 +533,69 @@ TEST(connection_marshal_demarshal)
>  	release_marshal_data(&data);
>  }
>  
> +static void
> +expected_fail_demarshal(struct marshal_data *data, const char *format,
> +                        const 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] >> 16);
> +
> +	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);
> +}
> +
> +/* These tests are verifying that the demarshaling code will gracefuly handle
> + * clients lying about string and array lengths and giving values near
> + * UINT32_MAX. Before fixes f7fdface and f5b9e3b9 this test would crash on
> + * 32bit systems.
> + */
> +TEST(connection_demarshal_failures)
> +{
> +	struct marshal_data data;
> +	unsigned int i;
> +	uint32_t msg[3];
> +
> +	const uint32_t overflowing_values[] = {
> +		/* Values very close to UINT32_MAX. Before f5b9e3b9 these
> +		 * would cause integer overflow in DIV_ROUNDUP. */
> +		0xffffffff, 0xfffffffe, 0xfffffffd, 0xfffffffc,
> +
> +		/* Values at various offsets from UINT32_MAX. Before f7fdface
> +		 * these would overflow the "p" pointer on 32bit systems,
> +		 * effectively subtracting the offset from it. It had good
> +		 * chance to cause crash depending on what was stored at that
> +		 * offset before "p". */
> +		0xfffff000, 0xffffd000, 0xffffc000, 0xffffb000
> +	};
> +
> +	setup_marshal_data(&data);
> +
> +	/* sender_id, does not matter */
> +	msg[0] = 0;
> +
> +	/* (size << 16 | opcode), opcode is 0, does not matter */
> +	msg[1] = sizeof(msg) << 16;
> +
> +	for (i = 0; i < ARRAY_LENGTH(overflowing_values); i++) {
> +		/* length of the string or array */
> +		msg[2] = overflowing_values[i];
> +
> +		expected_fail_demarshal(&data, "s", msg, EINVAL);
> +		expected_fail_demarshal(&data, "a", msg, EINVAL);
> +	}
> +
> +	release_marshal_data(&data);
> +}
> +
>  TEST(connection_marshal_alot)
>  {
>  	struct marshal_data data;