From patchwork Sat Jun 13 18:17:40 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Baptiste Jonglez X-Patchwork-Id: 1308732 X-Patchwork-Delegate: hauke@hauke-m.de Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.openwrt.org (client-ip=2607:7c80:54:e::133; helo=bombadil.infradead.org; envelope-from=openwrt-devel-bounces+incoming=patchwork.ozlabs.org@lists.openwrt.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=bitsofnetworks.org Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=lists.infradead.org header.i=@lists.infradead.org header.a=rsa-sha256 header.s=bombadil.20170209 header.b=KMFskWnT; dkim-atps=neutral Received: from bombadil.infradead.org (bombadil.infradead.org [IPv6:2607:7c80:54:e::133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 49km6y1GY3z9sR4 for ; Sun, 14 Jun 2020 04:18:17 +1000 (AEST) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Subject:MIME-Version:Message-Id:Date:To :From:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References: List-Owner; bh=++EmXFn0O6GJhSW2lv2v6Gzwh6qM5WXNNRJawMoTqx8=; b=KMFskWnT7Od9+V yvtkBLqziODdw01hByxc+qWxcV/NeFF4X0EYalFtLiuWxB1L99MffzPiRy2eYjfyAsoQrnW74yYDG y5CfjpXrWSm/O02VT8dmHH4zr3x052S8BE24EYMi7CcADp5p15cb1OkIVw758/eHD7nCHXBamrgMT iY+u6WHppIgtJt2cPQmPjeAEqGa6OnkslXeHAVtiXdgH2Lb87kWa3r++Ra9/RPxKoxgtWsgh6xLvs ceXGKFRMU9Rsyeo6jXIAT4s7l5yDJw9zKMZDW5TjV+m4YUFW5y+6fhlZud8jgODpgQZiCfUHcL4Ht Jz5qQLRPUXUEv+MIrZIg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jkAjO-0004oc-VR; Sat, 13 Jun 2020 18:18:14 +0000 Received: from mails.bitsofnetworks.org ([2001:912:1800:ff::131]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jkAjK-0004ng-Rb for openwrt-devel@lists.openwrt.org; Sat, 13 Jun 2020 18:18:13 +0000 Received: from [2001:912:1800::ac1] (helo=fedic.lan) by mails.bitsofnetworks.org with esmtp (Exim 4.89) (envelope-from ) id 1jkAj1-0007ke-Ts; Sat, 13 Jun 2020 20:17:51 +0200 From: Baptiste Jonglez To: openwrt-devel@lists.openwrt.org Date: Sat, 13 Jun 2020 20:17:40 +0200 Message-Id: <20200613181740.988875-1-baptiste@bitsofnetworks.org> X-Mailer: git-send-email 2.27.0 MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200613_111811_058571_647C9BD2 X-CRM114-Status: GOOD ( 17.05 ) X-Spam-Score: 0.0 (/) X-Spam-Report: SpamAssassin version 3.4.4 on bombadil.infradead.org summary: Content analysis details: (0.0 points) pts rule name description ---- ---------------------- -------------------------------------------------- 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.0 SPF_PASS SPF: sender matches SPF record Subject: [OpenWrt-Devel] [PATCH 18.06] libubox: backport additional length-checking fixes X-BeenThere: openwrt-devel@lists.openwrt.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: =?utf-8?b?UmFmYcWCIE1pxYJlY2tp?= , Baptiste Jonglez , Felix Fietkau Sender: "openwrt-devel" Errors-To: openwrt-devel-bounces+incoming=patchwork.ozlabs.org@lists.openwrt.org From: Baptiste Jonglez Fixes: FS#3177 Cc: Felix Fietkau Cc: Rafał Miłecki Signed-off-by: Baptiste Jonglez --- 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?= +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 +--- + 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 +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 +--- + 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 +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 +--- + 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 +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 +--- + 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;