From e61f36189c4354a202f2b3bee86a026c7080690f Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikhono@redhat.com>
Date: Tue, 29 Jan 2019 12:13:30 +0100
Subject: [PATCH 5/9] providers/proxy: fixed erroneous free of orig_grp
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Function `remove_duplicate_group_members(mem_ctx, orig_grp, new_grp)`
in case of empty orig_grp would return as a result:
```
*new_grp = talloc_steal(mem_ctx, orig_grp);
```
Since mem_ctx is freed in caller function that leads to deallocation
of orig_grp and to "use after free" bug.

Code was changes so remove_duplicate_group_members() behaves consistently
and always returns a new group created in given mem context.

Reviewed-by: Pavel Březina <pbrezina@redhat.com>

cherry picked from commit cd1538bc92a319a19915d3a19efd9d5e013f7e75
and amended with
commit e1b678c0cce73494d986610920b03956c1dbb62a by Lukas Slebodnik

Reviewed-by: Pavel Březina <pbrezina@redhat.com>
---
 src/providers/proxy/proxy_id.c | 70 ++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index 3e8a43ad7..25e866bfb 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -597,8 +597,32 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
         return ENOMEM;
     }
 
+    grp = talloc(tmp_ctx, struct group);
+    if (grp == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, "talloc failed.\n");
+        ret = ENOMEM;
+        goto done;
+    }
+
+    grp->gr_gid = orig_grp->gr_gid;
+
+    grp->gr_name = talloc_strdup(grp, orig_grp->gr_name);
+    if (grp->gr_name == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
+        ret = ENOMEM;
+        goto done;
+    }
+
+    grp->gr_passwd = talloc_strdup(grp, orig_grp->gr_passwd);
+    if (grp->gr_passwd == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
+        ret = ENOMEM;
+        goto done;
+    }
+
     if (orig_grp->gr_mem == NULL) {
-        ret = ENOENT;
+        grp->gr_mem = NULL;
+        ret = EOK;
         goto done;
     }
 
@@ -607,7 +631,14 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
     orig_member_count = i;
 
     if (orig_member_count == 0) {
-        ret = ENOENT;
+        grp->gr_mem = talloc_zero_array(grp, char *, 1);
+        if (grp->gr_mem == NULL) {
+            DEBUG(SSSDBG_OP_FAILURE, "talloc_zero_array failed.\n");
+            ret = ENOMEM;
+            goto done;
+        }
+        grp->gr_mem[0] = NULL;
+        ret = EOK;
         goto done;
     }
 
@@ -636,14 +667,8 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
 
     member_count = hash_count(member_tbl);
     if (member_count == 0) {
-        ret = ENOENT;
-        goto done;
-    }
-
-    grp = talloc(tmp_ctx, struct group);
-    if (grp == NULL) {
-        DEBUG(SSSDBG_OP_FAILURE, "talloc failed.\n");
-        ret = ENOMEM;
+        DEBUG(SSSDBG_CRIT_FAILURE, "Empty resulting hash table - must be internal bug.\n");
+        ret = EINVAL;
         goto done;
     }
 
@@ -673,32 +698,13 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
     }
     grp->gr_mem[i] = NULL;
 
-    grp->gr_gid = orig_grp->gr_gid;
-
-    grp->gr_name = talloc_strdup(grp, orig_grp->gr_name);
-    if (grp->gr_name == NULL) {
-        DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
-        ret = ENOMEM;
-        goto done;
-    }
-
-    grp->gr_passwd = talloc_strdup(grp, orig_grp->gr_passwd);
-    if (grp->gr_passwd == NULL) {
-        DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
-        ret = ENOMEM;
-        goto done;
-    }
-
-    *_grp = talloc_steal(mem_ctx, grp);
     ret = EOK;
 
 done:
-    talloc_zfree(tmp_ctx);
-
-    if (ret == ENOENT) {
-        *_grp = talloc_steal(mem_ctx, orig_grp);
-        ret = EOK;
+    if (ret == EOK) {
+        *_grp = talloc_steal(mem_ctx, grp);
     }
+    talloc_zfree(tmp_ctx);
 
     return ret;
 }
-- 
2.21.1