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

Jakub Hrozek jhrozek at redhat.com
Mon Apr 7 16:53:21 UTC 2014


On Fri, Apr 04, 2014 at 03:41:38PM +0200, Lukas Slebodnik wrote:
> On (04/04/14 15:18), Jakub Hrozek wrote:
> >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.
> 
> With the first version of patch string was freed, because it was alocated
> under "kr" context.
> Now, you should decide which version do you want to push :-)
> 
> LS

I think it's easier to explain with a patch :-)
-------------- next part --------------
>From 0d199bd33937fccc677e68548e206c51c6e8d5bc Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 7 Apr 2014 18:48:58 +0200
Subject: [PATCH] Steal the debug_prg_name

---
 src/providers/krb5/krb5_child.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 764f6ac7bf57b1f7d882961b7c6fa518818aaf23..aec0d9389dd4f3ae005b73ff12ca8293cee7560f 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -2013,6 +2013,7 @@ int main(int argc, const char *argv[])
         DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
         exit(-1);
     }
+    talloc_steal(kr, debug_prg_name);
 
     if (debug_fd != -1) {
         ret = set_debug_file_from_fd(debug_fd);
-- 
1.9.0



More information about the sssd-devel mailing list