From: Omar Polo <op@omarpolo.com>
Subject: gotwebd: memleak in (and small refactoring of) fcgi_parse_record
To: gameoftrees@openbsd.org
Cc: Tracey Emery <tracey@traceyemery.net>
Date: Wed, 31 Aug 2022 17:58:08 +0200

gotwebd parses the fastcgi params into a list.  (I think this is a
leftover from slowcgi, where that list is then used to fill the
environment of the CGI process.)  However, the list is never free'd
and also never looked at outside of fcgi_parse_params, so I think we
can drop it.

I ended up looking at the fastcgi code for a different reason.  I've
seen a crash in a bcopy in fcgi_parse_params once, and after
rebuilding gotwebd with -O2 it never picked up SCRIPT_NAME
correctly...  Turns out we're reading from an un-initialized variable
`dr_buf' and got lucky since.

While here I've slightly tweaked the code, in particular I've turned a

	if (n > 0) {
		... code ...
	} else
		return

into a IMHO more clear

	if (n == 0)
		return;
	...code...

and I'm making the matching of the variables stronger by ensuring that
name_len is as long as the params we're interested in.

OK?

diff /home/op/w/got
commit - 794662a4547b17c0243addf37d330d39e0eb5662
path + /home/op/w/got
blob - 865c86621023df5c6c3d96bfad927b4eab09a613
file + gotwebd/fcgi.c
--- 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,65 @@ 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 (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 (n < name_len + val_len)
 			return;
 
-		if ((env_entry = malloc(sizeof(struct env_val))) == NULL) {
-			log_warn("cannot malloc env_entry");
-			return;
-		}
+		val = buf + name_len;
 
-		if ((env_entry->val = calloc(sizeof(char), name_len + val_len +
-		    2)) == NULL) {
-			log_warn("cannot allocate env_entry->val");
-			free(env_entry);
-			return;
-		}
+		if (c->querystring[0] == '\0' &&
+		    val_len < MAX_QUERYSTRING &&
+		    name_len == 12 &&
+		    strncmp(buf, "QUERY_STRING", 12) == 0)
+			strncpy(c->querystring, val, val_len);
 
-		bcopy(buf, env_entry->val, name_len);
-		buf += name_len;
-		n -= name_len;
+		if (c->http_host[0] == '\0' &&
+		    val_len < GOTWEBD_MAXTEXT &&
+		    name_len == 9 &&
+		    strncmp(buf, "HTTP_HOST", 9) == 0) {
+			strncpy(c->http_host, val, val_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);
-			c->querystring[val_len] = '\0';
-		}
-		if (val_len < GOTWEBD_MAXTEXT && strcmp(env_entry->val,
-		    "HTTP_HOST") == 0 && c->http_host[0] == '\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);
-			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);
-			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;
+		if (c->script_name[0] == '\0' &&
+		    val_len < MAX_SCRIPT_NAME &&
+		    name_len == 11 &&
+		    strncmp(buf, "SCRIPT_NAME", 11) == 0)
+			strncpy(c->script_name, val, val_len);
 
-		SLIST_INSERT_HEAD(&c->env, env_entry, entry);
-		log_debug("env[%d], %s", c->env_count, env_entry->val);
-		c->env_count++;
+		if (c->server_name[0] == '\0' &&
+		    val_len < MAX_SERVER_NAME &&
+		    name_len == 11 &&
+		    strncmp(buf, "SERVER_NAME", 11) == 0)
+			strncpy(c->server_name, val, val_len);
+
+		buf += name_len + val_len;
+		n -= name_len - val_len;
 	}
 }
 
blob - ae08f3df1ae9fb58a5fac19a6fcfd02a1aad9247
file + gotwebd/gotwebd.h
--- 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;
 };