[3/3] connection: Prevent pointer overflow from large lengths.

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

Details

Message ID 20180814110753.4929-4-msrb@suse.com
State Accepted
Commit f7fdface41a9205c12aedf7fe04aba7792402909
Headers show
Series "Series without cover letter" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Michal Srb Aug. 14, 2018, 11:07 a.m.
If the remote side sends sufficiently large `length` field, it will
overflow the `p` pointer. Technically it is undefined behavior, in
practice it makes `p < end`, so the length check passes. Attempts to
access the data later causes crashes.

This issue manifests only on 32bit systems, but the behavior is
undefined everywhere.
---
 src/connection.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/connection.c b/src/connection.c
index 31396bc..a0f10ae 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -683,7 +683,7 @@  wl_connection_demarshal(struct wl_connection *connection,
 			struct wl_map *objects,
 			const struct wl_message *message)
 {
-	uint32_t *p, *next, *end, length, id;
+	uint32_t *p, *next, *end, length, length_in_u32, id;
 	int fd;
 	char *s;
 	int i, count, num_arrays;
@@ -739,8 +739,8 @@  wl_connection_demarshal(struct wl_connection *connection,
 				break;
 			}
 
-			next = p + div_roundup(length, sizeof *p);
-			if (next > end) {
+			length_in_u32 = div_roundup(length, sizeof *p);
+			if ((uint32_t) (end - p) < length_in_u32) {
 				wl_log("message too short, "
 				       "object (%d), message %s(%s)\n",
 				       closure->sender_id, message->name,
@@ -748,6 +748,7 @@  wl_connection_demarshal(struct wl_connection *connection,
 				errno = EINVAL;
 				goto err;
 			}
+			next = p + length_in_u32;
 
 			s = (char *) p;
 
@@ -798,8 +799,8 @@  wl_connection_demarshal(struct wl_connection *connection,
 		case 'a':
 			length = *p++;
 
-			next = p + div_roundup(length, sizeof *p);
-			if (next > end) {
+			length_in_u32 = div_roundup(length, sizeof *p);
+			if ((uint32_t) (end - p) < length_in_u32) {
 				wl_log("message too short, "
 				       "object (%d), message %s(%s)\n",
 				       closure->sender_id, message->name,
@@ -807,6 +808,7 @@  wl_connection_demarshal(struct wl_connection *connection,
 				errno = EINVAL;
 				goto err;
 			}
+			next = p + length_in_u32;
 
 			array_extra->size = length;
 			array_extra->alloc = 0;

Comments

On Tue, 14 Aug 2018 13:07:53 +0200
Michal Srb <msrb@suse.com> wrote:

> If the remote side sends sufficiently large `length` field, it will
> overflow the `p` pointer. Technically it is undefined behavior, in
> practice it makes `p < end`, so the length check passes. Attempts to
> access the data later causes crashes.
> 
> This issue manifests only on 32bit systems, but the behavior is
> undefined everywhere.
> ---
>  src/connection.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

Hi,

this looks correct to me and should address Jann's concerns too. I also
checked that (end - p) cannot become negative.

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

I'm still playing with the tests patch a bit.


Thanks,
pq

> 
> diff --git a/src/connection.c b/src/connection.c
> index 31396bc..a0f10ae 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -683,7 +683,7 @@ wl_connection_demarshal(struct wl_connection *connection,
>  			struct wl_map *objects,
>  			const struct wl_message *message)
>  {
> -	uint32_t *p, *next, *end, length, id;
> +	uint32_t *p, *next, *end, length, length_in_u32, id;
>  	int fd;
>  	char *s;
>  	int i, count, num_arrays;
> @@ -739,8 +739,8 @@ wl_connection_demarshal(struct wl_connection *connection,
>  				break;
>  			}
>  
> -			next = p + div_roundup(length, sizeof *p);
> -			if (next > end) {
> +			length_in_u32 = div_roundup(length, sizeof *p);
> +			if ((uint32_t) (end - p) < length_in_u32) {
>  				wl_log("message too short, "
>  				       "object (%d), message %s(%s)\n",
>  				       closure->sender_id, message->name,
> @@ -748,6 +748,7 @@ wl_connection_demarshal(struct wl_connection *connection,
>  				errno = EINVAL;
>  				goto err;
>  			}
> +			next = p + length_in_u32;
>  
>  			s = (char *) p;
>  
> @@ -798,8 +799,8 @@ wl_connection_demarshal(struct wl_connection *connection,
>  		case 'a':
>  			length = *p++;
>  
> -			next = p + div_roundup(length, sizeof *p);
> -			if (next > end) {
> +			length_in_u32 = div_roundup(length, sizeof *p);
> +			if ((uint32_t) (end - p) < length_in_u32) {
>  				wl_log("message too short, "
>  				       "object (%d), message %s(%s)\n",
>  				       closure->sender_id, message->name,
> @@ -807,6 +808,7 @@ wl_connection_demarshal(struct wl_connection *connection,
>  				errno = EINVAL;
>  				goto err;
>  			}
> +			next = p + length_in_u32;
>  
>  			array_extra->size = length;
>  			array_extra->alloc = 0;
On 2018-08-17 05:53 AM, Pekka Paalanen wrote:
> On Tue, 14 Aug 2018 13:07:53 +0200
> Michal Srb <msrb@suse.com> wrote:
> 
>> If the remote side sends sufficiently large `length` field, it will
>> overflow the `p` pointer. Technically it is undefined behavior, in
>> practice it makes `p < end`, so the length check passes. Attempts to
>> access the data later causes crashes.
>>
>> This issue manifests only on 32bit systems, but the behavior is
>> undefined everywhere.
>> ---
>>  src/connection.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> Hi,
> 
> this looks correct to me and should address Jann's concerns too. I also
> checked that (end - p) cannot become negative.
> 
> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

Also
Reviewed-by: Derek Foreman <derek.foreman.samsung@gmail.com>

I'll land this and the 2nd patch in the series today before rolling the
RC2 releases.

Looks like the test needs a follow up, so that can land later.

Thanks,
Derek

> I'm still playing with the tests patch a bit.
> 
> 
> Thanks,
> pq
> 
>>
>> diff --git a/src/connection.c b/src/connection.c
>> index 31396bc..a0f10ae 100644
>> --- a/src/connection.c
>> +++ b/src/connection.c
>> @@ -683,7 +683,7 @@ wl_connection_demarshal(struct wl_connection *connection,
>>  			struct wl_map *objects,
>>  			const struct wl_message *message)
>>  {
>> -	uint32_t *p, *next, *end, length, id;
>> +	uint32_t *p, *next, *end, length, length_in_u32, id;
>>  	int fd;
>>  	char *s;
>>  	int i, count, num_arrays;
>> @@ -739,8 +739,8 @@ wl_connection_demarshal(struct wl_connection *connection,
>>  				break;
>>  			}
>>  
>> -			next = p + div_roundup(length, sizeof *p);
>> -			if (next > end) {
>> +			length_in_u32 = div_roundup(length, sizeof *p);
>> +			if ((uint32_t) (end - p) < length_in_u32) {
>>  				wl_log("message too short, "
>>  				       "object (%d), message %s(%s)\n",
>>  				       closure->sender_id, message->name,
>> @@ -748,6 +748,7 @@ wl_connection_demarshal(struct wl_connection *connection,
>>  				errno = EINVAL;
>>  				goto err;
>>  			}
>> +			next = p + length_in_u32;
>>  
>>  			s = (char *) p;
>>  
>> @@ -798,8 +799,8 @@ wl_connection_demarshal(struct wl_connection *connection,
>>  		case 'a':
>>  			length = *p++;
>>  
>> -			next = p + div_roundup(length, sizeof *p);
>> -			if (next > end) {
>> +			length_in_u32 = div_roundup(length, sizeof *p);
>> +			if ((uint32_t) (end - p) < length_in_u32) {
>>  				wl_log("message too short, "
>>  				       "object (%d), message %s(%s)\n",
>>  				       closure->sender_id, message->name,
>> @@ -807,6 +808,7 @@ wl_connection_demarshal(struct wl_connection *connection,
>>  				errno = EINVAL;
>>  				goto err;
>>  			}
>> +			next = p + length_in_u32;
>>  
>>  			array_extra->size = length;
>>  			array_extra->alloc = 0;
> 
> 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>