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

Submitted by Michal Srb on Aug. 14, 2018, 11:07 a.m.

Details

Message ID 20180814110753.4929-2-msrb@suse.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Michal Srb Aug. 14, 2018, 11:07 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.
---
 tests/connection-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Patch hide | download patch | download mbox

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)
+{
+	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);
+}
+
 TEST(connection_marshal_alot)
 {
 	struct marshal_data data;

Comments

Pekka Paalanen Aug. 17, 2018, 1:15 p.m.
On Tue, 14 Aug 2018 13:07:51 +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.
> ---
>  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
Michal Srb Aug. 17, 2018, 1:35 p.m.
On pátek 17. srpna 2018 15:15:55 CEST Pekka Paalanen wrote:
> Hi Michal,

Hi,

Thank you for the reviews. I will work on the changes. Some answers below.

> > +		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?

The first line (0xffffffff, 0xfffffffe, 0xfffffffd, 0xfffffffc) is to trigger 
the overflow in DIV_ROUNDUP.

The second line (0xfffff000, 0xffffd000, 0xffffc000, 0xffffb000) are random 
lengths that are big enough to overflow the pointer `p` and wrap around to 
point it to some smaller address. We don't know what's in memory there, so 
trying different offsets hoping to hit address that is not mapped.

I'll add explanations for the values to the test.

The whole test is kinda heuristic, it is not guaranteed to cause crash. But 
there is good chance that it will. So if the code changes in future and some 
overflow is reintroduced, this has chance to discover it.

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

Right, I just copied it from the other tests. I'll update and comment the 
values.

Michal