On 2022/09/01 09:42:23 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Wed, Aug 31, 2022 at 05:58:08PM +0200, Omar Polo wrote:
> > 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.
> 
> Fix looks good, just one problem:
> 
> Is there a specific reason you are using strncpy() instead of strlcpy()?
> There are edge-case differences in the behaviour of NUL-terminating
> the result (see strncpy(3) EXAMPLES section).

The issue is that the fastcgi strings are not NUL terminated, we only
know how long they are.  I decided to use strncpy in this case because
there were the needed checks already in place and i was seduced by the
one-liner ifs body.

Using a memcpy + explicit NUL terminator (as the original code was
doing) is probably for the better.  (but change bcopy for memcpy.)

-----------------------------------------------
commit 4a9e511f0c82e35d1b9374ba030b29177a7bb74a (fcgi)
from: Omar Polo <op@omarpolo.com>
date: Thu Sep  1 08:48:48 2022 UTC
 
 gotwebd: plug leak in fcgi_parse_params
 
 fcgi_parse_params parses (some) 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.
 
diff b5c757f5f816a8061f4879da9e68a39141148e40 4a9e511f0c82e35d1b9374ba030b29177a7bb74a
commit - b5c757f5f816a8061f4879da9e68a39141148e40
commit + 4a9e511f0c82e35d1b9374ba030b29177a7bb74a
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 (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;
-		}
-
-		bcopy(buf, env_entry->val, name_len);
-		buf += name_len;
-		n -= 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 - 74ee67ae6734f6af0af3ab58f3276ceeccc8863a
blob + 8ed7b3a27f684b318e9ad129657be0edeca27f0c
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -223,9 +223,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;
 };