From: Amos Jeffries <squid3@treenet.co.nz>
Date: Sat, 17 Nov 2012 12:31:44 +0000 (-0700)
Subject: ext_kerberos_ldap_group: Memory Leaks
X-Git-Tag: SQUID_3_4_0_1~493
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=4ad7aabf20a9fd42902639d0c0ac567d45c10ad8;p=thirdparty%2Fsquid.git

ext_kerberos_ldap_group: Memory Leaks

* Allocate sufficient memory to nul-terminate the cleaned input buffer
  on worst-case input obfuscation level.

* Free temporary parsing buffers, and parsed group structures when parse
  aborts on failure.

* Remove several useless if() conditionals for simpler code flow:
 - Squid xfree() handles NULL pointers.
 - Squid safe_free() handles NULL pointers and assigns NULL after freeing
 - assigning x->next from local NULL pointer is okay.


 Detected by Coverity Scan. Issues 743377, 743378, 740412, 740417, 740418
   740419, 740420, 740421, 740448, 740458, 740342, 740422, 740449, 740450
   740423, 740424.
---

diff --git a/helpers/external_acl/kerberos_ldap_group/kerberos_ldap_group.cc b/helpers/external_acl/kerberos_ldap_group/kerberos_ldap_group.cc
index 3117bf3e3d..464b673235 100644
--- a/helpers/external_acl/kerberos_ldap_group/kerberos_ldap_group.cc
+++ b/helpers/external_acl/kerberos_ldap_group/kerberos_ldap_group.cc
@@ -231,7 +231,7 @@ main(int argc, char *const argv[])
     char *up=NULL, *dp=NULL, *np=NULL;
     char *nuser, *nuser8 = NULL, *netbios;
     char *c;
-    int opt,gopt;
+    int opt;
     struct main_args margs;
 
     setbuf(stdout, NULL);
@@ -336,8 +336,8 @@ main(int argc, char *const argv[])
     }
 
     debug((char *) "%s| %s: INFO: Starting version %s\n", LogTime(), PROGRAM, KERBEROS_LDAP_GROUP_VERSION);
+    int gopt = 0;
     if (create_gd(&margs)) {
-        gopt = 0;
         if ( margs.glist != NULL ) {
             debug((char *) "%s| %s: FATAL: Error in group list: %s\n", LogTime(), PROGRAM, margs.glist ? margs.glist : "NULL");
             SEND_ERR("");
@@ -441,9 +441,9 @@ main(int argc, char *const argv[])
             clean_args(&margs);
             exit(-1);
         }
-        if ( gopt && user ) {
+        if (gopt) {
             if ((group = strtok(NULL, " \n")) != NULL) {
-                debug((char *) "%s| %s: INFO: Read group list %s from stdin\n", LogTime(), PROGRAM,group);
+                debug((char *) "%s| %s: INFO: Read group list %s from stdin\n", LogTime(), PROGRAM, group);
                 rfc1738_unescape(group);
                 if (margs.groups) {
                     clean_gd(margs.groups);
diff --git a/helpers/external_acl/kerberos_ldap_group/support_group.cc b/helpers/external_acl/kerberos_ldap_group/support_group.cc
index f6f0868960..a43c59e712 100644
--- a/helpers/external_acl/kerberos_ldap_group/support_group.cc
+++ b/helpers/external_acl/kerberos_ldap_group/support_group.cc
@@ -42,6 +42,17 @@ init_gd(void) {
     return gdsp;
 }
 
+void
+free_gd(struct gdstruct *gdsp) {
+    while(gdsp) {
+        struct gdstruct *gdspn = gdsp->next;
+        xfree(gdsp->group);
+        xfree(gdsp->domain);
+        xfree(gdsp);
+        gdsp = gdspn;
+    }
+}
+
 char *utf8dup(struct main_args *margs);
 
 char *
@@ -101,35 +112,24 @@ char *hex_utf_char(struct main_args *margs, int flag);
 char *
 hex_utf_char(struct main_args *margs, int flag)
 {
-    char *up;
-    char *upd;
-    char *ul;
-    int a, n, nl, ival, ichar;
+    int ival, ichar;
     int iUTF2, iUTF3, iUTF4;
 
-    if (flag) {
-        up = margs->ulist;
-    } else {
-        up = margs->tlist;
-    }
-
+    char *up = (flag ? margs->ulist : margs->tlist);
     if (!up)
         return NULL;
 
-    upd = strrchr(up, '@');
-    if (upd)
-        a = upd - up;
-    else
-        a = strlen(up);
+    char *upd = strrchr(up, '@');
+    size_t a = (upd ? (upd - up) : strlen(up) );
 
-    ul = (char *) xmalloc(strlen(up));
-    n = 0;
-    nl = 0;
+    char *ul = (char *) xmalloc(strlen(up)+1);
+    size_t n = 0;
+    int nl = 0;
     iUTF2 = 0;
     iUTF3 = 0;
     iUTF4 = 0;
 
-    while (n < (int) strlen(up)) {
+    while (n < strlen(up)) {
         if (flag && n == a)
             break;
         if (up[n] == '@') {
@@ -286,8 +286,7 @@ hex_utf_char(struct main_args *margs, int flag)
     if (iUTF2 || iUTF3 || iUTF4) {
         debug((char *) "%s| %s: INFO: iUTF2: %d iUTF3: %d iUTF4: %d\n", LogTime(), PROGRAM, iUTF2, iUTF3, iUTF4);
         debug((char *) "%s| %s: WARNING: Invalid UTF-8 sequence for Unicode %s\n", LogTime(), PROGRAM, ul);
-        if (ul)
-            xfree(ul);
+        xfree(ul);
         return NULL;
     }
     if (flag && upd)
@@ -299,7 +298,6 @@ int
 create_gd(struct main_args *margs)
 {
     char *gp, *dp;
-    char *hp1, *hp2, *up;
     char *p;
     struct gdstruct *gdsp = NULL, *gdspn = NULL;
     /*
@@ -321,28 +319,38 @@ create_gd(struct main_args *margs)
      *
      *
      */
-    hp1 = hex_utf_char(margs, 0);
-    hp2 = hex_utf_char(margs, 1);
-    up = utf8dup(margs);
+    char *hp1 = hex_utf_char(margs, 0);
+    char *hp2 = hex_utf_char(margs, 1);
+    char *up = utf8dup(margs);
+    char *gpbuf = NULL;
+
+    // release the allocated UTF decoding buffers
+#define cleanup() { \
+    xfree(gpbuf); \
+    xfree(hp1); \
+    xfree(hp2); \
+    free_gd(gdsp); \
+ }
+
     p = up;
     if (hp1) {
         if (hp2) {
             if (up) {
-                p = (char *) xmalloc(strlen(up) + strlen(hp1) + strlen(hp2) + 2);
+                gpbuf = p = (char *) xmalloc(strlen(up) + strlen(hp1) + strlen(hp2) + 2);
                 strcpy(p, up);
                 strcat(p, ":");
                 strcat(p, hp1);
                 strcat(p, ":");
                 strcat(p, hp2);
             } else {
-                p = (char *) xmalloc(strlen(hp1) + strlen(hp2) + 1);
+                gpbuf = p = (char *) xmalloc(strlen(hp1) + strlen(hp2) + 1);
                 strcpy(p, hp1);
                 strcat(p, ":");
                 strcat(p, hp2);
             }
         } else {
             if (up) {
-                p = (char *) xmalloc(strlen(up) + strlen(hp1) + 1);
+                gpbuf = p = (char *) xmalloc(strlen(up) + strlen(hp1) + 1);
                 strcpy(p, up);
                 strcat(p, ":");
                 strcat(p, hp1);
@@ -352,7 +360,7 @@ create_gd(struct main_args *margs)
     } else {
         if (hp2) {
             if (up) {
-                p = (char *) xmalloc(strlen(up) + strlen(hp2) + 1);
+                gpbuf = p = (char *) xmalloc(strlen(up) + strlen(hp2) + 1);
                 strcpy(p, up);
                 strcat(p, ":");
                 strcat(p, hp2);
@@ -367,6 +375,7 @@ create_gd(struct main_args *margs)
 
     if (!p) {
         debug((char *) "%s| %s: ERROR: No groups defined.\n", LogTime(), PROGRAM);
+        cleanup();
         return (1);
     }
     while (*p) {		/* loop over group list */
@@ -377,18 +386,19 @@ create_gd(struct main_args *margs)
         if (*p == '@') {	/* end of group name - start of domain name */
             if (p == gp) {	/* empty group name not allowed */
                 debug((char *) "%s| %s: ERROR: No group defined for domain %s\n", LogTime(), PROGRAM, p);
+                cleanup();
                 return (1);
             }
             *p = '\0';
             ++p;
             gdsp = init_gd();
-            gdsp->group = gp;
-            if (gdspn)		/* Have already an existing structure */
-                gdsp->next = gdspn;
+            gdsp->group = xstrdup(gp);
+            gdsp->next = gdspn;
             dp = p;		/* after @ starts new domain name */
         } else if (*p == ':') {	/* end of group name or end of domain name */
             if (p == gp) {	/* empty group name not allowed */
                 debug((char *) "%s| %s: ERROR: No group defined for domain %s\n", LogTime(), PROGRAM, p);
+                cleanup();
                 return (1);
             }
             *p = '\0';
@@ -398,9 +408,8 @@ create_gd(struct main_args *margs)
                 dp = NULL;
             } else {		/* end of group name and no domain name */
                 gdsp = init_gd();
-                gdsp->group = gp;
-                if (gdspn)	/* Have already an existing structure */
-                    gdsp->next = gdspn;
+                gdsp->group = xstrdup(gp);
+                gdsp->next = gdspn;
             }
             gdspn = gdsp;
             gp = p;		/* after : starts new group name */
@@ -410,19 +419,22 @@ create_gd(struct main_args *margs)
     }
     if (p == gp) {		/* empty group name not allowed */
         debug((char *) "%s| %s: ERROR: No group defined for domain %s\n", LogTime(), PROGRAM, p);
+        cleanup();
         return (1);
     }
     if (dp) {			/* end of domain name */
         gdsp->domain = xstrdup(dp);
     } else {			/* end of group name and no domain name */
         gdsp = init_gd();
-        gdsp->group = gp;
+        gdsp->group = xstrdup(gp);
         if (gdspn)		/* Have already an existing structure */
             gdsp->next = gdspn;
     }
     debug((char *) "%s| %s: INFO: Group %s  Domain %s\n", LogTime(), PROGRAM, gdsp->group, gdsp->domain ? gdsp->domain : "NULL");
 
     margs->groups = gdsp;
+    gdsp = NULL; // prevent the cleanup() deallocating it.
+    cleanup();
     return (0);
 }
 #endif
diff --git a/helpers/external_acl/kerberos_ldap_group/support_ldap.cc b/helpers/external_acl/kerberos_ldap_group/support_ldap.cc
index a6b058bf9b..08ac11ba7e 100644
--- a/helpers/external_acl/kerberos_ldap_group/support_ldap.cc
+++ b/helpers/external_acl/kerberos_ldap_group/support_ldap.cc
@@ -369,19 +369,18 @@ search_group_tree(struct main_args *margs, LDAP * ld, char *bindp, char *ldap_gr
     search_exp = (char *) xmalloc(strlen(filter) + strlen(ldap_filter_esc) + 1);
     snprintf(search_exp, strlen(filter) + strlen(ldap_filter_esc) + 1, filter, ldap_filter_esc);
 
-    if (ldap_filter_esc)
-        xfree(ldap_filter_esc);
+    xfree(ldap_filter_esc);
 
     if (depth > margs->mdepth) {
         debug((char *) "%s| %s: DEBUG: Max search depth reached %d>%d\n", LogTime(), PROGRAM, depth, margs->mdepth);
+        xfree(search_exp);
         return 0;
     }
     debug((char *) "%s| %s: DEBUG: Search ldap server with bind path %s and filter : %s\n", LogTime(), PROGRAM, bindp, search_exp);
     rc = ldap_search_ext_s(ld, bindp, LDAP_SCOPE_SUBTREE,
                            search_exp, NULL, 0,
                            NULL, NULL, &searchtime, 0, &res);
-    if (search_exp)
-        xfree(search_exp);
+    xfree(search_exp);
 
     if (rc != LDAP_SUCCESS) {
         error((char *) "%s| %s: ERROR: Error searching ldap server: %s\n", LogTime(), PROGRAM, ldap_err2string(rc));
@@ -744,22 +743,16 @@ tool_ldap_open(struct main_args * margs, char *host, int port, char *ssl)
             rc = ldap_url_parse(ldapuri, &url);
             if (rc != LDAP_SUCCESS) {
                 error((char *) "%s| %s: ERROR: Error while parsing url: %s\n", LogTime(), PROGRAM, ldap_err2string(rc));
-                if (ldapuri)
-                    xfree(ldapuri);
-                if (url)
-                    xfree(url);
+                xfree(ldapuri);
+                xfree(url);
                 return NULL;
             }
 #else
 #error "No URL parsing function"
 #endif
-            if (url) {
-                xfree(url);
-                url = NULL;
-            }
+            safe_free(url);
             rc = ldap_initialize(&ld, ldapuri);
-            if (ldapuri)
-                xfree(ldapuri);
+            xfree(ldapuri);
             if (rc != LDAP_SUCCESS) {
                 error((char *) "%s| %s: ERROR: Error while initialising connection to ldap server: %s\n", LogTime(), PROGRAM, ldap_err2string(rc));
                 ldap_unbind(ld);
@@ -897,7 +890,7 @@ get_memberof(struct main_args *margs, char *user, char *domain, char *group)
                 continue;
             }
             lcreds = (ldap_creds *) xmalloc(sizeof(struct ldap_creds));
-            lcreds->dn = bindp ? xstrdup(bindp) : NULL;
+            lcreds->dn = NULL;
             lcreds->pw = margs->ssl ? xstrdup(margs->ssl) : NULL;
             ldap_set_rebind_proc(ld, ldap_sasl_rebind, (char *) lcreds);
             if (ld != NULL) {
@@ -938,9 +931,7 @@ get_memberof(struct main_args *margs, char *user, char *domain, char *group)
             port = atoi(p);
         }
         nhosts = get_hostname_list(margs, &hlist, 0, host);
-        if (host)
-            xfree(host);
-        host = NULL;
+        safe_free(host);
         for (i = 0; i < nhosts; ++i) {
 
             ld = tool_ldap_open(margs, hlist[i].host, port, ssl);
@@ -967,8 +958,7 @@ get_memberof(struct main_args *margs, char *user, char *domain, char *group)
 
         }
         nhosts = free_hostname_list(&hlist, nhosts);
-        if (bindp)
-            xfree(bindp);
+        xfree(bindp);
         if (margs->lbind) {
             bindp = xstrdup(margs->lbind);
         } else {
@@ -1005,15 +995,13 @@ get_memberof(struct main_args *margs, char *user, char *domain, char *group)
     search_exp = (char *) xmalloc(strlen(filter) + strlen(ldap_filter_esc) + 1);
     snprintf(search_exp, strlen(filter) + strlen(ldap_filter_esc) + 1, filter, ldap_filter_esc);
 
-    if (ldap_filter_esc)
-        xfree(ldap_filter_esc);
+    xfree(ldap_filter_esc);
 
     debug((char *) "%s| %s: DEBUG: Search ldap server with bind path %s and filter : %s\n", LogTime(), PROGRAM, bindp, search_exp);
     rc = ldap_search_ext_s(ld, bindp, LDAP_SCOPE_SUBTREE,
                            search_exp, NULL, 0,
                            NULL, NULL, &searchtime, 0, &res);
-    if (search_exp)
-        xfree(search_exp);
+    xfree(search_exp);
 
     if (rc != LDAP_SUCCESS) {
         error((char *) "%s| %s: ERROR: Error searching ldap server: %s\n", LogTime(), PROGRAM, ldap_err2string(rc));
@@ -1121,15 +1109,13 @@ get_memberof(struct main_args *margs, char *user, char *domain, char *group)
         search_exp = (char *) xmalloc(strlen(filter) + strlen(ldap_filter_esc) + 1);
         snprintf(search_exp, strlen(filter) + strlen(ldap_filter_esc) + 1, filter, ldap_filter_esc);
 
-        if (ldap_filter_esc)
-            xfree(ldap_filter_esc);
+        xfree(ldap_filter_esc);
 
         debug((char *) "%s| %s: DEBUG: Search ldap server with bind path %s and filter: %s\n", LogTime(), PROGRAM, bindp, search_exp);
         rc = ldap_search_ext_s(ld, bindp, LDAP_SCOPE_SUBTREE,
                                search_exp, NULL, 0,
                                NULL, NULL, &searchtime, 0, &res);
-        if (search_exp)
-            xfree(search_exp);
+        xfree(search_exp);
 
         debug((char *) "%s| %s: DEBUG: Found %d ldap entr%s\n", LogTime(), PROGRAM, ldap_count_entries(ld, res), ldap_count_entries(ld, res) > 1 || ldap_count_entries(ld, res) == 0 ? "ies" : "y");
 
@@ -1147,15 +1133,13 @@ get_memberof(struct main_args *margs, char *user, char *domain, char *group)
             search_exp = (char *) xmalloc(strlen(filter) + strlen(ldap_filter_esc) + 1);
             snprintf(search_exp, strlen(filter) + strlen(ldap_filter_esc) + 1, filter, ldap_filter_esc);
 
-            if (ldap_filter_esc)
-                xfree(ldap_filter_esc);
+            xfree(ldap_filter_esc);
 
             debug((char *) "%s| %s: DEBUG: Search ldap server with bind path %s and filter: %s\n", LogTime(), PROGRAM, bindp, search_exp);
             rc = ldap_search_ext_s(ld, bindp, LDAP_SCOPE_SUBTREE,
                                    search_exp, NULL, 0,
                                    NULL, NULL, &searchtime, 0, &res);
-            if (search_exp)
-                xfree(search_exp);
+            xfree(search_exp);
 
             max_attr_2 = get_attributes(margs, ld, res, ATTRIBUTE, &attr_value_2);
             /*
@@ -1212,16 +1196,11 @@ cleanup:
         krb5_cleanup();
 #endif
     if (lcreds) {
-        if (lcreds->dn)
-            xfree(lcreds->dn);
-        if (lcreds->pw)
-            xfree(lcreds->pw);
+        xfree(lcreds->dn);
+        xfree(lcreds->pw);
         xfree(lcreds);
     }
-    if (bindp)
-        xfree(bindp);
-    bindp = NULL;
+    xfree(bindp);
     return (retval);
-
 }
 #endif
diff --git a/helpers/external_acl/kerberos_ldap_group/support_lserver.cc b/helpers/external_acl/kerberos_ldap_group/support_lserver.cc
index 16bb243d2e..f232147889 100644
--- a/helpers/external_acl/kerberos_ldap_group/support_lserver.cc
+++ b/helpers/external_acl/kerberos_ldap_group/support_lserver.cc
@@ -40,6 +40,18 @@ init_ls(void) {
     return lssp;
 }
 
+void
+free_ls(struct lsstruct *lssp)
+{
+    while(lssp) {
+        struct lsstruct *lsspn = lssp->next;
+        xfree(lssp->lserver);
+        xfree(lssp->domain);
+        xfree(lssp);
+        lssp = lsspn;
+    }
+}
+
 int
 create_ls(struct main_args *margs)
 {
@@ -73,18 +85,19 @@ create_ls(struct main_args *margs)
         if (*p == '@') {	/* end of group name - start of domain name */
             if (p == np) {	/* empty group name not allowed */
                 debug((char *) "%s| %s: DEBUG: No ldap servers defined for domain %s\n", LogTime(), PROGRAM, p);
+                free_ls(lssp);
                 return (1);
             }
             *p = '\0';
             ++p;
             lssp = init_ls();
             lssp->lserver = xstrdup(np);
-            if (lsspn)		/* Have already an existing structure */
-                lssp->next = lsspn;
+            lssp->next = lsspn;
             dp = p;		/* after @ starts new domain name */
         } else if (*p == ':') {	/* end of group name or end of domain name */
             if (p == np) {	/* empty group name not allowed */
                 debug((char *) "%s| %s: DEBUG: No ldap servers defined for domain %s\n", LogTime(), PROGRAM, p);
+                free_ls(lssp);
                 return (1);
             }
             *p = '\0';
@@ -95,8 +108,7 @@ create_ls(struct main_args *margs)
             } else {		/* end of group name and no domain name */
                 lssp = init_ls();
                 lssp->lserver = xstrdup(np);
-                if (lsspn)	/* Have already an existing structure */
-                    lssp->next = lsspn;
+                lssp->next = lsspn;
             }
             lsspn = lssp;
             np = p;		/* after : starts new group name */
@@ -106,6 +118,7 @@ create_ls(struct main_args *margs)
     }
     if (p == np) {		/* empty group name not allowed */
         debug((char *) "%s| %s: DEBUG: No ldap servers defined for domain %s\n", LogTime(), PROGRAM, p);
+        free_ls(lssp);
         return (1);
     }
     if (dp) {			/* end of domain name */
diff --git a/helpers/external_acl/kerberos_ldap_group/support_netbios.cc b/helpers/external_acl/kerberos_ldap_group/support_netbios.cc
index a721385578..4016dfa768 100644
--- a/helpers/external_acl/kerberos_ldap_group/support_netbios.cc
+++ b/helpers/external_acl/kerberos_ldap_group/support_netbios.cc
@@ -41,6 +41,18 @@ init_nd(void) {
     return ndsp;
 }
 
+void
+free_nd(struct ndstruct *ndsp)
+{
+    while (ndsp) {
+        struct ndstruct *ndspn = ndsp->next;
+        xfree(ndsp->netbios);
+        xfree(ndsp->domain);
+        xfree(ndsp);
+        ndsp = ndspn;
+    }
+}
+
 int
 create_nd(struct main_args *margs)
 {
@@ -74,18 +86,19 @@ create_nd(struct main_args *margs)
         if (*p == '@') {	/* end of group name - start of domain name */
             if (p == np) {	/* empty group name not allowed */
                 debug((char *) "%s| %s: DEBUG: No netbios name defined for domain %s\n", LogTime(), PROGRAM, p);
+                free_nd(ndsp);
                 return (1);
             }
             *p = '\0';
             ++p;
             ndsp = init_nd();
             ndsp->netbios = xstrdup(np);
-            if (ndspn)		/* Have already an existing structure */
-                ndsp->next = ndspn;
+            ndsp->next = ndspn;
             dp = p;		/* after @ starts new domain name */
         } else if (*p == ':') {	/* end of group name or end of domain name */
             if (p == np) {	/* empty group name not allowed */
                 debug((char *) "%s| %s: DEBUG: No netbios name defined for domain %s\n", LogTime(), PROGRAM, p);
+                free_nd(ndsp);
                 return (1);
             }
             *p = '\0';
@@ -96,13 +109,13 @@ create_nd(struct main_args *margs)
             } else {		/* end of group name and no domain name */
                 ndsp = init_nd();
                 ndsp->netbios = xstrdup(np);
-                if (ndspn)	/* Have already an existing structure */
-                    ndsp->next = ndspn;
+                ndsp->next = ndspn;
             }
             ndspn = ndsp;
             np = p;		/* after : starts new group name */
             if (!ndsp->domain || !strcmp(ndsp->domain, "")) {
                 debug((char *) "%s| %s: DEBUG: No domain defined for netbios name %s\n", LogTime(), PROGRAM, ndsp->netbios);
+                free_nd(ndsp);
                 return (1);
             }
             debug((char *) "%s| %s: DEBUG: Netbios name %s  Domain %s\n", LogTime(), PROGRAM, ndsp->netbios, ndsp->domain);
@@ -111,6 +124,7 @@ create_nd(struct main_args *margs)
     }
     if (p == np) {		/* empty group name not allowed */
         debug((char *) "%s| %s: DEBUG: No netbios name defined for domain %s\n", LogTime(), PROGRAM, p);
+        free_nd(ndsp);
         return (1);
     }
     if (dp) {			/* end of domain name */
@@ -118,11 +132,11 @@ create_nd(struct main_args *margs)
     } else {			/* end of group name and no domain name */
         ndsp = init_nd();
         ndsp->netbios = xstrdup(np);
-        if (ndspn)		/* Have already an existing structure */
-            ndsp->next = ndspn;
+        ndsp->next = ndspn;
     }
     if (!ndsp->domain || !strcmp(ndsp->domain, "")) {
         debug((char *) "%s| %s: DEBUG: No domain defined for netbios name %s\n", LogTime(), PROGRAM, ndsp->netbios);
+        free_nd(ndsp);
         return (1);
     }
     debug((char *) "%s| %s: DEBUG: Netbios name %s  Domain %s\n", LogTime(), PROGRAM, ndsp->netbios, ndsp->domain);