[2/3] connection: Prevent integer overflow in DIV_ROUNDUP.

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

Details

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

Not browsing as part of any series.

Commit Message

Michal Srb Aug. 14, 2018, 11:07 a.m.
The DIV_ROUNDUP macro would overflow when trying to round values higher
than MAX_UINT32 - (a - 1). The result is 0 after the division. This is
potential security issue when demarshalling an array because the length
check is performed with the overflowed value, but then the original huge
value is stored for later use.

The issue was present only on 32bit platforms. The use of size_t in the
DIV_ROUNDUP macro already promoted everything to 64 bit size on 64 bit
systems.
---
 src/connection.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/connection.c b/src/connection.c
index 294c521..31396bc 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -44,7 +44,12 @@ 
 #include "wayland-private.h"
 #include "wayland-os.h"
 
-#define DIV_ROUNDUP(n, a) ( ((n) + ((a) - 1)) / (a) )
+static inline uint32_t div_roundup(uint32_t n, size_t a) {
+    // The cast to uint64_t is necessary to prevent overflow when rounding
+    // values close to UINT32_MAX. After the division it is again safe to
+    // cast back to uint32_t.
+    return (uint32_t) (((uint64_t) n + (a - 1)) / a);
+}
 
 struct wl_buffer {
 	char data[4096];
@@ -734,7 +739,7 @@  wl_connection_demarshal(struct wl_connection *connection,
 				break;
 			}
 
-			next = p + DIV_ROUNDUP(length, sizeof *p);
+			next = p + div_roundup(length, sizeof *p);
 			if (next > end) {
 				wl_log("message too short, "
 				       "object (%d), message %s(%s)\n",
@@ -793,7 +798,7 @@  wl_connection_demarshal(struct wl_connection *connection,
 		case 'a':
 			length = *p++;
 
-			next = p + DIV_ROUNDUP(length, sizeof *p);
+			next = p + div_roundup(length, sizeof *p);
 			if (next > end) {
 				wl_log("message too short, "
 				       "object (%d), message %s(%s)\n",
@@ -1068,7 +1073,7 @@  buffer_size_for_closure(struct wl_closure *closure)
 			}
 
 			size = strlen(closure->args[i].s) + 1;
-			buffer_size += 1 + DIV_ROUNDUP(size, sizeof(uint32_t));
+			buffer_size += 1 + div_roundup(size, sizeof(uint32_t));
 			break;
 		case 'a':
 			if (closure->args[i].a == NULL) {
@@ -1077,7 +1082,7 @@  buffer_size_for_closure(struct wl_closure *closure)
 			}
 
 			size = closure->args[i].a->size;
-			buffer_size += (1 + DIV_ROUNDUP(size, sizeof(uint32_t)));
+			buffer_size += (1 + div_roundup(size, sizeof(uint32_t)));
 			break;
 		default:
 			break;
@@ -1139,11 +1144,11 @@  serialize_closure(struct wl_closure *closure, uint32_t *buffer,
 			size = strlen(closure->args[i].s) + 1;
 			*p++ = size;
 
-			if (p + DIV_ROUNDUP(size, sizeof *p) > end)
+			if (p + div_roundup(size, sizeof *p) > end)
 				goto overflow;
 
 			memcpy(p, closure->args[i].s, size);
-			p += DIV_ROUNDUP(size, sizeof *p);
+			p += div_roundup(size, sizeof *p);
 			break;
 		case 'a':
 			if (closure->args[i].a == NULL) {
@@ -1154,11 +1159,11 @@  serialize_closure(struct wl_closure *closure, uint32_t *buffer,
 			size = closure->args[i].a->size;
 			*p++ = size;
 
-			if (p + DIV_ROUNDUP(size, sizeof *p) > end)
+			if (p + div_roundup(size, sizeof *p) > end)
 				goto overflow;
 
 			memcpy(p, closure->args[i].a->data, size);
-			p += DIV_ROUNDUP(size, sizeof *p);
+			p += div_roundup(size, sizeof *p);
 			break;
 		default:
 			break;

Comments

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

> The DIV_ROUNDUP macro would overflow when trying to round values higher
> than MAX_UINT32 - (a - 1). The result is 0 after the division. This is
> potential security issue when demarshalling an array because the length
> check is performed with the overflowed value, but then the original huge
> value is stored for later use.
> 
> The issue was present only on 32bit platforms. The use of size_t in the
> DIV_ROUNDUP macro already promoted everything to 64 bit size on 64 bit
> systems.
> ---
>  src/connection.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/src/connection.c b/src/connection.c
> index 294c521..31396bc 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -44,7 +44,12 @@
>  #include "wayland-private.h"
>  #include "wayland-os.h"
>  
> -#define DIV_ROUNDUP(n, a) ( ((n) + ((a) - 1)) / (a) )
> +static inline uint32_t div_roundup(uint32_t n, size_t a) {
> +    // The cast to uint64_t is necessary to prevent overflow when rounding
> +    // values close to UINT32_MAX. After the division it is again safe to
> +    // cast back to uint32_t.
> +    return (uint32_t) (((uint64_t) n + (a - 1)) / a);
> +}

The above has a few style issues:
- div_roundup should start on a new line as it is a function now
- use /* */ comment style
- use tabs for indent
- missing Signed-off-by

But aside from those, this patch is:

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

>  
>  struct wl_buffer {
>  	char data[4096];
> @@ -734,7 +739,7 @@ wl_connection_demarshal(struct wl_connection *connection,
>  				break;
>  			}
>  
> -			next = p + DIV_ROUNDUP(length, sizeof *p);
> +			next = p + div_roundup(length, sizeof *p);
>  			if (next > end) {
>  				wl_log("message too short, "
>  				       "object (%d), message %s(%s)\n",
> @@ -793,7 +798,7 @@ wl_connection_demarshal(struct wl_connection *connection,
>  		case 'a':
>  			length = *p++;
>  
> -			next = p + DIV_ROUNDUP(length, sizeof *p);
> +			next = p + div_roundup(length, sizeof *p);
>  			if (next > end) {
>  				wl_log("message too short, "
>  				       "object (%d), message %s(%s)\n",
> @@ -1068,7 +1073,7 @@ buffer_size_for_closure(struct wl_closure *closure)
>  			}
>  
>  			size = strlen(closure->args[i].s) + 1;
> -			buffer_size += 1 + DIV_ROUNDUP(size, sizeof(uint32_t));
> +			buffer_size += 1 + div_roundup(size, sizeof(uint32_t));
>  			break;
>  		case 'a':
>  			if (closure->args[i].a == NULL) {
> @@ -1077,7 +1082,7 @@ buffer_size_for_closure(struct wl_closure *closure)
>  			}
>  
>  			size = closure->args[i].a->size;
> -			buffer_size += (1 + DIV_ROUNDUP(size, sizeof(uint32_t)));
> +			buffer_size += (1 + div_roundup(size, sizeof(uint32_t)));
>  			break;
>  		default:
>  			break;
> @@ -1139,11 +1144,11 @@ serialize_closure(struct wl_closure *closure, uint32_t *buffer,
>  			size = strlen(closure->args[i].s) + 1;
>  			*p++ = size;
>  
> -			if (p + DIV_ROUNDUP(size, sizeof *p) > end)
> +			if (p + div_roundup(size, sizeof *p) > end)
>  				goto overflow;
>  
>  			memcpy(p, closure->args[i].s, size);
> -			p += DIV_ROUNDUP(size, sizeof *p);
> +			p += div_roundup(size, sizeof *p);
>  			break;
>  		case 'a':
>  			if (closure->args[i].a == NULL) {
> @@ -1154,11 +1159,11 @@ serialize_closure(struct wl_closure *closure, uint32_t *buffer,
>  			size = closure->args[i].a->size;
>  			*p++ = size;
>  
> -			if (p + DIV_ROUNDUP(size, sizeof *p) > end)
> +			if (p + div_roundup(size, sizeof *p) > end)
>  				goto overflow;
>  
>  			memcpy(p, closure->args[i].a->data, size);
> -			p += DIV_ROUNDUP(size, sizeof *p);
> +			p += div_roundup(size, sizeof *p);
>  			break;
>  		default:
>  			break;
On 2018-08-17 05:39 AM, Pekka Paalanen wrote:
> On Tue, 14 Aug 2018 13:07:52 +0200
> Michal Srb <msrb@suse.com> wrote:
> 
>> The DIV_ROUNDUP macro would overflow when trying to round values higher
>> than MAX_UINT32 - (a - 1). The result is 0 after the division. This is
>> potential security issue when demarshalling an array because the length
>> check is performed with the overflowed value, but then the original huge
>> value is stored for later use.
>>
>> The issue was present only on 32bit platforms. The use of size_t in the
>> DIV_ROUNDUP macro already promoted everything to 64 bit size on 64 bit
>> systems.
>> ---
>>  src/connection.c | 23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/connection.c b/src/connection.c
>> index 294c521..31396bc 100644
>> --- a/src/connection.c
>> +++ b/src/connection.c
>> @@ -44,7 +44,12 @@
>>  #include "wayland-private.h"
>>  #include "wayland-os.h"
>>  
>> -#define DIV_ROUNDUP(n, a) ( ((n) + ((a) - 1)) / (a) )
>> +static inline uint32_t div_roundup(uint32_t n, size_t a) {
>> +    // The cast to uint64_t is necessary to prevent overflow when rounding
>> +    // values close to UINT32_MAX. After the division it is again safe to
>> +    // cast back to uint32_t.
>> +    return (uint32_t) (((uint64_t) n + (a - 1)) / a);
>> +}
> 
> The above has a few style issues:
> - div_roundup should start on a new line as it is a function now
> - use /* */ comment style
> - use tabs for indent
> - missing Signed-off-by
> 
> But aside from those, this patch is:
> 
> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

It's also
Reviewed-by: Derek Foreman <derek.foreman.samsung@gmail.com>

I'd like to land this today, so I'll deal with the trivial style
complaints when landing.

Thanks,
Derek

>>  
>>  struct wl_buffer {
>>  	char data[4096];
>> @@ -734,7 +739,7 @@ wl_connection_demarshal(struct wl_connection *connection,
>>  				break;
>>  			}
>>  
>> -			next = p + DIV_ROUNDUP(length, sizeof *p);
>> +			next = p + div_roundup(length, sizeof *p);
>>  			if (next > end) {
>>  				wl_log("message too short, "
>>  				       "object (%d), message %s(%s)\n",
>> @@ -793,7 +798,7 @@ wl_connection_demarshal(struct wl_connection *connection,
>>  		case 'a':
>>  			length = *p++;
>>  
>> -			next = p + DIV_ROUNDUP(length, sizeof *p);
>> +			next = p + div_roundup(length, sizeof *p);
>>  			if (next > end) {
>>  				wl_log("message too short, "
>>  				       "object (%d), message %s(%s)\n",
>> @@ -1068,7 +1073,7 @@ buffer_size_for_closure(struct wl_closure *closure)
>>  			}
>>  
>>  			size = strlen(closure->args[i].s) + 1;
>> -			buffer_size += 1 + DIV_ROUNDUP(size, sizeof(uint32_t));
>> +			buffer_size += 1 + div_roundup(size, sizeof(uint32_t));
>>  			break;
>>  		case 'a':
>>  			if (closure->args[i].a == NULL) {
>> @@ -1077,7 +1082,7 @@ buffer_size_for_closure(struct wl_closure *closure)
>>  			}
>>  
>>  			size = closure->args[i].a->size;
>> -			buffer_size += (1 + DIV_ROUNDUP(size, sizeof(uint32_t)));
>> +			buffer_size += (1 + div_roundup(size, sizeof(uint32_t)));
>>  			break;
>>  		default:
>>  			break;
>> @@ -1139,11 +1144,11 @@ serialize_closure(struct wl_closure *closure, uint32_t *buffer,
>>  			size = strlen(closure->args[i].s) + 1;
>>  			*p++ = size;
>>  
>> -			if (p + DIV_ROUNDUP(size, sizeof *p) > end)
>> +			if (p + div_roundup(size, sizeof *p) > end)
>>  				goto overflow;
>>  
>>  			memcpy(p, closure->args[i].s, size);
>> -			p += DIV_ROUNDUP(size, sizeof *p);
>> +			p += div_roundup(size, sizeof *p);
>>  			break;
>>  		case 'a':
>>  			if (closure->args[i].a == NULL) {
>> @@ -1154,11 +1159,11 @@ serialize_closure(struct wl_closure *closure, uint32_t *buffer,
>>  			size = closure->args[i].a->size;
>>  			*p++ = size;
>>  
>> -			if (p + DIV_ROUNDUP(size, sizeof *p) > end)
>> +			if (p + div_roundup(size, sizeof *p) > end)
>>  				goto overflow;
>>  
>>  			memcpy(p, closure->args[i].a->data, size);
>> -			p += DIV_ROUNDUP(size, sizeof *p);
>> +			p += div_roundup(size, sizeof *p);
>>  			break;
>>  		default:
>>  			break;
> 
> 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>