On (06/06/16 18:55), Lukas Slebodnik wrote: >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 >- 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); I forgot to return error here. > } > > *_attrs = attrs; Upodated patch is attached. LS
>From 6b190337908f8167ce328cb763967a4c98f34cec 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 | 63 +++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 35 deletions(-) diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c index 5468929b691c6539cdf55f59be3560412e398f21..ee5c9bb2fca65488d651d909e575b32b406923e6 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,59 @@ 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); + return ret; } *_attrs = attrs; -- 2.7.4
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
