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

[Yousong Zhou]

Hi Daniel,

On Mon, 28 Oct 2019 at 18:32, Daniel Danzberger <daniel at 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 at 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 at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

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