| Message ID | 20200613181740.988875-1-baptiste@bitsofnetworks.org |
|---|---|
| State | Accepted |
| Delegated to: | Hauke Mehrtens |
| Headers | show |
| Series | [OpenWrt-Devel,18.06] libubox: backport additional length-checking fixes | expand |
Hi, I should have added more details in the commit message: this fixes a serious regression where procd fails to start some services, for instance rpcd. See FS#3177. This is the same regression that Rafał found in 19.07.2 and 19.07.3. I would like a review that I backported the right set of commits: I don't think I missed one, but I'm not 100% sure that the 4 commits I backported are all strictly necessary. In any case, they are all clean cherry-picks. Thanks, Baptiste On 13-06-20, Baptiste Jonglez wrote: > From: Baptiste Jonglez <git@bitsofnetworks.org> > > Fixes: FS#3177 > Cc: Felix Fietkau <nbd@nbd.name> > Cc: Rafał Miłecki <rafal@milecki.pl> > Signed-off-by: Baptiste Jonglez <git@bitsofnetworks.org> > --- > package/libs/libubox/Makefile | 2 +- > ...s-iteration-in-the-blobmsg_check_arr.patch | 75 ++++++++++ > ...sg-fix-length-in-blobmsg_check_array.patch | 28 ++++ > ...-and-fix-name-length-checks-in-blobm.patch | 49 +++++++ > ...21-blobmsg-fix-missing-length-checks.patch | 138 ++++++++++++++++++ > 5 files changed, 291 insertions(+), 1 deletion(-) > create mode 100644 package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch > create mode 100644 package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch > create mode 100644 package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch > create mode 100644 package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch > > diff --git a/package/libs/libubox/Makefile b/package/libs/libubox/Makefile > index e3a827c1ab..e4f1a6b503 100644 > --- a/package/libs/libubox/Makefile > +++ b/package/libs/libubox/Makefile > @@ -1,7 +1,7 @@ > include $(TOPDIR)/rules.mk > > PKG_NAME:=libubox > -PKG_RELEASE=4 > +PKG_RELEASE=5 > > PKG_SOURCE_PROTO:=git > PKG_SOURCE_URL=$(PROJECT_GIT)/project/libubox.git > diff --git a/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch b/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch > new file mode 100644 > index 0000000000..2834e10ee3 > --- /dev/null > +++ b/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch > @@ -0,0 +1,75 @@ > +From 5e75160f48785464f9213c6bc8c72b9372c5318b Mon Sep 17 00:00:00 2001 > +From: =?UTF-8?q?Rafa=C5=82=20Mi=C5=82ecki?= <rafal@milecki.pl> > +Date: Sat, 23 May 2020 13:18:51 +0200 > +Subject: [PATCH] blobmsg: fix attrs iteration in the blobmsg_check_array_len() > +MIME-Version: 1.0 > +Content-Type: text/plain; charset=UTF-8 > +Content-Transfer-Encoding: 8bit > + > +Starting with 75e300aeec25 ("blobmsg: fix wrong payload len passed from > +blobmsg_check_array") blobmsg_check_array_len() gets *blob* length > +passed as argument. It cannot be used with __blobmsg_for_each_attr() > +which expects *data* length. > + > +Use blobmsg_for_each_attr() which calculates *data* length on its own. > + > +The same bug was already reported in the past and there was fix attempt > +in the commit cd75136b1342 ("blobmsg: fix wrong payload len passed from > +blobmsg_check_array"). That change made blobmsg_check_attr_len() calls > +fail however. > + > +This is hopefully the correct & complete fix: > +1. blobmsg_check_array_len() gets *blob* length > +2. It calls blobmsg_check_attr_len() which requires *blob* length > +3. It uses blobmsg_for_each_attr() which gets *data* length > + > +This fixes iterating over random memory treated as attrs. That was > +resulting in check failing randomly for totally correct blobs. It's > +critical e.g. for procd project with its instance_fill_array() failing > +and procd not starting services. > + > +Fixes: 75e300aeec25 ("blobmsg: fix wrong payload len passed from blobmsg_check_array") > +Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > +--- > + blobmsg.c | 10 ++++++---- > + 1 file changed, 6 insertions(+), 4 deletions(-) > + > +diff --git a/blobmsg.c b/blobmsg.c > +index 8b9877d..59045e1 100644 > +--- a/blobmsg.c > ++++ b/blobmsg.c > +@@ -117,16 +117,18 @@ int blobmsg_check_array(const struct blob_attr *attr, int type) > + return blobmsg_check_array_len(attr, type, blob_len(attr)); > + } > + > +-int blobmsg_check_array_len(const struct blob_attr *attr, int type, size_t len) > ++int blobmsg_check_array_len(const struct blob_attr *attr, int type, > ++ size_t blob_len) > + { > + struct blob_attr *cur; > ++ size_t rem; > + bool name; > + int size = 0; > + > + if (type > BLOBMSG_TYPE_LAST) > + return -1; > + > +- if (!blobmsg_check_attr_len(attr, false, len)) > ++ if (!blobmsg_check_attr_len(attr, false, blob_len)) > + return -1; > + > + switch (blobmsg_type(attr)) { > +@@ -140,11 +142,11 @@ int blobmsg_check_array_len(const struct blob_attr *attr, int type, size_t len) > + return -1; > + } > + > +- __blobmsg_for_each_attr(cur, attr, len) { > ++ blobmsg_for_each_attr(cur, attr, rem) { > + if (type != BLOBMSG_TYPE_UNSPEC && blobmsg_type(cur) != type) > + return -1; > + > +- if (!blobmsg_check_attr_len(cur, name, len)) > ++ if (!blobmsg_check_attr_len(cur, name, rem)) > + return -1; > + > + size++; > diff --git a/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch b/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch > new file mode 100644 > index 0000000000..9db2fb4f9f > --- /dev/null > +++ b/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch > @@ -0,0 +1,28 @@ > +From c2fc622b771f679e8f55060ac60cfe02b9a80995 Mon Sep 17 00:00:00 2001 > +From: Felix Fietkau <nbd@nbd.name> > +Date: Mon, 25 May 2020 13:44:20 +0200 > +Subject: [PATCH] blobmsg: fix length in blobmsg_check_array > + > +blobmsg_check_array_len expects the length of the full attribute buffer, > +not just the data length. > +Due to other missing length checks (fixed in the next commit), this did > +not show up as a test failure > + > +Signed-off-by: Felix Fietkau <nbd@nbd.name> > +--- > + blobmsg.c | 2 +- > + 1 file changed, 1 insertion(+), 1 deletion(-) > + > +diff --git a/blobmsg.c b/blobmsg.c > +index 59045e1..daaa9fc 100644 > +--- a/blobmsg.c > ++++ b/blobmsg.c > +@@ -114,7 +114,7 @@ bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len) > + > + int blobmsg_check_array(const struct blob_attr *attr, int type) > + { > +- return blobmsg_check_array_len(attr, type, blob_len(attr)); > ++ return blobmsg_check_array_len(attr, type, blob_raw_len(attr)); > + } > + > + int blobmsg_check_array_len(const struct blob_attr *attr, int type, > diff --git a/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch b/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch > new file mode 100644 > index 0000000000..a481208789 > --- /dev/null > +++ b/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch > @@ -0,0 +1,49 @@ > +From 639c29d19717616b809d9a1e9042461ab8024370 Mon Sep 17 00:00:00 2001 > +From: Felix Fietkau <nbd@nbd.name> > +Date: Mon, 25 May 2020 14:49:35 +0200 > +Subject: [PATCH] blobmsg: simplify and fix name length checks in > + blobmsg_check_name > + > +blobmsg_hdr_valid_namelen was omitted when name==false > +The blob_len vs blobmsg_namelen changes were not taking into account > +potential padding between name and data > + > +Signed-off-by: Felix Fietkau <nbd@nbd.name> > +--- > + blobmsg.c | 13 ++++--------- > + 1 file changed, 4 insertions(+), 9 deletions(-) > + > +diff --git a/blobmsg.c b/blobmsg.c > +index daaa9fc..308bef7 100644 > +--- a/blobmsg.c > ++++ b/blobmsg.c > +@@ -48,8 +48,8 @@ static bool blobmsg_hdr_valid_namelen(const struct blobmsg_hdr *hdr, size_t len) > + > + static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool name) > + { > +- char *limit = (char *) attr + len; > + const struct blobmsg_hdr *hdr; > ++ uint16_t namelen; > + > + hdr = blobmsg_hdr_from_blob(attr, len); > + if (!hdr) > +@@ -58,16 +58,11 @@ static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool na > + if (name && !hdr->namelen) > + return false; > + > +- if (name && !blobmsg_hdr_valid_namelen(hdr, len)) > +- return false; > +- > +- if ((char *) hdr->name + blobmsg_namelen(hdr) + 1 > limit) > +- return false; > +- > +- if (blobmsg_namelen(hdr) > (blob_len(attr) - sizeof(struct blobmsg_hdr))) > ++ namelen = blobmsg_namelen(hdr); > ++ if (blob_len(attr) < (size_t)blobmsg_hdrlen(namelen)) > + return false; > + > +- if (hdr->name[blobmsg_namelen(hdr)] != 0) > ++ if (hdr->name[namelen] != 0) > + return false; > + > + return true; > diff --git a/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch b/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch > new file mode 100644 > index 0000000000..bfc440a329 > --- /dev/null > +++ b/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch > @@ -0,0 +1,138 @@ > +From 66195aee50424cbda0c2d858014e4cc58a2dc029 Mon Sep 17 00:00:00 2001 > +From: Felix Fietkau <nbd@nbd.name> > +Date: Mon, 25 May 2020 12:40:04 +0200 > +Subject: [PATCH] blobmsg: fix missing length checks > + > +blobmsg_check_attr_len was calling blobmsg_check_data for some, but not all > +attribute types. These checks was missing for arrays and tables. > + > +Additionally, the length check in blobmsg_check_data was a bit off, since > +it was comparing the blobmsg data length against the raw blob attr length. > + > +Fix this by checking the raw blob length against the buffer length in > +blobmsg_hdr_from_blob > + > +Signed-off-by: Felix Fietkau <nbd@nbd.name> > +--- > + blobmsg.c | 66 +++++++++++++++++-------------------------------------- > + 1 file changed, 20 insertions(+), 46 deletions(-) > + > +diff --git a/blobmsg.c b/blobmsg.c > +index 308bef7..7da4183 100644 > +--- a/blobmsg.c > ++++ b/blobmsg.c > +@@ -30,31 +30,18 @@ bool blobmsg_check_attr(const struct blob_attr *attr, bool name) > + return blobmsg_check_attr_len(attr, name, blob_raw_len(attr)); > + } > + > +-static const struct blobmsg_hdr* blobmsg_hdr_from_blob(const struct blob_attr *attr, size_t len) > +-{ > +- if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) > +- return NULL; > +- > +- return blob_data(attr); > +-} > +- > +-static bool blobmsg_hdr_valid_namelen(const struct blobmsg_hdr *hdr, size_t len) > +-{ > +- if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr) + blobmsg_namelen(hdr) + 1) > +- return false; > +- > +- return true; > +-} > +- > +-static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool name) > ++static bool blobmsg_check_name(const struct blob_attr *attr, bool name) > + { > + const struct blobmsg_hdr *hdr; > + uint16_t namelen; > + > +- hdr = blobmsg_hdr_from_blob(attr, len); > +- if (!hdr) > ++ if (!blob_is_extended(attr)) > ++ return !name; > ++ > ++ if (blob_len(attr) < sizeof(struct blobmsg_hdr)) > + return false; > + > ++ hdr = (const struct blobmsg_hdr *)blob_data(attr); > + if (name && !hdr->namelen) > + return false; > + > +@@ -68,29 +55,20 @@ static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool na > + return true; > + } > + > +-static const char* blobmsg_check_data(const struct blob_attr *attr, size_t len, size_t *data_len) > +-{ > +- char *limit = (char *) attr + len; > +- const char *data; > +- > +- *data_len = blobmsg_data_len(attr); > +- if (*data_len > blob_raw_len(attr)) > +- return NULL; > +- > +- data = blobmsg_data(attr); > +- if (data + *data_len > limit) > +- return NULL; > +- > +- return data; > +-} > +- > + bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len) > + { > + const char *data; > + size_t data_len; > + int id; > + > +- if (!blobmsg_check_name(attr, len, name)) > ++ if (len < sizeof(struct blob_attr)) > ++ return false; > ++ > ++ data_len = blob_raw_len(attr); > ++ if (data_len < sizeof(struct blob_attr) || data_len > len) > ++ return false; > ++ > ++ if (!blobmsg_check_name(attr, name)) > + return false; > + > + id = blob_id(attr); > +@@ -100,9 +78,8 @@ bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len) > + if (!blob_type[id]) > + return true; > + > +- data = blobmsg_check_data(attr, len, &data_len); > +- if (!data) > +- return false; > ++ data = blobmsg_data(attr); > ++ data_len = blobmsg_data_len(attr); > + > + return blob_check_type(data, data_len, blob_type[id]); > + } > +@@ -206,13 +183,13 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len, > + } > + > + __blob_for_each_attr(attr, data, len) { > +- hdr = blobmsg_hdr_from_blob(attr, len); > +- if (!hdr) > ++ if (!blobmsg_check_attr_len(attr, false, len)) > + return -1; > + > +- if (!blobmsg_hdr_valid_namelen(hdr, len)) > +- return -1; > ++ if (!blob_is_extended(attr)) > ++ continue; > + > ++ hdr = blob_data(attr); > + for (i = 0; i < policy_len; i++) { > + if (!policy[i].name) > + continue; > +@@ -224,9 +201,6 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len, > + if (blobmsg_namelen(hdr) != pslen[i]) > + continue; > + > +- if (!blobmsg_check_attr_len(attr, true, len)) > +- return -1; > +- > + if (tb[i]) > + continue;
Hi, On 20-06-20, Baptiste Jonglez wrote: > I should have added more details in the commit message: this fixes a > serious regression where procd fails to start some services, for instance > rpcd. See FS#3177. Any feedback on this regression fix? Thanks, Baptiste > This is the same regression that Rafał found in 19.07.2 and 19.07.3. > > I would like a review that I backported the right set of commits: I don't > think I missed one, but I'm not 100% sure that the 4 commits I backported > are all strictly necessary. > > In any case, they are all clean cherry-picks. > > On 13-06-20, Baptiste Jonglez wrote: > > From: Baptiste Jonglez <git@bitsofnetworks.org> > > > > Fixes: FS#3177 > > Cc: Felix Fietkau <nbd@nbd.name> > > Cc: Rafał Miłecki <rafal@milecki.pl> > > Signed-off-by: Baptiste Jonglez <git@bitsofnetworks.org> > > --- > > package/libs/libubox/Makefile | 2 +- > > ...s-iteration-in-the-blobmsg_check_arr.patch | 75 ++++++++++ > > ...sg-fix-length-in-blobmsg_check_array.patch | 28 ++++ > > ...-and-fix-name-length-checks-in-blobm.patch | 49 +++++++ > > ...21-blobmsg-fix-missing-length-checks.patch | 138 ++++++++++++++++++ > > 5 files changed, 291 insertions(+), 1 deletion(-) > > create mode 100644 package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch > > create mode 100644 package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch > > create mode 100644 package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch > > create mode 100644 package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch > > > > diff --git a/package/libs/libubox/Makefile b/package/libs/libubox/Makefile > > index e3a827c1ab..e4f1a6b503 100644 > > --- a/package/libs/libubox/Makefile > > +++ b/package/libs/libubox/Makefile > > @@ -1,7 +1,7 @@ > > include $(TOPDIR)/rules.mk > > > > PKG_NAME:=libubox > > -PKG_RELEASE=4 > > +PKG_RELEASE=5 > > > > PKG_SOURCE_PROTO:=git > > PKG_SOURCE_URL=$(PROJECT_GIT)/project/libubox.git > > diff --git a/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch b/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch > > new file mode 100644 > > index 0000000000..2834e10ee3 > > --- /dev/null > > +++ b/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch > > @@ -0,0 +1,75 @@ > > +From 5e75160f48785464f9213c6bc8c72b9372c5318b Mon Sep 17 00:00:00 2001 > > +From: =?UTF-8?q?Rafa=C5=82=20Mi=C5=82ecki?= <rafal@milecki.pl> > > +Date: Sat, 23 May 2020 13:18:51 +0200 > > +Subject: [PATCH] blobmsg: fix attrs iteration in the blobmsg_check_array_len() > > +MIME-Version: 1.0 > > +Content-Type: text/plain; charset=UTF-8 > > +Content-Transfer-Encoding: 8bit > > + > > +Starting with 75e300aeec25 ("blobmsg: fix wrong payload len passed from > > +blobmsg_check_array") blobmsg_check_array_len() gets *blob* length > > +passed as argument. It cannot be used with __blobmsg_for_each_attr() > > +which expects *data* length. > > + > > +Use blobmsg_for_each_attr() which calculates *data* length on its own. > > + > > +The same bug was already reported in the past and there was fix attempt > > +in the commit cd75136b1342 ("blobmsg: fix wrong payload len passed from > > +blobmsg_check_array"). That change made blobmsg_check_attr_len() calls > > +fail however. > > + > > +This is hopefully the correct & complete fix: > > +1. blobmsg_check_array_len() gets *blob* length > > +2. It calls blobmsg_check_attr_len() which requires *blob* length > > +3. It uses blobmsg_for_each_attr() which gets *data* length > > + > > +This fixes iterating over random memory treated as attrs. That was > > +resulting in check failing randomly for totally correct blobs. It's > > +critical e.g. for procd project with its instance_fill_array() failing > > +and procd not starting services. > > + > > +Fixes: 75e300aeec25 ("blobmsg: fix wrong payload len passed from blobmsg_check_array") > > +Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > > +--- > > + blobmsg.c | 10 ++++++---- > > + 1 file changed, 6 insertions(+), 4 deletions(-) > > + > > +diff --git a/blobmsg.c b/blobmsg.c > > +index 8b9877d..59045e1 100644 > > +--- a/blobmsg.c > > ++++ b/blobmsg.c > > +@@ -117,16 +117,18 @@ int blobmsg_check_array(const struct blob_attr *attr, int type) > > + return blobmsg_check_array_len(attr, type, blob_len(attr)); > > + } > > + > > +-int blobmsg_check_array_len(const struct blob_attr *attr, int type, size_t len) > > ++int blobmsg_check_array_len(const struct blob_attr *attr, int type, > > ++ size_t blob_len) > > + { > > + struct blob_attr *cur; > > ++ size_t rem; > > + bool name; > > + int size = 0; > > + > > + if (type > BLOBMSG_TYPE_LAST) > > + return -1; > > + > > +- if (!blobmsg_check_attr_len(attr, false, len)) > > ++ if (!blobmsg_check_attr_len(attr, false, blob_len)) > > + return -1; > > + > > + switch (blobmsg_type(attr)) { > > +@@ -140,11 +142,11 @@ int blobmsg_check_array_len(const struct blob_attr *attr, int type, size_t len) > > + return -1; > > + } > > + > > +- __blobmsg_for_each_attr(cur, attr, len) { > > ++ blobmsg_for_each_attr(cur, attr, rem) { > > + if (type != BLOBMSG_TYPE_UNSPEC && blobmsg_type(cur) != type) > > + return -1; > > + > > +- if (!blobmsg_check_attr_len(cur, name, len)) > > ++ if (!blobmsg_check_attr_len(cur, name, rem)) > > + return -1; > > + > > + size++; > > diff --git a/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch b/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch > > new file mode 100644 > > index 0000000000..9db2fb4f9f > > --- /dev/null > > +++ b/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch > > @@ -0,0 +1,28 @@ > > +From c2fc622b771f679e8f55060ac60cfe02b9a80995 Mon Sep 17 00:00:00 2001 > > +From: Felix Fietkau <nbd@nbd.name> > > +Date: Mon, 25 May 2020 13:44:20 +0200 > > +Subject: [PATCH] blobmsg: fix length in blobmsg_check_array > > + > > +blobmsg_check_array_len expects the length of the full attribute buffer, > > +not just the data length. > > +Due to other missing length checks (fixed in the next commit), this did > > +not show up as a test failure > > + > > +Signed-off-by: Felix Fietkau <nbd@nbd.name> > > +--- > > + blobmsg.c | 2 +- > > + 1 file changed, 1 insertion(+), 1 deletion(-) > > + > > +diff --git a/blobmsg.c b/blobmsg.c > > +index 59045e1..daaa9fc 100644 > > +--- a/blobmsg.c > > ++++ b/blobmsg.c > > +@@ -114,7 +114,7 @@ bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len) > > + > > + int blobmsg_check_array(const struct blob_attr *attr, int type) > > + { > > +- return blobmsg_check_array_len(attr, type, blob_len(attr)); > > ++ return blobmsg_check_array_len(attr, type, blob_raw_len(attr)); > > + } > > + > > + int blobmsg_check_array_len(const struct blob_attr *attr, int type, > > diff --git a/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch b/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch > > new file mode 100644 > > index 0000000000..a481208789 > > --- /dev/null > > +++ b/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch > > @@ -0,0 +1,49 @@ > > +From 639c29d19717616b809d9a1e9042461ab8024370 Mon Sep 17 00:00:00 2001 > > +From: Felix Fietkau <nbd@nbd.name> > > +Date: Mon, 25 May 2020 14:49:35 +0200 > > +Subject: [PATCH] blobmsg: simplify and fix name length checks in > > + blobmsg_check_name > > + > > +blobmsg_hdr_valid_namelen was omitted when name==false > > +The blob_len vs blobmsg_namelen changes were not taking into account > > +potential padding between name and data > > + > > +Signed-off-by: Felix Fietkau <nbd@nbd.name> > > +--- > > + blobmsg.c | 13 ++++--------- > > + 1 file changed, 4 insertions(+), 9 deletions(-) > > + > > +diff --git a/blobmsg.c b/blobmsg.c > > +index daaa9fc..308bef7 100644 > > +--- a/blobmsg.c > > ++++ b/blobmsg.c > > +@@ -48,8 +48,8 @@ static bool blobmsg_hdr_valid_namelen(const struct blobmsg_hdr *hdr, size_t len) > > + > > + static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool name) > > + { > > +- char *limit = (char *) attr + len; > > + const struct blobmsg_hdr *hdr; > > ++ uint16_t namelen; > > + > > + hdr = blobmsg_hdr_from_blob(attr, len); > > + if (!hdr) > > +@@ -58,16 +58,11 @@ static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool na > > + if (name && !hdr->namelen) > > + return false; > > + > > +- if (name && !blobmsg_hdr_valid_namelen(hdr, len)) > > +- return false; > > +- > > +- if ((char *) hdr->name + blobmsg_namelen(hdr) + 1 > limit) > > +- return false; > > +- > > +- if (blobmsg_namelen(hdr) > (blob_len(attr) - sizeof(struct blobmsg_hdr))) > > ++ namelen = blobmsg_namelen(hdr); > > ++ if (blob_len(attr) < (size_t)blobmsg_hdrlen(namelen)) > > + return false; > > + > > +- if (hdr->name[blobmsg_namelen(hdr)] != 0) > > ++ if (hdr->name[namelen] != 0) > > + return false; > > + > > + return true; > > diff --git a/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch b/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch > > new file mode 100644 > > index 0000000000..bfc440a329 > > --- /dev/null > > +++ b/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch > > @@ -0,0 +1,138 @@ > > +From 66195aee50424cbda0c2d858014e4cc58a2dc029 Mon Sep 17 00:00:00 2001 > > +From: Felix Fietkau <nbd@nbd.name> > > +Date: Mon, 25 May 2020 12:40:04 +0200 > > +Subject: [PATCH] blobmsg: fix missing length checks > > + > > +blobmsg_check_attr_len was calling blobmsg_check_data for some, but not all > > +attribute types. These checks was missing for arrays and tables. > > + > > +Additionally, the length check in blobmsg_check_data was a bit off, since > > +it was comparing the blobmsg data length against the raw blob attr length. > > + > > +Fix this by checking the raw blob length against the buffer length in > > +blobmsg_hdr_from_blob > > + > > +Signed-off-by: Felix Fietkau <nbd@nbd.name> > > +--- > > + blobmsg.c | 66 +++++++++++++++++-------------------------------------- > > + 1 file changed, 20 insertions(+), 46 deletions(-) > > + > > +diff --git a/blobmsg.c b/blobmsg.c > > +index 308bef7..7da4183 100644 > > +--- a/blobmsg.c > > ++++ b/blobmsg.c > > +@@ -30,31 +30,18 @@ bool blobmsg_check_attr(const struct blob_attr *attr, bool name) > > + return blobmsg_check_attr_len(attr, name, blob_raw_len(attr)); > > + } > > + > > +-static const struct blobmsg_hdr* blobmsg_hdr_from_blob(const struct blob_attr *attr, size_t len) > > +-{ > > +- if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) > > +- return NULL; > > +- > > +- return blob_data(attr); > > +-} > > +- > > +-static bool blobmsg_hdr_valid_namelen(const struct blobmsg_hdr *hdr, size_t len) > > +-{ > > +- if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr) + blobmsg_namelen(hdr) + 1) > > +- return false; > > +- > > +- return true; > > +-} > > +- > > +-static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool name) > > ++static bool blobmsg_check_name(const struct blob_attr *attr, bool name) > > + { > > + const struct blobmsg_hdr *hdr; > > + uint16_t namelen; > > + > > +- hdr = blobmsg_hdr_from_blob(attr, len); > > +- if (!hdr) > > ++ if (!blob_is_extended(attr)) > > ++ return !name; > > ++ > > ++ if (blob_len(attr) < sizeof(struct blobmsg_hdr)) > > + return false; > > + > > ++ hdr = (const struct blobmsg_hdr *)blob_data(attr); > > + if (name && !hdr->namelen) > > + return false; > > + > > +@@ -68,29 +55,20 @@ static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool na > > + return true; > > + } > > + > > +-static const char* blobmsg_check_data(const struct blob_attr *attr, size_t len, size_t *data_len) > > +-{ > > +- char *limit = (char *) attr + len; > > +- const char *data; > > +- > > +- *data_len = blobmsg_data_len(attr); > > +- if (*data_len > blob_raw_len(attr)) > > +- return NULL; > > +- > > +- data = blobmsg_data(attr); > > +- if (data + *data_len > limit) > > +- return NULL; > > +- > > +- return data; > > +-} > > +- > > + bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len) > > + { > > + const char *data; > > + size_t data_len; > > + int id; > > + > > +- if (!blobmsg_check_name(attr, len, name)) > > ++ if (len < sizeof(struct blob_attr)) > > ++ return false; > > ++ > > ++ data_len = blob_raw_len(attr); > > ++ if (data_len < sizeof(struct blob_attr) || data_len > len) > > ++ return false; > > ++ > > ++ if (!blobmsg_check_name(attr, name)) > > + return false; > > + > > + id = blob_id(attr); > > +@@ -100,9 +78,8 @@ bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len) > > + if (!blob_type[id]) > > + return true; > > + > > +- data = blobmsg_check_data(attr, len, &data_len); > > +- if (!data) > > +- return false; > > ++ data = blobmsg_data(attr); > > ++ data_len = blobmsg_data_len(attr); > > + > > + return blob_check_type(data, data_len, blob_type[id]); > > + } > > +@@ -206,13 +183,13 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len, > > + } > > + > > + __blob_for_each_attr(attr, data, len) { > > +- hdr = blobmsg_hdr_from_blob(attr, len); > > +- if (!hdr) > > ++ if (!blobmsg_check_attr_len(attr, false, len)) > > + return -1; > > + > > +- if (!blobmsg_hdr_valid_namelen(hdr, len)) > > +- return -1; > > ++ if (!blob_is_extended(attr)) > > ++ continue; > > + > > ++ hdr = blob_data(attr); > > + for (i = 0; i < policy_len; i++) { > > + if (!policy[i].name) > > + continue; > > +@@ -224,9 +201,6 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len, > > + if (blobmsg_namelen(hdr) != pslen[i]) > > + continue; > > + > > +- if (!blobmsg_check_attr_len(attr, true, len)) > > +- return -1; > > +- > > + if (tb[i]) > > + continue; > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff --git a/package/libs/libubox/Makefile b/package/libs/libubox/Makefile index e3a827c1ab..e4f1a6b503 100644 --- a/package/libs/libubox/Makefile +++ b/package/libs/libubox/Makefile @@ -1,7 +1,7 @@ include $(TOPDIR)/rules.mk PKG_NAME:=libubox -PKG_RELEASE=4 +PKG_RELEASE=5 PKG_SOURCE_PROTO:=git PKG_SOURCE_URL=$(PROJECT_GIT)/project/libubox.git diff --git a/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch b/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch new file mode 100644 index 0000000000..2834e10ee3 --- /dev/null +++ b/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch @@ -0,0 +1,75 @@ +From 5e75160f48785464f9213c6bc8c72b9372c5318b Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Rafa=C5=82=20Mi=C5=82ecki?= <rafal@milecki.pl> +Date: Sat, 23 May 2020 13:18:51 +0200 +Subject: [PATCH] blobmsg: fix attrs iteration in the blobmsg_check_array_len() +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Starting with 75e300aeec25 ("blobmsg: fix wrong payload len passed from +blobmsg_check_array") blobmsg_check_array_len() gets *blob* length +passed as argument. It cannot be used with __blobmsg_for_each_attr() +which expects *data* length. + +Use blobmsg_for_each_attr() which calculates *data* length on its own. + +The same bug was already reported in the past and there was fix attempt +in the commit cd75136b1342 ("blobmsg: fix wrong payload len passed from +blobmsg_check_array"). That change made blobmsg_check_attr_len() calls +fail however. + +This is hopefully the correct & complete fix: +1. blobmsg_check_array_len() gets *blob* length +2. It calls blobmsg_check_attr_len() which requires *blob* length +3. It uses blobmsg_for_each_attr() which gets *data* length + +This fixes iterating over random memory treated as attrs. That was +resulting in check failing randomly for totally correct blobs. It's +critical e.g. for procd project with its instance_fill_array() failing +and procd not starting services. + +Fixes: 75e300aeec25 ("blobmsg: fix wrong payload len passed from blobmsg_check_array") +Signed-off-by: Rafał Miłecki <rafal@milecki.pl> +--- + blobmsg.c | 10 ++++++---- + 1 file changed, 6 insertions(+), 4 deletions(-) + +diff --git a/blobmsg.c b/blobmsg.c +index 8b9877d..59045e1 100644 +--- a/blobmsg.c ++++ b/blobmsg.c +@@ -117,16 +117,18 @@ int blobmsg_check_array(const struct blob_attr *attr, int type) + return blobmsg_check_array_len(attr, type, blob_len(attr)); + } + +-int blobmsg_check_array_len(const struct blob_attr *attr, int type, size_t len) ++int blobmsg_check_array_len(const struct blob_attr *attr, int type, ++ size_t blob_len) + { + struct blob_attr *cur; ++ size_t rem; + bool name; + int size = 0; + + if (type > BLOBMSG_TYPE_LAST) + return -1; + +- if (!blobmsg_check_attr_len(attr, false, len)) ++ if (!blobmsg_check_attr_len(attr, false, blob_len)) + return -1; + + switch (blobmsg_type(attr)) { +@@ -140,11 +142,11 @@ int blobmsg_check_array_len(const struct blob_attr *attr, int type, size_t len) + return -1; + } + +- __blobmsg_for_each_attr(cur, attr, len) { ++ blobmsg_for_each_attr(cur, attr, rem) { + if (type != BLOBMSG_TYPE_UNSPEC && blobmsg_type(cur) != type) + return -1; + +- if (!blobmsg_check_attr_len(cur, name, len)) ++ if (!blobmsg_check_attr_len(cur, name, rem)) + return -1; + + size++; diff --git a/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch b/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch new file mode 100644 index 0000000000..9db2fb4f9f --- /dev/null +++ b/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch @@ -0,0 +1,28 @@ +From c2fc622b771f679e8f55060ac60cfe02b9a80995 Mon Sep 17 00:00:00 2001 +From: Felix Fietkau <nbd@nbd.name> +Date: Mon, 25 May 2020 13:44:20 +0200 +Subject: [PATCH] blobmsg: fix length in blobmsg_check_array + +blobmsg_check_array_len expects the length of the full attribute buffer, +not just the data length. +Due to other missing length checks (fixed in the next commit), this did +not show up as a test failure + +Signed-off-by: Felix Fietkau <nbd@nbd.name> +--- + blobmsg.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/blobmsg.c b/blobmsg.c +index 59045e1..daaa9fc 100644 +--- a/blobmsg.c ++++ b/blobmsg.c +@@ -114,7 +114,7 @@ bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len) + + int blobmsg_check_array(const struct blob_attr *attr, int type) + { +- return blobmsg_check_array_len(attr, type, blob_len(attr)); ++ return blobmsg_check_array_len(attr, type, blob_raw_len(attr)); + } + + int blobmsg_check_array_len(const struct blob_attr *attr, int type, diff --git a/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch b/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch new file mode 100644 index 0000000000..a481208789 --- /dev/null +++ b/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch @@ -0,0 +1,49 @@ +From 639c29d19717616b809d9a1e9042461ab8024370 Mon Sep 17 00:00:00 2001 +From: Felix Fietkau <nbd@nbd.name> +Date: Mon, 25 May 2020 14:49:35 +0200 +Subject: [PATCH] blobmsg: simplify and fix name length checks in + blobmsg_check_name + +blobmsg_hdr_valid_namelen was omitted when name==false +The blob_len vs blobmsg_namelen changes were not taking into account +potential padding between name and data + +Signed-off-by: Felix Fietkau <nbd@nbd.name> +--- + blobmsg.c | 13 ++++--------- + 1 file changed, 4 insertions(+), 9 deletions(-) + +diff --git a/blobmsg.c b/blobmsg.c +index daaa9fc..308bef7 100644 +--- a/blobmsg.c ++++ b/blobmsg.c +@@ -48,8 +48,8 @@ static bool blobmsg_hdr_valid_namelen(const struct blobmsg_hdr *hdr, size_t len) + + static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool name) + { +- char *limit = (char *) attr + len; + const struct blobmsg_hdr *hdr; ++ uint16_t namelen; + + hdr = blobmsg_hdr_from_blob(attr, len); + if (!hdr) +@@ -58,16 +58,11 @@ static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool na + if (name && !hdr->namelen) + return false; + +- if (name && !blobmsg_hdr_valid_namelen(hdr, len)) +- return false; +- +- if ((char *) hdr->name + blobmsg_namelen(hdr) + 1 > limit) +- return false; +- +- if (blobmsg_namelen(hdr) > (blob_len(attr) - sizeof(struct blobmsg_hdr))) ++ namelen = blobmsg_namelen(hdr); ++ if (blob_len(attr) < (size_t)blobmsg_hdrlen(namelen)) + return false; + +- if (hdr->name[blobmsg_namelen(hdr)] != 0) ++ if (hdr->name[namelen] != 0) + return false; + + return true; diff --git a/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch b/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch new file mode 100644 index 0000000000..bfc440a329 --- /dev/null +++ b/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch @@ -0,0 +1,138 @@ +From 66195aee50424cbda0c2d858014e4cc58a2dc029 Mon Sep 17 00:00:00 2001 +From: Felix Fietkau <nbd@nbd.name> +Date: Mon, 25 May 2020 12:40:04 +0200 +Subject: [PATCH] blobmsg: fix missing length checks + +blobmsg_check_attr_len was calling blobmsg_check_data for some, but not all +attribute types. These checks was missing for arrays and tables. + +Additionally, the length check in blobmsg_check_data was a bit off, since +it was comparing the blobmsg data length against the raw blob attr length. + +Fix this by checking the raw blob length against the buffer length in +blobmsg_hdr_from_blob + +Signed-off-by: Felix Fietkau <nbd@nbd.name> +--- + blobmsg.c | 66 +++++++++++++++++-------------------------------------- + 1 file changed, 20 insertions(+), 46 deletions(-) + +diff --git a/blobmsg.c b/blobmsg.c +index 308bef7..7da4183 100644 +--- a/blobmsg.c ++++ b/blobmsg.c +@@ -30,31 +30,18 @@ bool blobmsg_check_attr(const struct blob_attr *attr, bool name) + return blobmsg_check_attr_len(attr, name, blob_raw_len(attr)); + } + +-static const struct blobmsg_hdr* blobmsg_hdr_from_blob(const struct blob_attr *attr, size_t len) +-{ +- if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) +- return NULL; +- +- return blob_data(attr); +-} +- +-static bool blobmsg_hdr_valid_namelen(const struct blobmsg_hdr *hdr, size_t len) +-{ +- if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr) + blobmsg_namelen(hdr) + 1) +- return false; +- +- return true; +-} +- +-static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool name) ++static bool blobmsg_check_name(const struct blob_attr *attr, bool name) + { + const struct blobmsg_hdr *hdr; + uint16_t namelen; + +- hdr = blobmsg_hdr_from_blob(attr, len); +- if (!hdr) ++ if (!blob_is_extended(attr)) ++ return !name; ++ ++ if (blob_len(attr) < sizeof(struct blobmsg_hdr)) + return false; + ++ hdr = (const struct blobmsg_hdr *)blob_data(attr); + if (name && !hdr->namelen) + return false; + +@@ -68,29 +55,20 @@ static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool na + return true; + } + +-static const char* blobmsg_check_data(const struct blob_attr *attr, size_t len, size_t *data_len) +-{ +- char *limit = (char *) attr + len; +- const char *data; +- +- *data_len = blobmsg_data_len(attr); +- if (*data_len > blob_raw_len(attr)) +- return NULL; +- +- data = blobmsg_data(attr); +- if (data + *data_len > limit) +- return NULL; +- +- return data; +-} +- + bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len) + { + const char *data; + size_t data_len; + int id; + +- if (!blobmsg_check_name(attr, len, name)) ++ if (len < sizeof(struct blob_attr)) ++ return false; ++ ++ data_len = blob_raw_len(attr); ++ if (data_len < sizeof(struct blob_attr) || data_len > len) ++ return false; ++ ++ if (!blobmsg_check_name(attr, name)) + return false; + + id = blob_id(attr); +@@ -100,9 +78,8 @@ bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len) + if (!blob_type[id]) + return true; + +- data = blobmsg_check_data(attr, len, &data_len); +- if (!data) +- return false; ++ data = blobmsg_data(attr); ++ data_len = blobmsg_data_len(attr); + + return blob_check_type(data, data_len, blob_type[id]); + } +@@ -206,13 +183,13 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len, + } + + __blob_for_each_attr(attr, data, len) { +- hdr = blobmsg_hdr_from_blob(attr, len); +- if (!hdr) ++ if (!blobmsg_check_attr_len(attr, false, len)) + return -1; + +- if (!blobmsg_hdr_valid_namelen(hdr, len)) +- return -1; ++ if (!blob_is_extended(attr)) ++ continue; + ++ hdr = blob_data(attr); + for (i = 0; i < policy_len; i++) { + if (!policy[i].name) + continue; +@@ -224,9 +201,6 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len, + if (blobmsg_namelen(hdr) != pslen[i]) + continue; + +- if (!blobmsg_check_attr_len(attr, true, len)) +- return -1; +- + if (tb[i]) + continue;