[OpenWrt-Devel] [PATCH 2/3] Replace use of blobmsg_check_attr by blobmsg_check_attr_safe

[Tobias Schramm]

While I do agree that we could safely call blobmsg_check_attr I think
that - to the uninitiated reader - calling blobmsg_check_attr_safe
shows a lot more clearly that the methods in question are actually
safe to use with potentially broken /untrusted input. Otherwise you
would have to look at the implementation of __blob_for_each_attr and
understand it first. Also I don't see any downsides to calling
blobmsg_check_attr_safe over blobmsg_check_attr.
Am Fr., 23. Nov. 2018 um 05:06 Uhr schrieb Yousong Zhou <yszhou4tech at gmail.com>:
>
> On Thu, 22 Nov 2018 at 10:01, Tobias Schramm <tobleminer at gmail.com> wrote:
> >
> > blobmsg_check_attr_safe adds a length limit specifying the max offset from attr that
> > can be read safely
> >
> > Signed-off-by: Tobias Schramm <tobleminer at gmail.com>
> > ---
> >  blobmsg.c | 27 ++++++++++++++++++++++-----
> >  blobmsg.h |  2 ++
> >  2 files changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/blobmsg.c b/blobmsg.c
> > index 8019c45..dd4b506 100644
> > --- a/blobmsg.c
> > +++ b/blobmsg.c
> > @@ -32,18 +32,33 @@ blobmsg_namelen(const struct blobmsg_hdr *hdr)
> >  }
> >
> >  bool blobmsg_check_attr(const struct blob_attr *attr, bool name)
> > +{
> > +       return blobmsg_check_attr_safe(attr, name, blob_raw_len(attr));
> > +}
> > +
> > +bool blobmsg_check_attr_safe(const struct blob_attr *attr, bool name, size_t len)
> >  {
> >         const struct blobmsg_hdr *hdr;
> >         const char *data;
> > -       int id, len;
> > +       char *limit = (char*)attr + len;
> > +       int id, data_len;
> > +
> > +       if (len < sizeof(struct blob_attr))
> > +               return false;
> >
> >         if (blob_len(attr) < sizeof(struct blobmsg_hdr))
> >                 return false;
> >
> > +       if (len < sizeof(struct blobmsg_hdr))
> > +               return false;
> > +
> >         hdr = (void *) attr->data;
> >         if (!hdr->namelen && name)
> >                 return false;
> >
> > +       if ((char*)hdr->name + blobmsg_namelen(hdr) > limit)
> > +               return false;
> > +
> >         if (blobmsg_namelen(hdr) > blob_len(attr) - sizeof(struct blobmsg_hdr))
> >                 return false;
> >
> > @@ -51,8 +66,10 @@ bool blobmsg_check_attr(const struct blob_attr *attr, bool name)
> >                 return false;
> >
> >         id = blob_id(attr);
> > -       len = blobmsg_data_len(attr);
> > +       data_len = blobmsg_data_len(attr);
> >         data = blobmsg_data(attr);
> > +       if (data_len < 0 || data + data_len > limit)
> > +               return false;
> >
> >         if (id > BLOBMSG_TYPE_LAST)
> >                 return false;
> > @@ -60,7 +77,7 @@ bool blobmsg_check_attr(const struct blob_attr *attr, bool name)
> >         if (!blob_type[id])
> >                 return true;
> >
> > -       return blob_check_type(data, len, blob_type[id]);
> > +       return blob_check_type(data, data_len, blob_type[id]);
> >  }
> >
> >  int blobmsg_check_array(const struct blob_attr *attr, int type)
> > @@ -111,7 +128,7 @@ int blobmsg_parse_array(const struct blobmsg_policy *policy, int policy_len,
> >                     blob_id(attr) != policy[i].type)
> >                         continue;
> >
> > -               if (!blobmsg_check_attr(attr, false))
> > +               if (!blobmsg_check_attr_safe(attr, false, len))
>
> This cal happens inside __blob_for_each_attr() which should already
> have invariant (blob_pad_len(attr) <= len) hold
>
> >                         return -1;
> >
> >                 if (tb[i])
> > @@ -158,7 +175,7 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len,
> >                         if (blobmsg_namelen(hdr) != pslen[i])
> >                                 continue;
> >
> > -                       if (!blobmsg_check_attr(attr, true))
> > +                       if (!blobmsg_check_attr_safe(attr, true, len))
> >                                 return -1;
> >
>
> ditto
>                 yousong

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