------------------------------------------------------------
revno: 12401
revision-id: squid3@treenet.co.nz-20121124021756-0za3b7r7t3tloko1
parent: squid3@treenet.co.nz-20121124021327-9eddvqdqx29rbr9h
committer: Amos Jeffries <squid3@treenet.co.nz>
branch nick: 3.3
timestamp: Fri 2012-11-23 19:17:56 -0700
message:
  cachemgr.cgi: Memory Leaks and DoS Vulnerability
  
  * Ignore invalid Content-Length headers.
  
  * Limit received POST requests to 4KB and discard the rest.
  
  * Authentication credentials parser also leaks badly.
  
   Detected by Coverity Scan.
   Issues 740380, 740443, 740444, 740442, 740487, 740446, 740445
------------------------------------------------------------
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: squid3@treenet.co.nz-20121124021756-0za3b7r7t3tloko1
# target_branch: http://bzr.squid-cache.org/bzr/squid3/3.3
# testament_sha1: 3f9ea52d5755e21547e70802fdcb119044e0fd6f
# timestamp: 2012-11-24 02:54:10 +0000
# source_branch: http://bzr.squid-cache.org/bzr/squid3/3.3
# base_revision_id: squid3@treenet.co.nz-20121124021327-\
#   9eddvqdqx29rbr9h
# 
# Begin patch
=== modified file 'tools/cachemgr.cc'
--- tools/cachemgr.cc	2012-08-28 13:00:30 +0000
+++ tools/cachemgr.cc	2012-11-24 02:17:56 +0000
@@ -594,12 +594,15 @@
     if ((p = strchr(x, '\n')))
         *p = '\0';
     action = xstrtok(&x, '\t');
+    if (!action) {
+        xfree(buf);
+        return "";
+    }
     description = xstrtok(&x, '\t');
     if (!description)
         description = action;
-    if (!action)
-        return "";
     snprintf(html, sizeof(html), " <a href=\"%s\">%s</a>", menu_url(req, action), description);
+    xfree(buf);
     return html;
 }
 
@@ -828,7 +831,7 @@
     }
 
     if (!check_target_acl(req->hostname, req->port)) {
-        snprintf(buf, 1024, "target %s:%d not allowed in cachemgr.conf\n", req->hostname, req->port);
+        snprintf(buf, sizeof(buf), "target %s:%d not allowed in cachemgr.conf\n", req->hostname, req->port);
         error_html(buf);
         return 1;
     }
@@ -840,7 +843,7 @@
     } else if ((S = req->hostname))
         (void) 0;
     else {
-        snprintf(buf, 1024, "Unknown host: %s\n", req->hostname);
+        snprintf(buf, sizeof(buf), "Unknown host: %s\n", req->hostname);
         error_html(buf);
         return 1;
     }
@@ -854,17 +857,19 @@
 #else
     if ((s = socket(PF_INET, SOCK_STREAM, 0)) < 0) {
 #endif
-        snprintf(buf, 1024, "socket: %s\n", xstrerror());
+        snprintf(buf, sizeof(buf), "socket: %s\n", xstrerror());
         error_html(buf);
+        S.FreeAddrInfo(AI);
         return 1;
     }
 
     if (connect(s, AI->ai_addr, AI->ai_addrlen) < 0) {
-        snprintf(buf, 1024, "connect %s: %s\n",
+        snprintf(buf, sizeof(buf), "connect %s: %s\n",
                  S.ToURL(ipbuf,MAX_IPSTRLEN),
                  xstrerror());
         error_html(buf);
         S.FreeAddrInfo(AI);
+        close(s);
         return 1;
     }
 
@@ -952,8 +957,6 @@
 read_post_request(void)
 {
     char *s;
-    char *buf;
-    int len;
 
     if ((s = getenv("REQUEST_METHOD")) == NULL)
         return NULL;
@@ -964,15 +967,34 @@
     if ((s = getenv("CONTENT_LENGTH")) == NULL)
         return NULL;
 
-    if ((len = atoi(s)) <= 0)
-        return NULL;
-
-    buf = (char *)xmalloc(len + 1);
-
-    if (fread(buf, len, 1, stdin) == 0)
-        return NULL;
-
-    buf[len] = '\0';
+    if (*s == '-') // negative length content huh?
+        return NULL;
+
+    uint64_t len;
+
+    char *endptr = s+ strlen(s);
+    if ((len = strtoll(s, &endptr, 10)) <= 0)
+        return NULL;
+
+    // limit the input to something reasonable.
+    // 4KB should be enough for the GET/POST data length, but may be extended.
+    size_t bufLen = (len >= 4096 ? len : 4095);
+    char *buf = (char *)xmalloc(bufLen + 1);
+
+    size_t readLen = fread(buf, bufLen, 1, stdin);
+    if (readLen == 0) {
+        xfree(buf);
+        return NULL;
+    }
+    buf[readLen] = '\0';
+    len -= readLen;
+
+    // purge the remainder of the request entity
+    while (len > 0) {
+        char temp[65535];
+        readLen = fread(temp, 65535, 1, stdin);
+        len -= readLen;
+    }
 
     return buf;
 }
@@ -1118,37 +1140,49 @@
     debug("cmgr: length ok\n");
 
     /* parse ( a lot of memory leaks, but that is cachemgr style :) */
-    if ((host_name = strtok(buf, "|")) == NULL)
+    if ((host_name = strtok(buf, "|")) == NULL) {
+        xfree(buf);
         return;
+    }
 
     debug("cmgr: decoded host: '%s'\n", host_name);
 
-    if ((time_str = strtok(NULL, "|")) == NULL)
+    if ((time_str = strtok(NULL, "|")) == NULL) {
+        xfree(buf);
         return;
+    }
 
     debug("cmgr: decoded time: '%s' (now: %d)\n", time_str, (int) now);
 
-    if ((user_name = strtok(NULL, "|")) == NULL)
+    if ((user_name = strtok(NULL, "|")) == NULL) {
+        xfree(buf);
         return;
+    }
 
     debug("cmgr: decoded uname: '%s'\n", user_name);
 
-    if ((passwd = strtok(NULL, "|")) == NULL)
+    if ((passwd = strtok(NULL, "|")) == NULL) {
+        xfree(buf);
         return;
+    }
 
     debug("cmgr: decoded passwd: '%s'\n", passwd);
 
     /* verify freshness and validity */
-    if (atoi(time_str) + passwd_ttl < now)
+    if (atoi(time_str) + passwd_ttl < now) {
+        xfree(buf);
         return;
+    }
 
-    if (strcasecmp(host_name, req->hostname))
+    if (strcasecmp(host_name, req->hostname)) {
+        xfree(buf);
         return;
+    }
 
     debug("cmgr: verified auth. info.\n");
 
     /* ok, accept */
-    xfree(req->user_name);
+    safe_free(req->user_name);
 
     req->user_name = xstrdup(user_name);
 
@@ -1190,6 +1224,7 @@
 
     snprintf(&buf[stringLength], sizeof(buf) - stringLength, "Proxy-Authorization: Basic %s\r\n", str64);
 
+    xfree(str64);
     return buf;
 }