[OpenWrt-Devel] [PATCH][libubox] blobmsg: blobmsg_parse and blobmsg_parse_array oob read fixes

[Petr Štetiar]

Juraj Vijtiuk <juraj.vijtiuk at sartura.hr> [2020-01-14 22:11:18]:

> Here is the xxd output for the crash file that causes the invalid read when
> passed as data to blobmsg_parse_array with a len of 4:
>
> xxd crash-a3585b70f1c7ffbdec10f6dadc964336118485c4
> 00000000: 0300 0004                                ....

Thanks for sharing.

> Here is the relevant valgrind output, the main function here simply reads
> the input and passes it to blobmsg_parse_array:
>
> ==10829== Invalid read of size 2
> ==10829==    at 0x109DFC: blobmsg_namelen (blobmsg.h:74)
> ==10829==    by 0x109446: blobmsg_check_name (blobmsg.c:42)
> ==10829==    by 0x1092DD: blobmsg_check_attr_len (blobmsg.c:79)
> ==10829==    by 0x109A63: blobmsg_parse_array (blobmsg.c:159)
> ==10829==    by 0x10A7BA: main (blobmsg.c:412)
> ==10829==  Address 0x4a2e2b4 is 0 bytes after a block of size 4 alloc'd
> ==10829==    at 0x483877F: malloc (vg_replace_malloc.c:309)
> ==10829==    by 0x10A773: main (blobmsg.c:408)

That malloc is important here, one has to use dynamically allocated buffer and
Valgrind in order to see this OOB read access. There's already similar crash
from the previous runs, but without malloc'ed buffer Valgrind doesn't spot
that, lesson learned.

 xxd tests/fuzz/corpus/crash-75b146c4e6fac64d3e62236b27c64b50657bab2a 
 00000000: 0300 0002                                ....

> I assume that this checks whether hdr->name is a null terminated
> string. However, namelen seems to store the result returned as if it was returned by strlen,
> and the limit check doesn't seem to include the terminating null byte,
> although I suppose modifying that check would definitely be better than
> having two checks. The check would then look like this:
> 
>   if ((char *) hdr->name + blobmsg_namelen(hdr) + 1 > limit)
>                 return false;

Ok, makes sense, I'll check that.

> > >     __blob_for_each_attr(attr, data, len) {
> > > +           if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr))
> > > +                   return -1;
> >
> > If there is such problem, then this should be probably fixed directly in
> > __blob_for_each_attr so we possibly protect other __blob_for_each_attr
> > users[1].
> 
> Can you maybe provide a patch? I'd be happy to test it and let you
> know what the results are.

Ok.

-- ynezz

_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel