[OpenWrt-Devel] [PATCH v3 2/3] Replace use of blobmsg_check_attr by blobmsg_check_attr_safe

[Tobias Schramm]

blobmsg_check_attr_safe adds a length limit specifying the max offset from attr that
can be read safely

Signed-off-by: Tobias Schramm <tobleminer at gmail.com>
---
 blobmsg.c | 24 ++++++++++++++++++------
 blobmsg.h | 24 +++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/blobmsg.c b/blobmsg.c
index 8019c45..10f3801 100644
--- a/blobmsg.c
+++ b/blobmsg.c
@@ -31,19 +31,29 @@ blobmsg_namelen(const struct blobmsg_hdr *hdr)
 	return be16_to_cpu(hdr->namelen);
 }
 
-bool blobmsg_check_attr(const struct blob_attr *attr, bool name)
+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 +61,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 +72,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 +123,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))
 			return -1;
 
 		if (tb[i])
@@ -158,7 +170,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;
 
 			if (tb[i])
diff --git a/blobmsg.h b/blobmsg.h
index c75f1d9..d17b896 100644
--- a/blobmsg.h
+++ b/blobmsg.h
@@ -104,7 +104,29 @@ static inline int blobmsg_len(const struct blob_attr *attr)
 	return blobmsg_data_len(attr);
 }
 
-bool blobmsg_check_attr(const struct blob_attr *attr, bool name);
+/*
+ * blobmsg_check_attr_safe: safely validate a single untrusted attribute
+ *
+ * This method is a safe implementation of blobmsg_check_attr.
+ * It will limit all memory access performed on the blob to the
+ * range [attr, attr + len] (upper bound non inclusive) and is
+ * thus suited for checking untrusted blob attributes.
+ */
+bool blobmsg_check_attr_safe(const struct blob_attr *attr, bool name, size_t len);
+
+/*
+ * blobmsg_check_attr: validate a single attribute
+ *
+ * This method may be used with trusted data only. Providing
+ * malformed blobs will cause out of bounds memory access and
+ * crash your program or get your device 0wned.
+ */
+static inline 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_list(const struct blob_attr *attr, int type);
 
 /*
-- 
2.19.2


_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel