[SSSD] [PATCH] krb5_child: Fix use after free

Jakub Hrozek jhrozek at redhat.com
Fri Apr 4 13:18:20 UTC 2014


On Thu, Apr 03, 2014 at 07:11:37PM +0200, Jakub Hrozek wrote:
> On Thu, Mar 20, 2014 at 05:53:31PM +0100, Lukas Slebodnik wrote:
> > On (20/03/14 17:21), Jakub Hrozek wrote:
> > >On Thu, Mar 20, 2014 at 05:00:00PM +0100, Sumit Bose wrote:
> > >> On Thu, Mar 20, 2014 at 04:20:59PM +0100, Lukas Slebodnik wrote:
> > >> > ehlo,
> > >> > 
> > >> > debug_prg_name is used in debug_fn and it was allocated under
> > >> > talloc context "kr". The variable "kr" was removed before the last debug
> > >> > messages in function main. It is very little change that it will be overridden.
> > >> > 
> > >> > It is possible to see this issue with exported environment variable
> > >> > TALLOC_FREE_FILL=255
> > >> 
> > >> I'm pretty sure the patch works as expected and fixes the isssue. But I
> > >> wonder if a different approach might be better? I think it does not make
> > >> sense to allocate debug_prg_name on a given talloc context but that it
> > >> would be better to just allocate it on NULL. This is e.g. done in the
> > >> ldap_child (here a talloc_free() is missing on exit but this would be a
> > >> different story).  Then debug_prg_name can even be allocate before kr
> > >> and the debug messages for a failed allocation of kr can use the right
> > >> program name and not "sssd".
> > >
> > >I agree, because also given that krb5_child is a short lived process,
> > >we don't care too much about possible leaks.
> > No problem.
> > 
> > New version attached.
> > 
> > LS
> 
> This version works for me, do you want to also add a talloc_free() on
> exit to be clean or do you also consider this not needed for short-lived
> processes?

Thinking again, it would be nicer to explicitly free the string on child
exit. Yes, the leak it's not a big deal for a short-lived process but it
would read better.



More information about the sssd-devel mailing list