[2/2] connection: fix demarshal of invalid header

Submitted by Pekka Paalanen on March 6, 2019, 11:58 a.m.

Details

Message ID 20190306115839.10932-2-ppaalanen@gmail.com
State Accepted
Commit bace3cd819798571189671b68590adff3fd40997
Headers show
Series "Series without cover letter" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Pekka Paalanen March 6, 2019, 11:58 a.m.
From: Pekka Paalanen <pekka.paalanen@collabora.com>

The size argument to wl_connection_demarshal() is taken from the message by the
caller wl_client_connection_data(), therefore 'size' is untrusted data
controllable by a Wayland client. The size should always be at least the header
size, otherwise the header is invalid.

If the size is smaller than header size, it leads to reading past the end of
allocated memory. Furthermore if size is zero, wl_closure_init() changes
behaviour and leaves num_arrays uninitialized, leading to access of arbitrary
memory.

Check that 'size' fits at least the header. The space for arguments is already
properly checked.

This makes the request_bogus_size test free of errors under Valgrind.

Fixes: https://gitlab.freedesktop.org/wayland/wayland/issues/52

Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
---
 src/connection.c        | 8 ++++++++
 tests/connection-test.c | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/connection.c b/src/connection.c
index 474c97b..7fba999 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -695,6 +695,14 @@  wl_connection_demarshal(struct wl_connection *connection,
 	struct wl_closure *closure;
 	struct wl_array *array_extra;
 
+	/* Space for sender_id and opcode */
+	if (size < 2 * sizeof *p) {
+		wl_log("message too short, invalid header\n");
+		wl_connection_consume(connection, size);
+		errno = EINVAL;
+		return NULL;
+	}
+
 	closure = wl_closure_init(message, size, &num_arrays, NULL);
 	if (closure == NULL) {
 		wl_connection_consume(connection, size);
diff --git a/tests/connection-test.c b/tests/connection-test.c
index c3496e6..a06a3cc 100644
--- a/tests/connection-test.c
+++ b/tests/connection-test.c
@@ -798,7 +798,7 @@  expect_error_recv(int sockfd, uint32_t expected_error)
  * However, running under Valgrind would point out invalid reads and use of
  * uninitialized values.
  */
-FAIL_TEST(request_bogus_size)
+TEST(request_bogus_size)
 {
 	struct wl_display *display;
 	struct wl_client *client;

Comments

Hi,

On Wednesday, March 6, 2019 12:58 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> From: Pekka Paalanen pekka.paalanen@collabora.com
>
> The size argument to wl_connection_demarshal() is taken from the message by the
> caller wl_client_connection_data(), therefore 'size' is untrusted data
> controllable by a Wayland client. The size should always be at least the header
> size, otherwise the header is invalid.
>
> If the size is smaller than header size, it leads to reading past the end of
> allocated memory. Furthermore if size is zero, wl_closure_init() changes
> behaviour and leaves num_arrays uninitialized, leading to access of arbitrary
> memory.
>
> Check that 'size' fits at least the header. The space for arguments is already
> properly checked.
>
> This makes the request_bogus_size test free of errors under Valgrind.
>
> Fixes: https://gitlab.freedesktop.org/wayland/wayland/issues/52
>
> Signed-off-by: Pekka Paalanen pekka.paalanen@collabora.com

Both patches look good to me. I've also tested them with -fsanitize=address.

Take this with a grain of salt since I'm not very familiar with libwayland's
demarshalling code, but this is:

Reviewed-by: Simon Ser <contact@emersion.fr>