On Wed, Apr 02, 2014 at 10:29:15PM +0200, Jakub Hrozek wrote:
> Hi,
> 
> please see the attached patch that fixes a use-after-free bug in the
> SELinux provider.
> 
> Pair-Programmed-With: lsleb...@redhat.com

The patch solves the issue, but I think the issue was there in the first
place, because the 'order' string is somewhat hidden. I would suggest to
use a struct like

struct order_ctx {
    const char *order;
    char **order_array;
    size_t order_count;
};

It might be a bit overkill for this small usage, but I think it makes
the intention and the relation between order and order_array more clear.

bye,
Sumit

> From 538afda9760d0baae716b159e36da3c5edcaef29 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek <jhro...@redhat.com>
> Date: Wed, 2 Apr 2014 22:11:59 +0200
> Subject: [PATCH] IPA: Fix SELinux mapping order memory hierarchy
> 
> https://fedorahosted.org/sssd/ticket/2300
> 
> The list of SELinux mapping orders was allocated on tmp_ctx and parsed
> into an array. The array itself was correctly allocated on mem_ctx but
> its contents remained on tmp_ctx, leading to a use-after-free error.
> This patch fixes the memory hierarchy so that both the array and its
> contents are allocated on mem_ctx.
> ---
>  src/providers/ipa/ipa_selinux.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/src/providers/ipa/ipa_selinux.c b/src/providers/ipa/ipa_selinux.c
> index 
> 1fc0c68d0bdc7cc5131d61694db26a208c51d300..7c3ce45c606595b52b13beca749c1c9d93bd31f2
>  100644
> --- a/src/providers/ipa/ipa_selinux.c
> +++ b/src/providers/ipa/ipa_selinux.c
> @@ -601,21 +601,15 @@ static errno_t create_order_array(TALLOC_CTX *mem_ctx, 
> const char *map_order,
>          goto done;
>      }
>  
> -    order = talloc_strdup(tmp_ctx, map_order);
> -    if (order == NULL) {
> -        ret = ENOMEM;
> -        goto done;
> -    }
> -    len = strlen(order);
> -
>      /* The "order" string contains one or more SELinux user records
>       * separated by $. Now we need to create an array of string from
>       * this one string. First find out how many elements in the array
>       * will be. This way only one alloc will be necessary for the array
>       */
>      order_count = 1;
> +    len = strlen(map_order);
>      for (i = 0; i < len; i++) {
> -        if (order[i] == '$') order_count++;
> +        if (map_order[i] == '$') order_count++;
>      }
>  
>      order_array = talloc_array(tmp_ctx, char *, order_count);
> @@ -624,6 +618,12 @@ static errno_t create_order_array(TALLOC_CTX *mem_ctx, 
> const char *map_order,
>          goto done;
>      }
>  
> +    order = talloc_strdup(order_array, map_order);
> +    if (order == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
>      /* Now fill the array with pointers to the original string. Also
>       * use binary zeros to make multiple string out of the one.
>       */
> -- 
> 1.9.0
> 

> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Reply via email to