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 <[email protected]>: > > On Thu, 22 Nov 2018 at 10:01, Tobias Schramm <[email protected]> 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 <[email protected]> > > --- > > 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 [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
