Re: [SSSD] [PATCH] IPA: Fix SELinux mapping order memory hierarchy
On Thu, Apr 03, 2014 at 05:13:19PM +0200, Jakub Hrozek wrote:
> On Thu, Apr 03, 2014 at 01:25:04PM +0200, Sumit Bose wrote:
> > On Thu, Apr 03, 2014 at 12:58:03PM +0200, Jakub Hrozek wrote:
> > > On Thu, Apr 03, 2014 at 09:45:46AM +0200, Sumit Bose wrote:
> > > > 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
> > >
> > > Would you accept this small patch for 1.11 and your proposed (and
> > > better, I agree) change for master?
> >
> > sure, ACK.
> >
> > But I think it would be better to apply this one to 1.11 and master and
> > then add another patch with the changes to master.
> >
> > bye,
> > Sumit
>
> OK, thank you for the acks. I filed another ticket for the refactor and
> gave it to Michal:
> https://fedorahosted.org/sssd/ticket/2304
Pushed upstream:
master: 355b8a655cfcc4e783077d12f76b55da1d23fb87
sssd-1-11: ac93a2d27415abd730aa1063b1689def8be9dbe9
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] IPA: Fix SELinux mapping order memory hierarchy
On Thu, Apr 03, 2014 at 01:25:04PM +0200, Sumit Bose wrote:
> On Thu, Apr 03, 2014 at 12:58:03PM +0200, Jakub Hrozek wrote:
> > On Thu, Apr 03, 2014 at 09:45:46AM +0200, Sumit Bose wrote:
> > > 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
> >
> > Would you accept this small patch for 1.11 and your proposed (and
> > better, I agree) change for master?
>
> sure, ACK.
>
> But I think it would be better to apply this one to 1.11 and master and
> then add another patch with the changes to master.
>
> bye,
> Sumit
OK, thank you for the acks. I filed another ticket for the refactor and
gave it to Michal:
https://fedorahosted.org/sssd/ticket/2304
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] IPA: Fix SELinux mapping order memory hierarchy
On Thu, Apr 03, 2014 at 12:58:03PM +0200, Jakub Hrozek wrote:
> On Thu, Apr 03, 2014 at 09:45:46AM +0200, Sumit Bose wrote:
> > 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
>
> Would you accept this small patch for 1.11 and your proposed (and
> better, I agree) change for master?
sure, ACK.
But I think it would be better to apply this one to 1.11 and master and
then add another patch with the changes to master.
bye,
Sumit
> ___
> 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
Re: [SSSD] [PATCH] IPA: Fix SELinux mapping order memory hierarchy
On Thu, Apr 03, 2014 at 09:45:46AM +0200, Sumit Bose wrote:
> 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
Would you accept this small patch for 1.11 and your proposed (and
better, I agree) change for master?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] IPA: Fix SELinux mapping order memory hierarchy
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
> 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
