From: Rafael Sadowski Subject: httpd: don't send the fastcgi param struct over imsg and tweaks To: tech@openbsd.org Date: Thu, 16 Jul 2026 11:30:09 +0200 Based on the "custom header" idea and found issues. Send one imsg per param with a small fixed header (fastcgi_param_imsg) followed by the name and value bytes, and read it back with imsg_get_ibuf() and ibuf_get_string(). The struct now uses dynamic buffer instead of fixed, and the value limit is raised to 8192. Parse order is now preserved end-to-end instead of being reversed twice. This follows the config order: ... fcgi_add_param: HTTP_USER_AGENT[15] => curl/8.21.0[11], total_len: 189 fcgi_add_param: FCGI_TEST_A[11] => A[1], total_len: 217 fcgi_add_param: FCGI_TEST_B[11] => B[1], total_len: 231 fcgi_add_param: FCGI_TEST_C[11] => C[1], total_len: 245 fcgi_add_param: FCGI_TEST_D[11] => D[1], total_len: 259 fcgi_add_param: FCGI_TEST_E[11] => E[1], total_len: 273 fcgi_add_param: FCGI_TEST_BB[12] => BB[2], total_len: 287 ... This diff not include the change in "httpd: do NOT send location fcgiparams ...": https://marc.info/?l=openbsd-tech&m=178397190636339&w=2 Feedback, OK? Rafael diff --git a/config.c b/config.c index 146ed2c..ec1ec26 100644 --- a/config.c +++ b/config.c @@ -348,67 +348,52 @@ config_settls(struct httpd *env, struct server *srv, enum tls_config_type type, int config_getserver_fcgiparams(struct httpd *env, struct imsg *imsg) { - struct server *srv; - struct server_config *srv_conf, *iconf; - struct fastcgi_param *fp; - uint32_t id; - size_t c, nc, len; - uint8_t *p = imsg->data; - - len = sizeof(nc) + sizeof(id); - if (IMSG_DATA_SIZE(imsg) < len) { - log_debug("%s: invalid message length", __func__); + struct server_config *srv_conf; + struct fastcgi_param *fp; + struct fastcgi_param_imsg fpmsg; + struct ibuf ibuf; + + if (imsg_get_ibuf(imsg, &ibuf) == -1 || + ibuf_get(&ibuf, &fpmsg, sizeof(fpmsg)) == -1) { + log_debug("%s: invalid message", __func__); return (-1); } - memcpy(&nc, p, sizeof(nc)); /* number of params */ - p += sizeof(nc); - - memcpy(&id, p, sizeof(id)); /* server conf id */ - srv_conf = serverconfig_byid(id); - p += sizeof(id); - - len += nc*sizeof(*fp); - if (IMSG_DATA_SIZE(imsg) < len) { - log_debug("%s: invalid message length", __func__); + if ((srv_conf = serverconfig_byid(fpmsg.id)) == NULL) { + log_debug("%s: invalid config id", __func__); return (-1); } - /* Find associated server config */ - TAILQ_FOREACH(srv, env->sc_servers, srv_entry) { - if (srv->srv_conf.id == id) { - srv_conf = &srv->srv_conf; - break; - } - TAILQ_FOREACH(iconf, &srv->srv_hosts, entry) { - if (iconf->id == id) { - srv_conf = iconf; - break; - } - } + if (fpmsg.namelen > HTTPD_FCGI_NAME_MAX - 1 || + fpmsg.vallen > HTTPD_FCGI_VAL_MAX - 1) { + log_debug("%s: fastcgi_param too long", __func__); + return (-1); } - /* Fetch FCGI parameters */ - for (c = 0; c < nc; c++) { - if ((fp = calloc(1, sizeof(*fp))) == NULL) - fatalx("fcgiparams out of memory"); - memcpy(fp, p, sizeof(*fp)); - TAILQ_INSERT_HEAD(&srv_conf->fcgiparams, fp, entry); + if ((fp = calloc(1, sizeof(*fp))) == NULL) + fatal("fastcgi_param out of memory"); - p += sizeof(*fp); + fp->name = ibuf_get_string(&ibuf, fpmsg.namelen); + fp->value = ibuf_get_string(&ibuf, fpmsg.vallen); + if (fp->name == NULL || fp->value == NULL) { + free(fp->name); + free(fp->value); + free(fp); + return (-1); } + TAILQ_INSERT_TAIL(&srv_conf->fcgiparams, fp, entry); return (0); } int config_setserver_fcgiparams(struct httpd *env, struct server *srv) { - struct privsep *ps = env->sc_ps; - struct server_config *srv_conf = &srv->srv_conf; - struct fastcgi_param *fp; - struct iovec *iov; - size_t c = 0, nc = 0; + struct privsep *ps = env->sc_ps; + struct server_config *srv_conf = &srv->srv_conf; + struct fastcgi_param *fp; + struct fastcgi_param_imsg fpmsg; + struct iovec iov[3]; DPRINTF("%s: sending fcgiparam for \"%s[%u]\" to %s fd %d", __func__, srv_conf->name, srv_conf->id, ps->ps_title[PROC_SERVER], @@ -418,28 +403,24 @@ config_setserver_fcgiparams(struct httpd *env, struct server *srv) return (0); TAILQ_FOREACH(fp, &srv_conf->fcgiparams, entry) { - nc++; - } - if ((iov = calloc(nc + 2, sizeof(*iov))) == NULL) - return (-1); - - iov[c].iov_base = &nc; /* number of params */ - iov[c++].iov_len = sizeof(nc); - iov[c].iov_base = &srv_conf->id; /* server config id */ - iov[c++].iov_len = sizeof(srv_conf->id); - - TAILQ_FOREACH(fp, &srv_conf->fcgiparams, entry) { /* push FCGI params */ - iov[c].iov_base = fp; - iov[c++].iov_len = sizeof(*fp); - } - if (proc_composev(ps, PROC_SERVER, IMSG_CFG_FCGI, iov, c) != 0) { - log_warn("%s: failed to compose IMSG_CFG_FCGI imsg for " - "`%s'", __func__, srv_conf->name); - free(iov); - return (-1); + fpmsg.id = srv_conf->id; + fpmsg.namelen = strlen(fp->name); + fpmsg.vallen = strlen(fp->value); + + iov[0].iov_base = &fpmsg; + iov[0].iov_len = sizeof(fpmsg); + iov[1].iov_base = fp->name; + iov[1].iov_len = fpmsg.namelen; + iov[2].iov_base = fp->value; + iov[2].iov_len = fpmsg.vallen; + + if (proc_composev(ps, PROC_SERVER, IMSG_CFG_FCGI, iov, 3) + != 0) { + log_warn("%s: failed to compose IMSG_CFG_FCGI " + "for `%s'", __func__, srv_conf->name); + return (-1); + } } - free(iov); - return (0); } @@ -727,6 +708,8 @@ config_getserver(struct httpd *env, struct imsg *imsg) memcpy(&srv->srv_conf, &srv_conf, sizeof(srv->srv_conf)); srv->srv_s = fd; + TAILQ_INIT(&srv->srv_conf.fcgiparams); + if (config_getserver_auth(env, &srv->srv_conf) != 0) goto fail; diff --git a/httpd.h b/httpd.h index 740721e..3cce5d1 100644 --- a/httpd.h +++ b/httpd.h @@ -65,8 +65,8 @@ #define HTTPD_TLS_CIPHERS "secure" #define HTTPD_TLS_DHE_PARAMS "none" #define HTTPD_TLS_ECDHE_CURVES "default" -#define HTTPD_FCGI_NAME_MAX 511 -#define HTTPD_FCGI_VAL_MAX 511 +#define HTTPD_FCGI_NAME_MAX 512 +#define HTTPD_FCGI_VAL_MAX 8192 #define FD_RESERVE 5 #define SERVER_MAX_CLIENTS 1024 @@ -440,9 +440,15 @@ struct server_tls_ticket { unsigned char tt_key[TLS_TICKET_KEY_SIZE]; }; +struct fastcgi_param_imsg { + uint32_t id; /* server conf id */ + uint16_t namelen; + uint16_t vallen; +}; + struct fastcgi_param { - char name[HTTPD_FCGI_NAME_MAX]; - char value[HTTPD_FCGI_VAL_MAX]; + char *name; + char *value; TAILQ_ENTRY(fastcgi_param) entry; }; diff --git a/parse.y b/parse.y index 65664e1..257ecb7 100644 --- a/parse.y +++ b/parse.y @@ -813,29 +813,33 @@ fcgiflags : SOCKET STRING { if ((param = calloc(1, sizeof(*param))) == NULL) fatal("out of memory"); - if (strlcpy(param->name, $2, sizeof(param->name)) >= - sizeof(param->name)) { - yyerror("fastcgi_param name truncated"); + if (strlen($2) > HTTPD_FCGI_NAME_MAX - 1) { + yyerror("fastcgi param name too long (max %d)", + HTTPD_FCGI_NAME_MAX - 1); free($2); free($3); - free(param); YYERROR; } - if (strlcpy(param->value, $3, sizeof(param->value)) >= - sizeof(param->value)) { - yyerror("fastcgi_param value truncated"); + + if (strlen($3) > HTTPD_FCGI_VAL_MAX - 1) { + yyerror("fastcgi param value too long (max %d)", + HTTPD_FCGI_VAL_MAX - 1); free($2); free($3); - free(param); YYERROR; } + + if ((param->name = strdup($2)) == NULL || + (param->value = strdup($3)) == NULL) + fatal("out of memory"); + free($2); free($3); DPRINTF("[%s,%s,%d]: adding param \"%s\" value \"%s\"", srv_conf->location, srv_conf->name, srv_conf->id, param->name, param->value); - TAILQ_INSERT_HEAD(&srv_conf->fcgiparams, param, entry); + TAILQ_INSERT_TAIL(&srv_conf->fcgiparams, param, entry); } | STRIP NUMBER { if ($2 < 0 || $2 > INT_MAX) { diff --git a/server.c b/server.c index 0731730..a12d6f9 100644 --- a/server.c +++ b/server.c @@ -483,8 +483,11 @@ serverconfig_free(struct server_config *srv_conf) freezero(srv_conf->tls_cert, srv_conf->tls_cert_len); freezero(srv_conf->tls_key, srv_conf->tls_key_len); - TAILQ_FOREACH_SAFE(param, &srv_conf->fcgiparams, entry, tparam) + TAILQ_FOREACH_SAFE(param, &srv_conf->fcgiparams, entry, tparam) { + free(param->name); + free(param->value); free(param); + } } void