[OpenWrt-Devel] [PATCH 18.06] libubox: backport additional length-checking fixes
Baptiste Jonglez
baptiste at bitsofnetworks.org
Sat Jun 20 13:55:46 EDT 2020
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 at bitsofnetworks.org>
>
> Fixes: FS#3177
> Cc: Felix Fietkau <nbd at nbd.name>
> Cc: Rafał Miłecki <rafal at milecki.pl>
> Signed-off-by: Baptiste Jonglez <git at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20200620/12c99425/attachment.sig>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
More information about the openwrt-devel
mailing list