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

[Pekka Paalanen]

On Tue, 14 Aug 2018 13:07:51 +0200
Michal Srb <msrb at 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.
> ---
>  tests/connection-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 

Hi Michal,

sorry for taking a while. Thank you very much for adding tests. I think
the tests are effectively right, but I would like to see them polished
more, as noted below.

A code comment should explain that without the fixes, the new tests
will still pass on 64-bit arch. In the logs I get:

message too short, object (400200), message test(s)
message too short, object (400200), message test(a)
...

One needs to run a 32-bit build to see the problems.

> diff --git a/tests/connection-test.c b/tests/connection-test.c
> index 157e1bc..09b0f00 100644
> --- a/tests/connection-test.c
> +++ b/tests/connection-test.c
> @@ -533,6 +533,52 @@ TEST(connection_marshal_demarshal)
>  	release_marshal_data(&data);
>  }
>  
> +static void
> +expected_fail_demarshal(struct marshal_data *data, const char *format, uint32_t *msg, int expected_error)

Split long line.

'msg' could be const.

> +{
> +	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];

Declare unsigned i here.

> +
> +	setup_marshal_data(&data);
> +
> +	// These need careful handling on 32bit systems.

Use C89 comments /* */.

> +	uint32_t overflowing_values[] = {

const

> +		0xffffffff, 0xfffffffe, 0xfffffffd, 0xfffffffc,
> +		0xfffff000, 0xffffd000, 0xffffc000, 0xffffb000

What is the rationale for the second row on values here?
I can understand the first row which are very close to overflow.
Why is 0xffffe000 not included?

> +	};

Call setup_marshal_data() here to avoid mixed declarations and code.


> +	for (unsigned int i = 0; i < ARRAY_LENGTH(overflowing_values); i++) {

The following are quite confusing to me:

> +		msg[0] = 400200;

This is the sender id, which is irrelevant to the code being tested;
can be arbitrary. Could use a comment /* sender id */.

> +		msg[1] = 24;

This might be due to bad examples already present in this file.

This is the ((size << 16) | opcode) field. The size is unused in the
code to be tested, and the opcode is irrelevant too. However, you are
repurposing this field for message size in a way that does not match
the wire format. I find it confusing.

I also could not figure out where the 24 comes from. I would have
expected:
- header, 8 bytes
- 's' argument length, 4 bytes
- followed by the literal string data including the terminating NUL and
  padding up to the next 4 byte boundary

Of course, the excercise is to lie about the string length, so the
literal string data is irrelevant. The arbitrary 24 works here, but my
complaint is that there is no recorded rationale where it came from.

This file already contains mistakes like line 420:

	msg[1] = 12;		/* size = 12, opcode = 0 */

That's actually size=0, opcode=12. I guess that this mislead you.

> +		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);

Arrays are serialized the same as strings, so doing the exact same test
with an array is fine. If 'msg' in expected_fail_demarshal() was const,
it would be obviously safe to avoid repeating the assignments.

> +	}
> +
> +	release_marshal_data(&data);
> +}
> +
>  TEST(connection_marshal_alot)
>  {
>  	struct marshal_data data;


I managed to set up a 32-bit build in Gitlab CI. With only patch 1, the
new test fails with:

string not nul-terminated, message test(s)
connection-test: ../tests/connection-test.c:551: expected_fail_demarshal: Assertion `closure == NULL' failed.
test "connection_demarshal_failures":   signal 6, fail.

The first line is an unexpected pass, but I think the test works. It
does catch a case that should not get through.

With patches 1 and 2, the output is exactly the same. With all three
patches the CI succeeds on both 32 and 64 bit.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180817/c7336451/attachment.sig>