[PATCH] LDAP: Fix crash when processing nested groups

[Jakub Hrozek]

https://fedorahosted.org/sssd/ticket/1932

There is a rather strange workaround in the nested groups processing
code that calls tevent_req_post outside _send(). However, it broke in
certain situations where the tevent_req_call resulted in req being freed,
which freed state by extension and then the subsequent _post call was a
use-after-free. This patch saves the two variables used outside state so
that it's safe to use them even after the callback.

[Jakub Hrozek]

On Tue, Aug 06, 2013 at 09:25:55PM +0200, Jakub Hrozek wrote:
>  https://fedorahosted.org/sssd/ticket/1932
>  
>  There is a rather strange workaround in the nested groups processing
>  code that calls tevent_req_post outside _send(). However, it broke in
>  certain situations where the tevent_req_call resulted in req being freed,
>  which freed state by extension and then the subsequent _post call was a
>  use-after-free. This patch saves the two variables used outside state so
>  that it's safe to use them even after the callback. 
Sorry I forgot to make it clear that this commit only applies on the 1.9
branch. The rewritten nested groups processing that is present on 1.10
and 1.11 doesn't suffer from this problem.

[Jakub Hrozek]

On Tue, Aug 06, 2013 at 09:35:22PM +0200, Jakub Hrozek wrote:
>  On Tue, Aug 06, 2013 at 09:25:55PM +0200, Jakub Hrozek wrote:
>  > https://fedorahosted.org/sssd/ticket/1932
>  > 
>  > There is a rather strange workaround in the nested groups processing
>  > code that calls tevent_req_post outside _send(). However, it broke in
>  > certain situations where the tevent_req_call resulted in req being freed,
>  > which freed state by extension and then the subsequent _post call was a
>  > use-after-free. This patch saves the two variables used outside state so
>  > that it's safe to use them even after the callback.
>  
>  Sorry I forgot to make it clear that this commit only applies on the 1.9
>  branch. The rewritten nested groups processing that is present on 1.10
>  and 1.11 doesn't suffer from this problem. 
Oh and one more remark -- Pavel suggested that we might even want to
call tevent_req_post before tevent_req_done. Since tevent_req_post
pretty much just schedules an immediate event, I think that would work,
too, but I'm not 100% sure I'm not missing any detail, so I opted for
this safer but uglier way, especially considering that the fix is only
targeting a maintenance branch.

[Pavel Březina]

On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
>  https://fedorahosted.org/sssd/ticket/1932
>
>  There is a rather strange workaround in the nested groups processing
>  code that calls tevent_req_post outside _send(). However, it broke in
>  certain situations where the tevent_req_call resulted in req being freed,
>  which freed state by extension and then the subsequent _post call was a
>  use-after-free. This patch saves the two variables used outside state so
>  that it's safe to use them even after the callback. 
Hi,
great catch! Sometimes I wish tevent_req_done() would schedule immediate 
event, instead of firing the callback directly.

Nack to the patch but ack to the solution. The same code is also in 
sdap_nested_group_lookup_user(). Can you fix it as well? It maybe can't 
be triggered, but who knows...

[Jakub Hrozek]

On Wed, Aug 07, 2013 at 09:53:13AM +0200, Pavel Březina wrote:
>  On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
>  >https://fedorahosted.org/sssd/ticket/1932
>  >
>  >There is a rather strange workaround in the nested groups processing
>  >code that calls tevent_req_post outside _send(). However, it broke in
>  >certain situations where the tevent_req_call resulted in req being freed,
>  >which freed state by extension and then the subsequent _post call was a
>  >use-after-free. This patch saves the two variables used outside state so
>  >that it's safe to use them even after the callback.
>  
>  Hi,
>  great catch! Sometimes I wish tevent_req_done() would schedule
>  immediate event, instead of firing the callback directly.
>  
>  Nack to the patch but ack to the solution. The same code is also in
>  sdap_nested_group_lookup_user(). Can you fix it as well? It maybe
>  can't be triggered, but who knows... 
I think it can be triggered if all users are out of all search bases.
New patch is attached.

[Pavel Březina]

On 08/07/2013 11:59 AM, Jakub Hrozek wrote:
>  On Wed, Aug 07, 2013 at 09:53:13AM +0200, Pavel Březina wrote:
> > On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
> >> https://fedorahosted.org/sssd/ticket/1932
> >>
> >> There is a rather strange workaround in the nested groups processing
> >> code that calls tevent_req_post outside _send(). However, it broke in
> >> certain situations where the tevent_req_call resulted in req being freed,
> >> which freed state by extension and then the subsequent _post call was a
> >> use-after-free. This patch saves the two variables used outside state so
> >> that it's safe to use them even after the callback.
> >
> > Hi,
> > great catch! Sometimes I wish tevent_req_done() would schedule
> > immediate event, instead of firing the callback directly.
> >
> > Nack to the patch but ack to the solution. The same code is also in
> > sdap_nested_group_lookup_user(). Can you fix it as well? It maybe
> > can't be triggered, but who knows...
>
>  I think it can be triggered if all users are out of all search bases.
>  New patch is attached. 
Ack.

BTW there was lately several use-after-free issues found by users. We 
should start testing our patches with TALLOC_FREE_FILL=0 - this will 
overwrite freed memory with zeros on talloc_free() so if there is 
anything like that in covered code, we will hit that.

[Jakub Hrozek]

On Wed, Aug 07, 2013 at 11:36:37PM +0200, Pavel Březina wrote:
>  On 08/07/2013 11:59 AM, Jakub Hrozek wrote:
>  >On Wed, Aug 07, 2013 at 09:53:13AM +0200, Pavel Březina wrote:
>  >>On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
>  >>>https://fedorahosted.org/sssd/ticket/1932
>  >>>
>  >>>There is a rather strange workaround in the nested groups processing
>  >>>code that calls tevent_req_post outside _send(). However, it broke in
>  >>>certain situations where the tevent_req_call resulted in req being freed,
>  >>>which freed state by extension and then the subsequent _post call was a
>  >>>use-after-free. This patch saves the two variables used outside state so
>  >>>that it's safe to use them even after the callback.
>  >>
>  >>Hi,
>  >>great catch! Sometimes I wish tevent_req_done() would schedule
>  >>immediate event, instead of firing the callback directly.
>  >>
>  >>Nack to the patch but ack to the solution. The same code is also in
>  >>sdap_nested_group_lookup_user(). Can you fix it as well? It maybe
>  >>can't be triggered, but who knows...
>  >
>  >I think it can be triggered if all users are out of all search bases.
>  >New patch is attached.
>  
>  Ack. 
Pushed to sssd-1-9

>  
>  BTW there was lately several use-after-free issues found by users.
>  We should start testing our patches with TALLOC_FREE_FILL=0 - this
>  will overwrite freed memory with zeros on talloc_free() so if there
>  is anything like that in covered code, we will hit that. 
Yep :-)

We should also be using MALLOC_PERTURB, which does the same thing for
mallocated memory, making it easier to detect uninitialized access to
heap memory.

Can you add a note about TALLOC_FREE_FILL to
https://fedorahosted.org/sssd/wiki/DevelTips ?

[Lukas Slebodnik]

On (08/08/13 00:50), Jakub Hrozek wrote:
> On Wed, Aug 07, 2013 at 11:36:37PM +0200, Pavel Březina wrote:
> > On 08/07/2013 11:59 AM, Jakub Hrozek wrote:
> > >On Wed, Aug 07, 2013 at 09:53:13AM +0200, Pavel Březina wrote:
> > >>On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
> > >>>https://fedorahosted.org/sssd/ticket/1932
> > >>>
> > >>>There is a rather strange workaround in the nested groups processing
> > >>>code that calls tevent_req_post outside _send(). However, it broke in
> > >>>certain situations where the tevent_req_call resulted in req being
> freed,
> > >>>which freed state by extension and then the subsequent _post call was a
> > >>>use-after-free. This patch saves the two variables used outside state so
> > >>>that it's safe to use them even after the callback.
> > >>
> > >>Hi,
> > >>great catch! Sometimes I wish tevent_req_done() would schedule
> > >>immediate event, instead of firing the callback directly.
> > >>
> > >>Nack to the patch but ack to the solution. The same code is also in
> > >>sdap_nested_group_lookup_user(). Can you fix it as well? It maybe
> > >>can't be triggered, but who knows...
> > >
> > >I think it can be triggered if all users are out of all search bases.
> > >New patch is attached.
> > 
> > Ack.
>
> Pushed to sssd-1-9
>
> > 
> > BTW there was lately several use-after-free issues found by users.
> > We should start testing our patches with TALLOC_FREE_FILL=0 - this
> > will overwrite freed memory with zeros on talloc_free() so if there
> > is anything like that in covered code, we will hit that.
>
> Yep :-)
>
> We should also be using MALLOC_PERTURB, which does the same thing for
> mallocated memory, making it easier to detect uninitialized access to
> heap memory.
>
> Can you add a note about TALLOC_FREE_FILL to
> https://fedorahosted.org/sssd/wiki/DevelTips ? 
+1
but I would use different value than 0, because it will be impossible
to distinguish between zero initialized memory and released memory (use after
free)

I would prefer very big value for example 0xFD (FreeD memory)

LS

[Jakub Hrozek]

On Thu, Aug 08, 2013 at 08:33:17AM +0200, Lukas Slebodnik wrote:
>  On (08/08/13 00:50), Jakub Hrozek wrote:
>  >On Wed, Aug 07, 2013 at 11:36:37PM +0200, Pavel Březina wrote:
>  >> On 08/07/2013 11:59 AM, Jakub Hrozek wrote:
>  >> >On Wed, Aug 07, 2013 at 09:53:13AM +0200, Pavel Březina wrote:
>  >> >>On 08/06/2013 09:25 PM, Jakub Hrozek wrote:
>  >> >>>https://fedorahosted.org/sssd/ticket/1932
>  >> >>>
>  >> >>>There is a rather strange workaround in the nested groups
> processing
>  >> >>>code that calls tevent_req_post outside _send(). However, it broke
> in
>  >> >>>certain situations where the tevent_req_call resulted in req being
> freed,
>  >> >>>which freed state by extension and then the subsequent _post call
> was a
>  >> >>>use-after-free. This patch saves the two variables used outside
> state so
>  >> >>>that it's safe to use them even after the callback.
>  >> >>
>  >> >>Hi,
>  >> >>great catch! Sometimes I wish tevent_req_done() would schedule
>  >> >>immediate event, instead of firing the callback directly.
>  >> >>
>  >> >>Nack to the patch but ack to the solution. The same code is also in
>  >> >>sdap_nested_group_lookup_user(). Can you fix it as well? It maybe
>  >> >>can't be triggered, but who knows...
>  >> >
>  >> >I think it can be triggered if all users are out of all search bases.
>  >> >New patch is attached.
>  >> 
>  >> Ack.
>  >
>  >Pushed to sssd-1-9
>  >
>  >> 
>  >> BTW there was lately several use-after-free issues found by users.
>  >> We should start testing our patches with TALLOC_FREE_FILL=0 - this
>  >> will overwrite freed memory with zeros on talloc_free() so if there
>  >> is anything like that in covered code, we will hit that.
>  >
>  >Yep :-)
>  >
>  >We should also be using MALLOC_PERTURB, which does the same thing for
>  >mallocated memory, making it easier to detect uninitialized access to
>  >heap memory.
>  >
>  >Can you add a note about TALLOC_FREE_FILL to
>  >https://fedorahosted.org/sssd/wiki/DevelTips ?
>  +1
>  but I would use different value than 0, because it will be impossible
>  to distinguish between zero initialized memory and released memory (use after
>  free)
>  
>  I would prefer very big value for example 0xFD (FreeD memory)
>  
>  LS 
You can even use random values:

export MALLOC_PERTURB_=$(($RANDOM % 255 + 1))