On 2018-08-17 05:53 AM, Pekka Paalanen wrote: > On Tue, 14 Aug 2018 13:07:53 +0200 > Michal Srb <m...@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.paala...@collabora.co.uk>
Also
Reviewed-by: Derek Foreman <derek.foreman.sams...@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
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
