[OpenWrt-Devel] rpcd: uci: fix segfault and double free on set method

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 <daniel@dd-wrt.com>
---
 uci.c | 55 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 26 insertions(+), 29 deletions(-)

Hi Daniel,

On Mon, 28 Oct 2019 at 18:32, Daniel Danzberger <daniel@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 <daniel@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

Hi Daniel, Yousong,

thanks for the reporting issue and the proposed patch. I'd prefer to go
with a minimal variant which merely zeroes the flags to avoid touching
too much code.

Imho the current uci_set() behavior of freeing uci_ptr members without
zeroing them is a bug that should be corrected as well.

~ Jo