From: Omar Polo Subject: Re: gotwebd: memleak in (and small refactoring of) fcgi_parse_record To: Stefan Sperling Cc: gameoftrees@openbsd.org, Tracey Emery Date: Thu, 01 Sep 2022 10:52:01 +0200 On 2022/09/01 09:42:23 +0200, Stefan Sperling 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 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; };