From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: httpd: imsg_get_fd and fix a use-after-free
To: tech@openbsd.org
Date: Tue, 16 Jan 2024 17:45:32 +0100

This converts httpd to use imsg_get_fd().

While there fix a use-after-free on failure in config_getserver().
The problem is hit when get_data() fails for the variable-length values.
Shuffling the code a bit to insert the srv onto the lists later fixes the
problem.

Also do the same no-fd passing change in proc_forward_imsg() as done in
iked. Again nothing needs this so neuter this miss-feature.

-- 
:wq Claudio

Index: config.c
===================================================================
RCS file: /cvs/src/usr.sbin/httpd/config.c,v
diff -u -p -r1.63 config.c
--- config.c	28 Dec 2022 21:30:16 -0000	1.63
+++ config.c	12 Dec 2023 16:46:54 -0000
@@ -626,6 +626,7 @@ config_getserver(struct httpd *env, stru
 	struct server_config	 srv_conf;
 	uint8_t			*p = imsg->data;
 	size_t			 s;
+	int			 fd;
 
 	IMSG_SIZE_CHECK(imsg, &srv_conf);
 	memcpy(&srv_conf, p, sizeof(srv_conf));
@@ -634,6 +635,8 @@ config_getserver(struct httpd *env, stru
 	/* Reset these variables to avoid free'ing invalid pointers */
 	serverconfig_reset(&srv_conf);
 
+	fd = imsg_get_fd(imsg);
+
 	if ((IMSG_DATA_SIZE(imsg) - s) < (size_t)srv_conf.return_uri_len) {
 		log_debug("%s: invalid message length", __func__);
 		goto fail;
@@ -643,11 +646,11 @@ config_getserver(struct httpd *env, stru
 	if ((srv = server_byaddr((struct sockaddr *)
 	    &srv_conf.ss, srv_conf.port)) != NULL) {
 		/* Add "host" to existing listening server */
-		if (imsg->fd != -1) {
+		if (fd != -1) {
 			if (srv->srv_s == -1)
-				srv->srv_s = imsg->fd;
+				srv->srv_s = fd;
 			else
-				close(imsg->fd);
+				close(fd);
 		}
 		return (config_getserver_config(env, srv, imsg));
 	}
@@ -660,7 +663,7 @@ config_getserver(struct httpd *env, stru
 		goto fail;
 
 	memcpy(&srv->srv_conf, &srv_conf, sizeof(srv->srv_conf));
-	srv->srv_s = imsg->fd;
+	srv->srv_s = fd;
 
 	if (config_getserver_auth(env, &srv->srv_conf) != 0)
 		goto fail;
@@ -668,14 +671,6 @@ config_getserver(struct httpd *env, stru
 	SPLAY_INIT(&srv->srv_clients);
 	TAILQ_INIT(&srv->srv_hosts);
 
-	TAILQ_INSERT_TAIL(&srv->srv_hosts, &srv->srv_conf, entry);
-	TAILQ_INSERT_TAIL(env->sc_servers, srv, srv_entry);
-
-	DPRINTF("%s: %s %d configuration \"%s[%u]\", flags: %s", __func__,
-	    ps->ps_title[privsep_process], ps->ps_instance,
-	    srv->srv_conf.name, srv->srv_conf.id,
-	    printb_flags(srv->srv_conf.flags, SRVFLAG_BITS));
-
 	/*
 	 * Get all variable-length values for the parent server.
 	 */
@@ -685,11 +680,19 @@ config_getserver(struct httpd *env, stru
 			goto fail;
 	}
 
+	TAILQ_INSERT_TAIL(&srv->srv_hosts, &srv->srv_conf, entry);
+	TAILQ_INSERT_TAIL(env->sc_servers, srv, srv_entry);
+
+	DPRINTF("%s: %s %d configuration \"%s[%u]\", flags: %s", __func__,
+	    ps->ps_title[privsep_process], ps->ps_instance,
+	    srv->srv_conf.name, srv->srv_conf.id,
+	    printb_flags(srv->srv_conf.flags, SRVFLAG_BITS));
+
 	return (0);
 
  fail:
-	if (imsg->fd != -1)
-		close(imsg->fd);
+	if (fd != -1)
+		close(fd);
 	if (srv != NULL)
 		serverconfig_free(&srv->srv_conf);
 	free(srv);
Index: logger.c
===================================================================
RCS file: /cvs/src/usr.sbin/httpd/logger.c,v
diff -u -p -r1.24 logger.c
--- logger.c	27 Jan 2021 07:21:53 -0000	1.24
+++ logger.c	12 Dec 2023 16:41:02 -0000
@@ -144,9 +144,9 @@ logger_open_fd(struct imsg *imsg)
 
 	TAILQ_FOREACH(log, &log_files, log_entry) {
 		if (log->log_id == id) {
+			log->log_fd = imsg_get_fd(imsg);
 			DPRINTF("%s: received log fd %d, file %s",
-			    __func__, imsg->fd, log->log_name);
-			log->log_fd = imsg->fd;
+			    __func__, log->log_fd, log->log_name);
 			return (0);
 		}
 	}
Index: proc.c
===================================================================
RCS file: /cvs/src/usr.sbin/httpd/proc.c,v
diff -u -p -r1.42 proc.c
--- proc.c	15 Feb 2023 20:44:01 -0000	1.42
+++ proc.c	12 Dec 2023 16:40:05 -0000
@@ -668,7 +668,7 @@ proc_dispatch(int fd, short event, void 
 		case IMSG_CTL_PROCFD:
 			IMSG_SIZE_CHECK(&imsg, &pf);
 			memcpy(&pf, imsg.data, sizeof(pf));
-			proc_accept(ps, imsg.fd, pf.pf_procid,
+			proc_accept(ps, imsg_get_fd(&imsg), pf.pf_procid,
 			    pf.pf_instance);
 			break;
 		default:
@@ -799,7 +799,7 @@ proc_forward_imsg(struct privsep *ps, st
     enum privsep_procid id, int n)
 {
 	return (proc_compose_imsg(ps, id, n, imsg->hdr.type,
-	    imsg->hdr.peerid, imsg->fd, imsg->data, IMSG_DATA_SIZE(imsg)));
+	    imsg->hdr.peerid, -1, imsg->data, IMSG_DATA_SIZE(imsg)));
 }
 
 struct imsgbuf *