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

Lukas Slebodnik lslebodn at redhat.com
Tue Apr 8 09:02:34 UTC 2014


On (07/04/14 22:21), Sumit Bose wrote:
>On Mon, Apr 07, 2014 at 09:20:50PM +0200, Lukas Slebodnik wrote:
>> On (07/04/14 21:01), Sumit Bose wrote:
>> >On Mon, Apr 07, 2014 at 08:39:07PM +0200, Jakub Hrozek wrote:
>> >> On Mon, Apr 07, 2014 at 08:35:20PM +0200, Lukas Slebodnik wrote:
>> >> > On (07/04/14 20:30), Jakub Hrozek wrote:
>> >> > >On Mon, Apr 07, 2014 at 08:03:32PM +0200, Lukas Slebodnik wrote:
>> >> > >> On (07/04/14 18:53), Jakub Hrozek wrote:
>> >> > >> >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 :-)
>> >> > >> 
>> >> > >> You didn't test your patch :-)
>> >> > >
>> >> > >I left that to you :-)
>> >> > >
>> >> > >See another proposal below:
>> >> > >
>> >> > >> 
>> >> > >> 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);
>> >> > >> 
>> >> > >> // snip
>> >> > >> done:
>> >> > >>     krb5_cleanup(kr);
>> >> > >>     talloc_free(kr);
>> >> > >>     ^^^^^^^^^^^^^^^
>> >> > >> //debug_prg_name is freed
>> >> > >> 
>> >> > >>     if (ret == EOK) {
>> >> > >>         DEBUG(SSSDBG_TRACE_FUNC, "krb5_child completed successfully\n");
>> >> > >>         ^^^^^^
>> >> > >> // use after free
>> >> > >          talloc_free(tmp_str);
>> >> > >>         exit(0);
>> >> > >>     } else {
>> >> > >>         DEBUG(SSSDBG_CRIT_FAILURE, "krb5_child failed!\n");
>> >> > >>         ^^^^^^
>> >> > >> // use after free
>> >> > >          talloc_free(tmp_str);
>> >> > >>         exit(-1);
>> >> > >>     }
>> >> > >> 
>> >> > >
>> >> > >Or just:
>> >> > > done:
>> >> > >     krb5_cleanup(kr);
>> >> > >     talloc_free(kr);
>> >> > >     if (ret == EOK) {
>> >> > >         DEBUG(SSSDBG_TRACE_FUNC, "krb5_child completed successfully\n");
>> >> > >         talloc_free(tmp_str);
>> >> > >         rv = 0;
>> >> > >     } else {
>> >> > >         DEBUG(SSSDBG_CRIT_FAILURE, "krb5_child failed!\n");
>> >> > >         talloc_free(tmp_str);
>> >> > >         rv = -1;
>> >> > >     }
>> >> > >     exit(rv);
>> >> > 
>> >> > And now, you can compare your solution with the first patch from this thread
>> >> > and then try to read replies to the 1st mail from this thread :-)
>> >> 
>> >> Except first patch didn't allocate on NULL? I think that was the meat of
>> >> Sumit's comments..
>> >
>> >yes, just allocate on NULL and free it explicitly in the end to make
>> >valgrind happy. The reason for suggesting NULL is that debug_prg_name is
>> >global and I think putting it under a specific talloc-hierarchy does not
>> >make sense.
>> >
>> but there is no difference. You allocate debug_prg_name on NULL and then steal
>> to another talloc context. (like in ldap_child)
>
>Why do you have to steal it? Just allocate on NULL and free it in the
>end.
>
>bye,
>Sumit
>
Attached patch use the same approach as in ldap_child.

LS
-------------- next part --------------
>From 17d1d7194505b4c047256e703a1d21fb009c36a6 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Tue, 8 Apr 2014 10:56:22 +0200
Subject: [PATCH] krb5_child: Fix use after free in debug message

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
---
 src/providers/krb5/krb5_child.c | 26 +++++++++++++++-----------
 src/providers/ldap/ldap_child.c |  1 +
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 22264e484d880c1527ec01e2d6c58c73c52cccd8..7c8403868eec3ac3f2ec51938ad870929dbdc279 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -2000,14 +2000,9 @@ int main(int argc, const char *argv[])
 
     DEBUG_INIT(debug_level);
 
-    kr = talloc_zero(NULL, struct krb5_req);
-    if (kr == NULL) {
-        DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
-        exit(-1);
-    }
-
-    debug_prg_name = talloc_asprintf(kr, "[sssd[krb5_child[%d]]]", getpid());
+    debug_prg_name = talloc_asprintf(NULL, "[sssd[krb5_child[%d]]]", getpid());
     if (!debug_prg_name) {
+        debug_prg_name = "[sssd[krb5_child]]";
         DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
         ret = ENOMEM;
         goto done;
@@ -2022,6 +2017,14 @@ int main(int argc, const char *argv[])
 
     DEBUG(SSSDBG_TRACE_FUNC, "krb5_child started.\n");
 
+    kr = talloc_zero(NULL, struct krb5_req);
+    if (kr == NULL) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
+        ret = ENOMEM;
+        goto done;
+    }
+    talloc_steal(kr, debug_prg_name);
+
     ret = k5c_recv_data(kr, STDIN_FILENO, &offline);
     if (ret != EOK) {
         goto done;
@@ -2080,13 +2083,14 @@ int main(int argc, const char *argv[])
     }
 
 done:
-    krb5_cleanup(kr);
-    talloc_free(kr);
     if (ret == EOK) {
         DEBUG(SSSDBG_TRACE_FUNC, "krb5_child completed successfully\n");
-        exit(0);
+        ret = 0;
     } else {
         DEBUG(SSSDBG_CRIT_FAILURE, "krb5_child failed!\n");
-        exit(-1);
+        ret = -1;
     }
+    krb5_cleanup(kr);
+    talloc_free(kr);
+    exit(ret);
 }
diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index 34f23ec80916210b29872caecf90c9855906ddf6..0e5e1614addedd293e93d0a33b3ed97f3ffa4c16 100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -462,6 +462,7 @@ int main(int argc, const char *argv[])
 
     debug_prg_name = talloc_asprintf(NULL, "[sssd[ldap_child[%d]]]", getpid());
     if (!debug_prg_name) {
+        debug_prg_name = "[sssd[ldap_child]]";
         DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
         goto fail;
     }
-- 
1.9.0



More information about the sssd-devel mailing list