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

[Juraj Vijtiuk]

Hello,

On Sun, Jan 12, 2020 at 01:09:57PM +0100, Petr Štetiar wrote:
> > @@ -35,10 +35,16 @@ static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool na
> >     char *limit = (char *) attr + len;
> >     const struct blobmsg_hdr *hdr;
> >
> > +   if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr))
> > +           return false;
> > +
>
> why is this change needed? If I'm reading it correctly, then blobmsg_check_name
> is only called from blobmsg_check_attr_len and there is already much stricter guard:
>  bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len)
>  {
>         if (len < sizeof(struct blob_attr))
>                 return false;
>
>         if (!blobmsg_check_name(attr, len, name))
>                 return false;
>
I wasn't sure what the best placement for the check would be, so I put it into the blobmsg_check_name function.
My rationale however was that the existing check in blobmsg_check_attr_len checks whether we have enough data to call blobmsg_check_name at all, as it uses attr as a blob_attr struct. The check was added because sizeof(struct blob_attr) is only 4, as it contains a uint32_t and a flexible array member. The size of the struct containing the flexible array member is as if it was omitted when doing a sizeof of the struct, as specified by the C standards from C99 upwards if I recall correctly. Therefore, another len check in blobmsg_check_name is added to check if there is enough data in attr->data so that it makes sense to call blob_data(attr), as otherwise, if the struct has only the first 4 bytes filled in, the flexible array attr->data is empty, so hdr->namelen will access unused memory which causes the oob read.

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

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)

The same issue appears with the same input file for blobmsg_parse.

> >     hdr = blob_data(attr);
> >     if (name && !hdr->namelen)
> >             return false;
> >
> > +   if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr) + blobmsg_namelen(hdr) + 1)
> > +           return false;
>
> Didn't checked it in detail yet, but isn't it almost the same as the check few
> lines bellow, just written differently?
> >     if ((char *) hdr->name + blobmsg_namelen(hdr) > limit)
> >             return false;
>
> ...
>

You are correct, it is almost the same, however it prevents another
invalid read below, in blobmsg.c:48:

  if (hdr->name[blobmsg_namelen(hdr)] != 0)
                return false;

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;



Here is the xxd of the file that caused the issue to appear:
00000000: 0300 0003 0000                           ......

Here is the relevant valgrind output:
==6076== Invalid read of size 1
==6076==    at 0x1094B8: blobmsg_check_name (blobmsg.c:48)
==6076==    by 0x1092DD: blobmsg_check_attr_len (blobmsg.c:79)
==6076==    by 0x109A63: blobmsg_parse_array (blobmsg.c:159)
==6076==    by 0x10A7BA: main (blobmsg.c:412)
==6076==  Address 0x4a2e2b6 is 0 bytes after a block of size 6 alloc'd
==6076==    at 0x483877F: malloc (vg_replace_malloc.c:309)
==6076==    by 0x10A773: main (blobmsg.c:408)
> > @@ -191,7 +197,11 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len,
> >     }
> >
> >     __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.

Regards,
Juraj

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