Hi Daniel,
On Mon, 28 Oct 2019 at 18:32, Daniel Danzberger <dan...@dd-wrt.com> wrote:
>
> Invalid reuse of pointers from uci_ptr can cause the rcpd to segfault on
> already freed memory.
> This bug could be trigged by calling 'set' with emtpy values on multiple non
> existing or already cleard options.
>
> For example:
> ubus call uci set
> '{"config":"network","section":"wan","values":{"proto":"static","foo":"",
> "bar":""}}'
>
> Signed-off-by: Daniel Danzberger <dan...@dd-wrt.com>
> ---
> uci.c | 55 ++++++++++++++++++++++++++-----------------------------
> 1 file changed, 26 insertions(+), 29 deletions(-)
>
> diff --git a/uci.c b/uci.c
> index 1587a19..e6ddbb5 100644
> --- a/uci.c
> +++ b/uci.c
> @@ -812,55 +812,58 @@ out:
> * option of if the existing options value differs from the blob value
> */
> static int
> -rpc_uci_merge_set(struct blob_attr *opt, struct uci_ptr *ptr)
> +rpc_uci_merge_set(struct blob_attr *opt,
> + struct uci_package *package,
> + const char *section)
> {
> + struct uci_ptr ptr = {};
> struct blob_attr *cur;
> int rem, rv;
>
> - ptr->o = NULL;
> - ptr->option = blobmsg_name(opt);
> - ptr->value = NULL;
Also zeroing out ptr->flags should suffice to fix the issue.
- When setting "proto", flags will have UCI_LOOKUP_COMPLETE set.
- That flag remained there when working on "foo", and uci_set()
thought we were setting empty string on existing option, calling
uci_delete() instead which caused ptr->s to be freed.
- Then when working on "bar", we access already freed up ptr->s
Preferably we could rework uci to allow setting empty string as value,
but that changes existing behavior and could break things.
Regards,
yousong
> + ptr.p = package;
> + ptr.section = section;
> + ptr.option = blobmsg_name(opt);
>
> - if (!rpc_uci_verify_name(ptr->option))
> + if (!rpc_uci_verify_name(ptr.option))
> return UBUS_STATUS_INVALID_ARGUMENT;
>
> - if (rpc_uci_lookup(ptr) || !ptr->s)
> + if (rpc_uci_lookup(&ptr) || !ptr.s)
> return UBUS_STATUS_NOT_FOUND;
>
> if (blobmsg_type(opt) == BLOBMSG_TYPE_ARRAY)
> {
> - if (ptr->o)
> - uci_delete(cursor, ptr);
> + if (ptr.o)
> + uci_delete(cursor, &ptr);
>
> rv = UBUS_STATUS_INVALID_ARGUMENT;
>
> blobmsg_for_each_attr(cur, opt, rem)
> {
> - if (!rpc_uci_format_blob(cur, &ptr->value))
> + if (!rpc_uci_format_blob(cur, &ptr.value))
> continue;
>
> - uci_add_list(cursor, ptr);
> + uci_add_list(cursor, &ptr);
> rv = 0;
> }
>
> return rv;
> }
> - else if (ptr->o && ptr->o->type == UCI_TYPE_LIST)
> + else if (ptr.o && ptr.o->type == UCI_TYPE_LIST)
> {
> - uci_delete(cursor, ptr);
> + uci_delete(cursor, &ptr);
>
> - if (!rpc_uci_format_blob(opt, &ptr->value))
> + if (!rpc_uci_format_blob(opt, &ptr.value))
> return UBUS_STATUS_INVALID_ARGUMENT;
>
> - uci_set(cursor, ptr);
> + uci_set(cursor, &ptr);
> }
> else
> {
> - if (!rpc_uci_format_blob(opt, &ptr->value))
> + if (!rpc_uci_format_blob(opt, &ptr.value))
> return UBUS_STATUS_INVALID_ARGUMENT;
>
> - if (!ptr->o || !ptr->o->v.string || strcmp(ptr->o->v.string,
> ptr->value))
> - uci_set(cursor, ptr);
> + if (!ptr.o || !ptr.o->v.string || strcmp(ptr.o->v.string,
> ptr.value))
> + uci_set(cursor, &ptr);
> }
>
> return 0;
> @@ -875,7 +878,7 @@ rpc_uci_set(struct ubus_context *ctx, struct ubus_object
> *obj,
> struct blob_attr *cur;
> struct uci_package *p = NULL;
> struct uci_element *e;
> - struct uci_ptr ptr = { 0 };
> + const char *package, *section;
> int rem, rv, err = 0;
>
> blobmsg_parse(rpc_uci_set_policy, __RPC_S_MAX, tb,
> @@ -892,17 +895,17 @@ rpc_uci_set(struct ubus_context *ctx, struct
> ubus_object *obj,
> !rpc_uci_verify_section(blobmsg_data(tb[RPC_S_SECTION])))
> return UBUS_STATUS_INVALID_ARGUMENT;
>
> - ptr.package = blobmsg_data(tb[RPC_S_CONFIG]);
> + package = blobmsg_data(tb[RPC_S_CONFIG]);
>
> - if (uci_load(cursor, ptr.package, &p))
> + if (uci_load(cursor, package, &p))
> return rpc_uci_status();
>
> if (tb[RPC_S_SECTION])
> {
> - ptr.section = blobmsg_data(tb[RPC_S_SECTION]);
> + section = blobmsg_data(tb[RPC_S_SECTION]);
> blobmsg_for_each_attr(cur, tb[RPC_S_VALUES], rem)
> {
> - rv = rpc_uci_merge_set(cur, &ptr);
> + rv = rpc_uci_merge_set(cur, p, section);
>
> if (rv)
> err = rv;
> @@ -916,12 +919,9 @@ rpc_uci_set(struct ubus_context *ctx, struct ubus_object
> *obj,
> tb[RPC_S_TYPE],
> tb[RPC_S_MATCH]))
> continue;
>
> - ptr.s = NULL;
> - ptr.section = e->name;
> -
> blobmsg_for_each_attr(cur, tb[RPC_S_VALUES], rem)
> {
> - rv = rpc_uci_merge_set(cur, &ptr);
> + rv = rpc_uci_merge_set(cur, p, e->name);
>
> if (rv)
> err = rv;
> @@ -929,9 +929,6 @@ rpc_uci_set(struct ubus_context *ctx, struct ubus_object
> *obj,
> }
> }
>
> - if (!err && !ptr.s)
> - err = UBUS_STATUS_NOT_FOUND;
> -
> if (!err)
> uci_save(cursor, p);
>
> --
> 2.23.0
>
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
