From e61f36189c4354a202f2b3bee86a026c7080690f Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov 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 cherry picked from commit cd1538bc92a319a19915d3a19efd9d5e013f7e75 and amended with commit e1b678c0cce73494d986610920b03956c1dbb62a by Lukas Slebodnik Reviewed-by: Pavel Březina --- 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