Download raw body.
httpd: don't send the fastcgi param struct over imsg and tweaks
httpd: don't send the fastcgi param struct over imsg and tweaks
On Thu, 16 Jul 2026 11:30:09 +0200,
Rafael Sadowski <rafael@sizeofvoid.org> wrote:
>
> 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);
Are you sure that here no possible null case which will lead to crash?
Probably some tricky things like some fastcgi { param ... } inside a
location may attempt to write to null.
> }
> | 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
>
--
wbr, Kirill
httpd: don't send the fastcgi param struct over imsg and tweaks
httpd: don't send the fastcgi param struct over imsg and tweaks