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

Lukas Slebodnik lslebodn at redhat.com
Thu Mar 20 16:53:31 UTC 2014


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
-------------- next part --------------
>From 538d120d8ca5d87ebf3eb5e11015d8c2bb06a80e Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Thu, 20 Mar 2014 17:47:34 +0100
Subject: [PATCH] childs: Fix use after free in krb5_child

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

This patch also uses temporary variable for storing "debug program name".
Because if talloc_asprintf failed debug_prg_name would be NULL and NULL would
be used in debug_fn
---
 src/providers/krb5/krb5_child.c | 16 +++++++++-------
 src/providers/ldap/ldap_child.c |  6 ++++--
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 1bff0e99b586e0b1a54a660320f91cdab275fbe1..0be1b9650d1d4cf08228671c9c6108ba8e92abd2 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -1918,6 +1918,7 @@ int main(int argc, const char *argv[])
     poptContext pc;
     int debug_fd = -1;
     errno_t ret;
+    char *tmp_name;
 
     struct poptOption long_options[] = {
         POPT_AUTOHELP
@@ -1950,19 +1951,20 @@ int main(int argc, const char *argv[])
 
     DEBUG_INIT(debug_level);
 
+    tmp_name = talloc_asprintf(NULL, "[sssd[krb5_child[%d]]]", getpid());
+    if (!tmp_name) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
+        ret = ENOMEM;
+        goto done;
+    }
+    debug_prg_name = tmp_name;
+
     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());
-    if (!debug_prg_name) {
-        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
-        ret = ENOMEM;
-        goto done;
-    }
-
     if (debug_fd != -1) {
         ret = set_debug_file_from_fd(debug_fd);
         if (ret != EOK) {
diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index 34f23ec80916210b29872caecf90c9855906ddf6..897c17800b915c84b249cfe3cd213e36f9c25eb6 100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -424,6 +424,7 @@ int main(int argc, const char *argv[])
     uint8_t *buf = NULL;
     ssize_t len = 0;
     const char *ccname = NULL;
+    char *tmp_name;
     time_t expire_time = 0;
     struct input_buffer *ibuf = NULL;
     struct response *resp = NULL;
@@ -460,11 +461,12 @@ int main(int argc, const char *argv[])
 
     DEBUG_INIT(debug_level);
 
-    debug_prg_name = talloc_asprintf(NULL, "[sssd[ldap_child[%d]]]", getpid());
-    if (!debug_prg_name) {
+    tmp_name = talloc_asprintf(NULL, "[sssd[ldap_child[%d]]]", getpid());
+    if (!tmp_name) {
         DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
         goto fail;
     }
+    debug_prg_name = tmp_name;
 
     if (debug_fd != -1) {
         ret = set_debug_file_from_fd(debug_fd);
-- 
1.8.5.3



More information about the sssd-devel mailing list