From a1523d76b016ed46501f61e38ad38999d6c66f52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0tetiar?= Date: Thu, 19 Dec 2019 11:25:56 +0100 Subject: fix blob parsing vulnerability by using blob_parse_untrusted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit blob_parse expects blobs from trusted inputs, but it can be supplied with possibly malicious blobs from untrusted inputs as well, which might lead to undefined behaviour and/or crash of ubus daemon. In order to prevent such conditions, switch to blob_parse_untrusted which should hopefully handle such untrusted inputs appropriately. Signed-off-by: Petr Štetiar --- cli.c | 2 +- libubus-internal.h | 2 +- libubus-io.c | 4 ++-- libubus-obj.c | 6 +++--- libubus-req.c | 6 +++--- libubus.c | 4 ++-- tests/fuzz/test-fuzz.c | 2 +- ubusd.h | 2 +- ubusd_acl.c | 2 +- ubusd_proto.c | 6 +++--- 10 files changed, 18 insertions(+), 18 deletions(-) diff --git a/cli.c b/cli.c index 421f244..f566279 100644 --- a/cli.c +++ b/cli.c @@ -472,7 +472,7 @@ ubus_cli_monitor_cb(struct ubus_context *ctx, uint32_t seq, struct blob_attr *ms bool send; char *data; - blob_parse(msg, tb, policy, UBUS_MONITOR_MAX); + blob_parse_untrusted(msg, blob_raw_len(msg), tb, policy, UBUS_MONITOR_MAX); if (!tb[UBUS_MONITOR_CLIENT] || !tb[UBUS_MONITOR_PEER] || diff --git a/libubus-internal.h b/libubus-internal.h index 8cf99b3..24477a0 100644 --- a/libubus-internal.h +++ b/libubus-internal.h @@ -17,7 +17,7 @@ extern struct blob_buf b; extern const struct ubus_method watch_method; -struct blob_attr **ubus_parse_msg(struct blob_attr *msg); +struct blob_attr **ubus_parse_msg(struct blob_attr *msg, size_t len); bool ubus_validate_hdr(struct ubus_msghdr *hdr); void ubus_handle_data(struct uloop_fd *u, unsigned int events); int ubus_send_msg(struct ubus_context *ctx, uint32_t seq, diff --git a/libubus-io.c b/libubus-io.c index 81c1cd1..ba1016d 100644 --- a/libubus-io.c +++ b/libubus-io.c @@ -43,9 +43,9 @@ static const struct blob_attr_info ubus_policy[UBUS_ATTR_MAX] = { static struct blob_attr *attrbuf[UBUS_ATTR_MAX]; -__hidden struct blob_attr **ubus_parse_msg(struct blob_attr *msg) +__hidden struct blob_attr **ubus_parse_msg(struct blob_attr *msg, size_t len) { - blob_parse(msg, attrbuf, ubus_policy, UBUS_ATTR_MAX); + blob_parse_untrusted(msg, len, attrbuf, ubus_policy, UBUS_ATTR_MAX); return attrbuf; } diff --git a/libubus-obj.c b/libubus-obj.c index 2580b24..29cbb2b 100644 --- a/libubus-obj.c +++ b/libubus-obj.c @@ -121,7 +121,7 @@ void __hidden ubus_process_obj_msg(struct ubus_context *ctx, struct ubus_msghdr_ struct ubus_object *obj; uint32_t objid; void *prev_data = NULL; - attrbuf = ubus_parse_msg(buf->data); + attrbuf = ubus_parse_msg(buf->data, blob_raw_len(buf->data)); if (!attrbuf[UBUS_ATTR_OBJID]) return; @@ -160,7 +160,7 @@ void __hidden ubus_process_obj_msg(struct ubus_context *ctx, struct ubus_msghdr_ static void ubus_add_object_cb(struct ubus_request *req, int type, struct blob_attr *msg) { struct ubus_object *obj = req->priv; - struct blob_attr **attrbuf = ubus_parse_msg(msg); + struct blob_attr **attrbuf = ubus_parse_msg(msg, blob_raw_len(msg)); if (!attrbuf[UBUS_ATTR_OBJID]) return; @@ -240,7 +240,7 @@ int ubus_add_object(struct ubus_context *ctx, struct ubus_object *obj) static void ubus_remove_object_cb(struct ubus_request *req, int type, struct blob_attr *msg) { struct ubus_object *obj = req->priv; - struct blob_attr **attrbuf = ubus_parse_msg(msg); + struct blob_attr **attrbuf = ubus_parse_msg(msg, blob_raw_len(msg)); if (!attrbuf[UBUS_ATTR_OBJID]) return; diff --git a/libubus-req.c b/libubus-req.c index fd9a548..ae9d192 100644 --- a/libubus-req.c +++ b/libubus-req.c @@ -31,7 +31,7 @@ static void req_data_cb(struct ubus_request *req, int type, struct blob_attr *da if (!req->data_cb) return; - attr = ubus_parse_msg(data); + attr = ubus_parse_msg(data, blob_raw_len(data)); if (!attr[UBUS_ATTR_DATA]) return; @@ -328,7 +328,7 @@ int ubus_notify(struct ubus_context *ctx, struct ubus_object *obj, static bool ubus_get_status(struct ubus_msghdr_buf *buf, int *ret) { - struct blob_attr **attrbuf = ubus_parse_msg(buf->data); + struct blob_attr **attrbuf = ubus_parse_msg(buf->data, blob_raw_len(buf->data)); if (!attrbuf[UBUS_ATTR_STATUS]) return false; @@ -435,7 +435,7 @@ static void ubus_process_notify_status(struct ubus_request *req, int id, struct if (!id) { /* first id: ubusd's status message with a list of ids */ - tb = ubus_parse_msg(buf->data); + tb = ubus_parse_msg(buf->data, blob_raw_len(buf->data)); if (tb[UBUS_ATTR_SUBSCRIBERS]) { blob_for_each_attr(cur, tb[UBUS_ATTR_SUBSCRIBERS], rem) { if (!blob_check_type(blob_data(cur), blob_len(cur), BLOB_ATTR_INT32)) diff --git a/libubus.c b/libubus.c index b405891..91f317c 100644 --- a/libubus.c +++ b/libubus.c @@ -139,7 +139,7 @@ static void ubus_lookup_cb(struct ubus_request *ureq, int type, struct blob_attr struct blob_attr **attr; req = container_of(ureq, struct ubus_lookup_request, req); - attr = ubus_parse_msg(msg); + attr = ubus_parse_msg(msg, blob_raw_len(msg)); if (!attr[UBUS_ATTR_OBJID] || !attr[UBUS_ATTR_OBJPATH] || !attr[UBUS_ATTR_OBJTYPE]) @@ -175,7 +175,7 @@ static void ubus_lookup_id_cb(struct ubus_request *req, int type, struct blob_at struct blob_attr **attr; uint32_t *id = req->priv; - attr = ubus_parse_msg(msg); + attr = ubus_parse_msg(msg, blob_raw_len(msg)); if (!attr[UBUS_ATTR_OBJID]) return; diff --git a/tests/fuzz/test-fuzz.c b/tests/fuzz/test-fuzz.c index 9922ff9..7a7a1eb 100644 --- a/tests/fuzz/test-fuzz.c +++ b/tests/fuzz/test-fuzz.c @@ -28,7 +28,7 @@ static void _ubus_parse_msg(const uint8_t *data, size_t size) if (blob_pad_len(attr) > UBUS_MAX_MSGLEN) return; - ubus_parse_msg(attr); + ubus_parse_msg(attr, size); } int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) diff --git a/ubusd.h b/ubusd.h index 867cde9..923e43d 100644 --- a/ubusd.h +++ b/ubusd.h @@ -72,7 +72,7 @@ struct ubus_msg_buf *ubus_msg_new(void *data, int len, bool shared); void ubus_msg_send(struct ubus_client *cl, struct ubus_msg_buf *ub); ssize_t ubus_msg_writev(int fd, struct ubus_msg_buf *ub, size_t offset); void ubus_msg_free(struct ubus_msg_buf *ub); -struct blob_attr **ubus_parse_msg(struct blob_attr *msg); +struct blob_attr **ubus_parse_msg(struct blob_attr *msg, size_t len); struct ubus_client *ubusd_proto_new_client(int fd, uloop_fd_handler cb); void ubusd_proto_receive_message(struct ubus_client *cl, struct ubus_msg_buf *ub); diff --git a/ubusd_acl.c b/ubusd_acl.c index f19df9a..e426a4a 100644 --- a/ubusd_acl.c +++ b/ubusd_acl.c @@ -549,7 +549,7 @@ static int ubusd_reply_query(struct ubus_client *cl, struct ubus_msg_buf *ub, st static int ubusd_acl_recv(struct ubus_client *cl, struct ubus_msg_buf *ub, const char *method, struct blob_attr *msg) { if (!strcmp(method, "query")) - return ubusd_reply_query(cl, ub, ubus_parse_msg(ub->data), msg); + return ubusd_reply_query(cl, ub, ubus_parse_msg(ub->data, blob_raw_len(ub->data)), msg); return UBUS_STATUS_INVALID_COMMAND; } diff --git a/ubusd_proto.c b/ubusd_proto.c index 4dd89dd..4746605 100644 --- a/ubusd_proto.c +++ b/ubusd_proto.c @@ -34,9 +34,9 @@ static const struct blob_attr_info ubus_policy[UBUS_ATTR_MAX] = { [UBUS_ATTR_GROUP] = { .type = BLOB_ATTR_STRING }, }; -struct blob_attr **ubus_parse_msg(struct blob_attr *msg) +struct blob_attr **ubus_parse_msg(struct blob_attr *msg, size_t len) { - blob_parse(msg, attrbuf, ubus_policy, UBUS_ATTR_MAX); + blob_parse_untrusted(msg, len, attrbuf, ubus_policy, UBUS_ATTR_MAX); return attrbuf; } @@ -454,7 +454,7 @@ void ubusd_proto_receive_message(struct ubus_client *cl, struct ubus_msg_buf *ub /* Note: no callback should free the `ub` buffer that's always done right after the callback finishes */ if (cb) - ret = cb(cl, ub, ubus_parse_msg(ub->data)); + ret = cb(cl, ub, ubus_parse_msg(ub->data, blob_raw_len(ub->data))); else ret = UBUS_STATUS_INVALID_COMMAND; -- cgit v1.2.1