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

diff --git a/tools/cachemgr.cc b/tools/cachemgr.cc
index 1af18d15c162e54f431886f4f423ad43f12b74f1..7f08f4fcef136c857a3054f9cb57a616739e4e33 100644 (file)
--- a/tools/cachemgr.cc
+++ b/tools/cachemgr.cc
@@ -586,12 +586,15 @@ munge_action_line(const char *_buf, cachemgr_request * req)
     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;
 }
 
@@ -820,7 +823,7 @@ process_request(cachemgr_request * req)
     }
 
     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;
     }
@@ -832,7 +835,7 @@ process_request(cachemgr_request * req)
     } 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;
     }
@@ -846,17 +849,19 @@ process_request(cachemgr_request * req)
 #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;
     }
 
@@ -917,8 +922,6 @@ static char *
 read_post_request(void)
 {
     char *s;
-    char *buf;
-    int len;
 
     if ((s = getenv("REQUEST_METHOD")) == NULL)
         return NULL;
@@ -929,15 +932,34 @@ read_post_request(void)
     if ((s = getenv("CONTENT_LENGTH")) == NULL)
         return NULL;
 
-    if ((len = atoi(s)) <= 0)
+    if (*s == '-') // negative length content huh?
         return NULL;
 
-    buf = (char *)xmalloc(len + 1);
+    uint64_t len;
 
-    if (fread(buf, len, 1, stdin) == 0)
+    char *endptr = s+ strlen(s);
+    if ((len = strtoll(s, &endptr, 10)) <= 0)
         return NULL;
 
-    buf[len] = '\0';
+    // 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;
 }
@@ -1077,37 +1099,49 @@ decode_pub_auth(cachemgr_request * req)
     debug(3) fprintf(stderr, "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(3) fprintf(stderr, "cmgr: decoded host: '%s'\n", host_name);
 
-    if ((time_str = strtok(NULL, "|")) == NULL)
+    if ((time_str = strtok(NULL, "|")) == NULL) {
+        xfree(buf);
         return;
+    }
 
     debug(3) fprintf(stderr, "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(3) fprintf(stderr, "cmgr: decoded uname: '%s'\n", user_name);
 
-    if ((passwd = strtok(NULL, "|")) == NULL)
+    if ((passwd = strtok(NULL, "|")) == NULL) {
+        xfree(buf);
         return;
+    }
 
     debug(2) fprintf(stderr, "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(1) fprintf(stderr, "cmgr: verified auth. info.\n");
 
     /* ok, accept */
-    xfree(req->user_name);
+    safe_free(req->user_name);
 
     req->user_name = xstrdup(user_name);
 
@@ -1145,6 +1179,7 @@ make_auth_header(const cachemgr_request * req)
 
     snprintf(&buf[stringLength], sizeof(buf) - stringLength, "Proxy-Authorization: Basic %s\r\n", str64);
 
+    xfree(str64);
     return buf;
 }