[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