commit 40a95f4f726567f73ee83d4866608f7ea0654eaa
from: Omar Polo <op@omarpolo.com>
date: Thu Sep 01 09:36:58 2022 UTC

gotwebd: plug leak in fcgi_parse_params

fcgi_parse_params parses fastcgi parameters into a list.  (This is a
leftover from slowcgi where that list is later used to populate the
environment of the CGI process.)  However, this list is never looked at
and its memory never released, so just drop it.

Make the matching on fastcgi parameters name strictier by checking also
that the length is the one we expect; otherwise we might pick up
parameters with the same prefix string (i.e. FOO vs FOO_WITH_SUFFIX)

While here turn some bcopy into memcpy and simplify some if-nesting too.
Fix the reading from an un-initialized pointer that I introduced in a
previous commit.

ok stsp@

commit - af7ccbc139100f0bb94b8d2ef998dedc122a18f5
commit + 40a95f4f726567f73ee83d4866608f7ea0654eaa
blob - 865c86621023df5c6c3d96bfad927b4eab09a613
blob + e77b8424231bf88c153a32dd90a19292c41dc39d
--- gotwebd/fcgi.c
+++ gotwebd/fcgi.c
@@ -173,18 +173,14 @@ fcgi_parse_begin_request(uint8_t *buf, uint16_t n,
 	}
 
 	c->request_started = 1;
-
 	c->id = id;
-	SLIST_INIT(&c->env);
-	c->env_count = 0;
 }
 
 void
 fcgi_parse_params(uint8_t *buf, uint16_t n, struct request *c, uint16_t id)
 {
-	struct env_val *env_entry;
 	uint32_t name_len, val_len;
-	uint8_t *sd, *dr_buf;
+	uint8_t *sd, *val;
 
 	if (!c->request_started) {
 		log_warn("FCGI_PARAMS without FCGI_BEGIN_REQUEST, ignoring");
@@ -216,83 +212,72 @@ fcgi_parse_params(uint8_t *buf, uint16_t n, struct req
 				return;
 		}
 
-		if (n > 0) {
-			if (buf[0] >> 7 == 0) {
-				val_len = buf[0];
-				n--;
-				buf++;
-			} else {
-				if (n > 3) {
-					val_len = ((buf[0] & 0x7f) << 24) +
-					    (buf[1] << 16) + (buf[2] << 8) +
-					     buf[3];
-					n -= 4;
-					buf += 4;
-				} else
-					return;
-			}
-		} else
+		if (n == 0)
 			return;
 
-		if (n < name_len + val_len)
-			return;
-
-		if ((env_entry = malloc(sizeof(struct env_val))) == NULL) {
-			log_warn("cannot malloc env_entry");
-			return;
+		if (buf[0] >> 7 == 0) {
+			val_len = buf[0];
+			n--;
+			buf++;
+		} else {
+			if (n > 3) {
+				val_len = ((buf[0] & 0x7f) << 24) +
+					(buf[1] << 16) + (buf[2] << 8) +
+					buf[3];
+				n -= 4;
+				buf += 4;
+			} else
+				return;
 		}
 
-		if ((env_entry->val = calloc(sizeof(char), name_len + val_len +
-		    2)) == NULL) {
-			log_warn("cannot allocate env_entry->val");
-			free(env_entry);
+		if (n < name_len + val_len)
 			return;
-		}
 
-		bcopy(buf, env_entry->val, name_len);
-		buf += name_len;
-		n -= name_len;
+		val = buf + name_len;
 
-		env_entry->val[name_len] = '\0';
-		if (val_len < MAX_QUERYSTRING && strcmp(env_entry->val,
-		    "QUERY_STRING") == 0 && c->querystring[0] == '\0') {
-			bcopy(buf, c->querystring, val_len);
+		if (c->querystring[0] == '\0' &&
+		    val_len < MAX_QUERYSTRING &&
+		    name_len == 12 &&
+		    strncmp(buf, "QUERY_STRING", 12) == 0) {
+			memcpy(c->querystring, val, val_len);
 			c->querystring[val_len] = '\0';
 		}
-		if (val_len < GOTWEBD_MAXTEXT && strcmp(env_entry->val,
-		    "HTTP_HOST") == 0 && c->http_host[0] == '\0') {
 
+		if (c->http_host[0] == '\0' &&
+		    val_len < GOTWEBD_MAXTEXT &&
+		    name_len == 9 &&
+		    strncmp(buf, "HTTP_HOST", 9) == 0) {
+			memcpy(c->http_host, val, val_len);
+			c->http_host[val_len] = '\0';
+
 			/*
 			 * lazily get subdomain
 			 * will only get domain if no subdomain exists
 			 * this can still work if gotweb server name is the same
 			 */
-			sd = strchr(buf, '.');
+			sd = strchr(c->http_host, '.');
 			if (sd)
 				*sd = '\0';
-
-			bcopy(buf, c->http_host, val_len);
-			c->http_host[val_len] = '\0';
 		}
-		if (val_len < MAX_SCRIPT_NAME && strcmp(env_entry->val,
-		    "SCRIPT_NAME") == 0 && c->script_name[0] == '\0') {
-			bcopy(dr_buf, c->script_name, val_len);
+
+		if (c->script_name[0] == '\0' &&
+		    val_len < MAX_SCRIPT_NAME &&
+		    name_len == 11 &&
+		    strncmp(buf, "SCRIPT_NAME", 11) == 0) {
+			memcpy(c->script_name, val, val_len);
 			c->script_name[val_len] = '\0';
 		}
-		if (val_len < MAX_SERVER_NAME && strcmp(env_entry->val,
-		    "SERVER_NAME") == 0 && c->server_name[0] == '\0') {
-			bcopy(buf, c->server_name, val_len);
+
+		if (c->server_name[0] == '\0' &&
+		    val_len < MAX_SERVER_NAME &&
+		    name_len == 11 &&
+		    strncmp(buf, "SERVER_NAME", 11) == 0) {
+			memcpy(c->server_name, val, val_len);
 			c->server_name[val_len] = '\0';
 		}
-		env_entry->val[name_len] = '=';
 
-		bcopy(buf, (env_entry->val) + name_len + 1, val_len);
-		buf += val_len;
-		n -= val_len;
-
-		SLIST_INSERT_HEAD(&c->env, env_entry, entry);
-		log_debug("env[%d], %s", c->env_count, env_entry->val);
-		c->env_count++;
+		buf += name_len + val_len;
+		n -= name_len - val_len;
 	}
 }
 
blob - bf762e12dd70fa07bb83c88d023e1ebb29d35f27
blob + ac7f962b1abdcfe1c535e8bc210c9f619e7a2cbc
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -222,9 +222,6 @@ struct request {
 	char				 script_name[MAX_SCRIPT_NAME];
 	char				 server_name[MAX_SERVER_NAME];
 
-	struct env_head			 env;
-	int				 env_count;
-
 	uint8_t				 request_started;
 };