On (15/03/16 12:31), Lukas Slebodnik wrote: >On (15/03/16 11:26), Pavel Březina wrote: >>On 03/07/2016 01:33 PM, Lukas Slebodnik wrote: >>>On (07/03/16 12:12), Pavel Březina wrote: >>>>On 03/07/2016 10:14 AM, Lukas Slebodnik wrote: >>>>>ehlo, >>>>> >>>>>simple aptch is attached. >>>> >>>>When there, can you also talloc_free(attrs) on error? Thanks. >>>See updated patch >> >>Some time has passed now so I take it as you won't implement Sumit's >>suggestion. >I will but I have tasks with higher priority :-) > Updated patch is attached.
LS
>From e616ea9e8e58d0ed70b56edc338184d783597004 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik <[email protected]> Date: Mon, 6 Jun 2016 18:15:44 +0200 Subject: [PATCH] TOOLS: Prevent dereference of null pointer VAR_CHECK is called with (var, EOK, ...) EOK would be returned in case of "var != EOK" and output argument _attrs would not be initialized. Therefore there could be dereference of null pointer after calling function usermod_build_attrs. --- src/tools/sss_sync_ops.c | 62 +++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 35 deletions(-) diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c index 5468929b691c6539cdf55f59be3560412e398f21..e47aef37d2b89b28b7ff18555473136bdf7596cf 100644 --- a/src/tools/sss_sync_ops.c +++ b/src/tools/sss_sync_ops.c @@ -37,13 +37,6 @@ #define ATTR_NAME_SEP '=' #define ATTR_VAL_SEP ',' -#define VAR_CHECK(var, val, attr, msg) do { \ - if (var != (val)) { \ - DEBUG(SSSDBG_CRIT_FAILURE, msg" attribute: %s\n", attr); \ - return val; \ - } \ -} while(0) - static int attr_name_val_split(TALLOC_CTX *mem_ctx, const char *nameval, char **_name, char ***_values, int *_nvals) { @@ -200,8 +193,9 @@ static int usermod_build_attrs(TALLOC_CTX *mem_ctx, int lock, struct sysdb_attrs **_attrs) { - int ret; + int ret = EOK; struct sysdb_attrs *attrs; + const char *attr_name = NULL; attrs = sysdb_new_attrs(mem_ctx); if (attrs == NULL) { @@ -209,60 +203,58 @@ static int usermod_build_attrs(TALLOC_CTX *mem_ctx, } if (shell) { + attr_name = SYSDB_SHELL; ret = sysdb_attrs_add_string(attrs, - SYSDB_SHELL, + attr_name, shell); - VAR_CHECK(ret, EOK, SYSDB_SHELL, - "Could not add attribute to changeset\n"); } - if (home) { + if (ret == EOK && home) { + attr_name = SYSDB_HOMEDIR; ret = sysdb_attrs_add_string(attrs, - SYSDB_HOMEDIR, + attr_name, home); - VAR_CHECK(ret, EOK, SYSDB_HOMEDIR, - "Could not add attribute to changeset\n"); } - if (gecos) { + if (ret == EOK && gecos) { + attr_name = SYSDB_GECOS; ret = sysdb_attrs_add_string(attrs, - SYSDB_GECOS, + attr_name, gecos); - VAR_CHECK(ret, EOK, SYSDB_GECOS, - "Could not add attribute to changeset\n"); } - if (uid) { + if (ret == EOK && uid) { + attr_name = SYSDB_UIDNUM; ret = sysdb_attrs_add_long(attrs, - SYSDB_UIDNUM, + attr_name, uid); - VAR_CHECK(ret, EOK, SYSDB_UIDNUM, - "Could not add attribute to changeset\n"); } - if (gid) { + if (ret == EOK && gid) { + attr_name = SYSDB_GIDNUM; ret = sysdb_attrs_add_long(attrs, - SYSDB_GIDNUM, + attr_name, gid); - VAR_CHECK(ret, EOK, SYSDB_GIDNUM, - "Could not add attribute to changeset\n"); } - if (lock == DO_LOCK) { + if (ret == EOK && lock == DO_LOCK) { + attr_name = SYSDB_DISABLED; ret = sysdb_attrs_add_string(attrs, - SYSDB_DISABLED, + attr_name, "true"); - VAR_CHECK(ret, EOK, SYSDB_DISABLED, - "Could not add attribute to changeset\n"); } - if (lock == DO_UNLOCK) { + if (ret == EOK && lock == DO_UNLOCK) { + attr_name = SYSDB_DISABLED; /* PAM code checks for 'false' value in SYSDB_DISABLED attribute */ ret = sysdb_attrs_add_string(attrs, - SYSDB_DISABLED, + attr_name, "false"); - VAR_CHECK(ret, EOK, SYSDB_DISABLED, - "Could not add attribute to changeset\n"); + } + + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Could not add attribute [%s] to changeset.\n", attr_name); } *_attrs = attrs; -- 2.7.4
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
