From: Rafael Sadowski <rafael@sizeofvoid.org>
Subject: httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro
To: tech@openbsd.org
Cc: claudio@openbsd.org
Date: Sat, 17 Oct 2026 08:50:45 +0200

Second attempt, since the first one still had a bug. I overlooked the
fact that in 'config_getserver', the 'imsg' data was passed through
again to 'config_getserver_config' and read there once more. With the
switch to the new API, the message is retrieved by 'imsg_get_data' and
cannot be retrieved again. This has now been fixed.

Our regression didn't find that. I have an idea.

commit 0fba93bae28cb57b9c1c598ff41835ca84617961
Author: Rafael Sadowski <rafael@sizeofvoid.org>
Date:   Sat Oct 17 08:39:35 2026 +0200

    httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro

diff --git a/config.c b/config.c
index ddf09f2..957da66 100644
--- a/config.c
+++ b/config.c
@@ -130,8 +130,8 @@ config_getreset(struct httpd *env, struct imsg *imsg)
 {
 	unsigned int	 mode;
 
-	IMSG_SIZE_CHECK(imsg, &mode);
-	memcpy(&mode, imsg->data, sizeof(mode));
+	if (imsg_get_data(imsg, &mode, sizeof(mode)) == -1)
+		fatalx("%s: imsg_get_data", __func__);
 
 	config_purge(env, mode);
 
@@ -744,7 +744,8 @@ config_getserver_config(struct httpd *env, struct server *srv,
 	if ((srv_conf = calloc(1, sizeof(*srv_conf))) == NULL)
 		return (-1);
 
-	IMSG_SIZE_CHECK(imsg, srv_conf);
+	if (imsg_get_len(imsg) < sizeof(*srv_conf))
+		fatalx("%s: bad length imsg received", __func__);
 	memcpy(srv_conf, p, sizeof(*srv_conf));
 	s = sizeof(*srv_conf);
 
@@ -927,7 +928,8 @@ config_getserver(struct httpd *env, struct imsg *imsg)
 	size_t			 s;
 	int			 fd;
 
-	IMSG_SIZE_CHECK(imsg, &srv_conf);
+	if (imsg_get_len(imsg) < sizeof(srv_conf))
+		fatalx("%s: bad length imsg received", __func__);
 	memcpy(&srv_conf, p, sizeof(srv_conf));
 	s = sizeof(srv_conf);
 
@@ -1052,22 +1054,27 @@ config_gettls(struct httpd *env, struct server_config *srv_conf,
 int
 config_getserver_tls(struct httpd *env, struct imsg *imsg)
 {
+	struct ibuf		 ibuf;
 	struct server_config	*srv_conf = NULL;
 	struct tls_config	 tls_conf;
-	uint8_t			*p = imsg->data;
+	uint8_t			*data;
 	size_t			 len;
 
-	IMSG_SIZE_CHECK(imsg, &tls_conf);
-	memcpy(&tls_conf, p, sizeof(tls_conf));
+	if (imsg_get_ibuf(imsg, &ibuf) == -1) {
+		log_warn("%s: imsg_get_ibuf", __func__);
+		return (-1);
+	}
 
-	len = tls_conf.tls_chunk_len;
+	if (ibuf_get(&ibuf, &tls_conf, sizeof(tls_conf)) == -1)
+		fatalx("%s: ibuf_get", __func__);
 
-	if ((IMSG_DATA_SIZE(imsg) - sizeof(tls_conf)) < len) {
+	if (ibuf_size(&ibuf) != tls_conf.tls_chunk_len) {
 		log_debug("%s: invalid message length", __func__);
 		goto fail;
 	}
 
-	p += sizeof(tls_conf);
+	data = ibuf_data(&ibuf);
+	len = tls_conf.tls_chunk_len;
 
 	if ((srv_conf = serverconfig_byid(tls_conf.id)) == NULL) {
 		log_debug("%s: server not found", __func__);
@@ -1076,32 +1083,32 @@ config_getserver_tls(struct httpd *env, struct imsg *imsg)
 
 	switch (tls_conf.tls_type) {
 	case TLS_CFG_CA:
-		if (config_gettls(env, srv_conf, &tls_conf, "ca", p, len,
+		if (config_gettls(env, srv_conf, &tls_conf, "ca", data, len,
 		    &srv_conf->tls_ca, &srv_conf->tls_ca_len) != 0)
 			goto fail;
 		break;
 
 	case TLS_CFG_CERT:
-		if (config_gettls(env, srv_conf, &tls_conf, "cert", p, len,
+		if (config_gettls(env, srv_conf, &tls_conf, "cert", data, len,
 		    &srv_conf->tls_cert, &srv_conf->tls_cert_len) != 0)
 			goto fail;
 		break;
 
 	case TLS_CFG_CRL:
-		if (config_gettls(env, srv_conf, &tls_conf, "crl", p, len,
+		if (config_gettls(env, srv_conf, &tls_conf, "crl", data, len,
 		    &srv_conf->tls_crl, &srv_conf->tls_crl_len) != 0)
 			goto fail;
 		break;
 
 	case TLS_CFG_KEY:
-		if (config_gettls(env, srv_conf, &tls_conf, "key", p, len,
+		if (config_gettls(env, srv_conf, &tls_conf, "key", data, len,
 		    &srv_conf->tls_key, &srv_conf->tls_key_len) != 0)
 			goto fail;
 		break;
 
 	case TLS_CFG_OCSP_STAPLE:
 		if (config_gettls(env, srv_conf, &tls_conf, "ocsp staple",
-		    p, len, &srv_conf->tls_ocsp_staple,
+		    data, len, &srv_conf->tls_ocsp_staple,
 		    &srv_conf->tls_ocsp_staple_len) != 0)
 			goto fail;
 		break;
@@ -1153,10 +1160,9 @@ config_getmedia(struct httpd *env, struct imsg *imsg)
 	struct privsep		*ps = env->sc_ps;
 #endif
 	struct media_type	 media;
-	uint8_t			*p = imsg->data;
 
-	IMSG_SIZE_CHECK(imsg, &media);
-	memcpy(&media, p, sizeof(media));
+	if (imsg_get_data(imsg, &media, sizeof(media)) == -1)
+		fatalx("%s: imsg_get_data", __func__);
 
 	if (media_add(env->sc_mediatypes, &media) == NULL) {
 		log_debug("%s: failed to add media \"%s\"",
@@ -1209,10 +1215,9 @@ config_getauth(struct httpd *env, struct imsg *imsg)
 	struct privsep		*ps = env->sc_ps;
 #endif
 	struct auth		 auth;
-	uint8_t			*p = imsg->data;
 
-	IMSG_SIZE_CHECK(imsg, &auth);
-	memcpy(&auth, p, sizeof(auth));
+	if (imsg_get_data(imsg, &auth, sizeof(auth)) == -1)
+		fatalx("%s: imsg_get_data", __func__);
 
 	if (auth_add(env->sc_auth, &auth) == NULL) {
 		log_debug("%s: failed to add auth \"%s[%u]\"",
diff --git a/httpd.c b/httpd.c
index abfbb5c..7ebdc84 100644
--- a/httpd.c
+++ b/httpd.c
@@ -430,8 +430,8 @@ parent_dispatch_logger(int fd, struct privsep_proc *p, struct imsg *imsg)
 
 	switch (imsg_get_type(imsg)) {
 	case IMSG_CTL_RESET:
-		IMSG_SIZE_CHECK(imsg, &v);
-		memcpy(&v, imsg->data, sizeof(v));
+		if (imsg_get_data(imsg, &v, sizeof(v)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		parent_reload(env, v, NULL);
 		break;
 	case IMSG_CTL_SHUTDOWN:
diff --git a/httpd.h b/httpd.h
index 1e54479..e238e04 100644
--- a/httpd.h
+++ b/httpd.h
@@ -164,10 +164,6 @@ struct imsgev {
 	short			 events;
 };
 
-#define IMSG_SIZE_CHECK(imsg, p) do {				\
-	if (IMSG_DATA_SIZE(imsg) < sizeof(*p))			\
-		fatalx("bad length imsg received");		\
-} while (0)
 #define IMSG_DATA_SIZE(imsg)	((imsg)->hdr.len - IMSG_HEADER_SIZE)
 #define MAX_IMSG_DATA_SIZE	(MAX_IMSGSIZE - IMSG_HEADER_SIZE)
 
diff --git a/logger.c b/logger.c
index 3b9a7d9..c75e7b7 100644
--- a/logger.c
+++ b/logger.c
@@ -140,8 +140,8 @@ logger_open_fd(struct imsg *imsg)
 	struct log_file		*log;
 	uint32_t		 id;
 
-	IMSG_SIZE_CHECK(imsg, &id);
-	memcpy(&id, imsg->data, sizeof(id));
+	if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
+		fatalx("%s: imsg_get_data", __func__);
 
 	TAILQ_FOREACH(log, &log_files, log_entry) {
 		if (log->log_id == id) {
@@ -158,19 +158,36 @@ logger_open_fd(struct imsg *imsg)
 int
 logger_open_priv(struct imsg *imsg)
 {
+	struct ibuf		 ibuf;
 	char			 path[PATH_MAX];
 	char			 name[PATH_MAX], *p;
+	char			*filename = NULL;
 	uint32_t		 id;
 	size_t			 len;
-	int			 fd;
+	int			 fd, ret;
 
-	/* called from the privileged process */
-	IMSG_SIZE_CHECK(imsg, &id);
-	memcpy(&id, imsg->data, sizeof(id));
-	p = (char *)imsg->data + sizeof(id);
+	if (imsg_get_ibuf(imsg, &ibuf) == -1) {
+		log_warn("%s: imsg_get_ibuf", __func__);
+		return (-1);
+	}
+
+	if (ibuf_get(&ibuf, &id, sizeof(id)) == -1)
+		fatalx("%s: ibuf_get", __func__);
+
+	if ((len = ibuf_size(&ibuf)) == 0) {
+		log_debug("%s: invalid message length", __func__);
+		return (-1);
+	}
 
-	if ((size_t)snprintf(name, sizeof(name), "/%s", p) >= sizeof(name))
+	if ((filename = ibuf_get_string(&ibuf, len)) == NULL) {
+		log_warn("%s: ibuf_get_string", __func__);
 		return (-1);
+	}
+	ret = snprintf(name, sizeof(name), "/%s", filename);
+	free(filename);
+	if (ret < 0 || (size_t)ret >= sizeof(name))
+		return (-1);
+
 	if ((len = strlcpy(path, httpd_env->sc_logdir, sizeof(path))) >=
 	    sizeof(path))
 		return (-1);
@@ -243,13 +260,19 @@ logger_start(void)
 int
 logger_log(struct imsg *imsg)
 {
+	struct ibuf		 ibuf;
 	char			*logline;
 	uint32_t		 id;
 	struct server_config	*srv_conf;
 	struct log_file		*log;
 
-	IMSG_SIZE_CHECK(imsg, &id);
-	memcpy(&id, imsg->data, sizeof(id));
+	if (imsg_get_ibuf(imsg, &ibuf) == -1) {
+		log_warn("%s: imsg_get_ibuf", __func__);
+		return (-1);
+	}
+
+	if (ibuf_get(&ibuf, &id, sizeof(id)) == -1)
+		fatalx("%s: ibuf_get", __func__);
 
 	if ((srv_conf = serverconfig_byid(id)) == NULL)
 		fatalx("invalid logging requestr");
@@ -265,7 +288,7 @@ logger_log(struct imsg *imsg)
 	}
 
 	/* XXX get_string() would sanitize the string, but add a malloc */
-	logline = (char *)imsg->data + sizeof(id);
+	logline = ibuf_data(&ibuf);
 
 	/* For debug output */
 	log_debug("%s", logline);
diff --git a/server.c b/server.c
index 9975a1c..1ce17f8 100644
--- a/server.c
+++ b/server.c
@@ -1421,8 +1421,8 @@ server_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
 			return (-1);
 		break;
 	case IMSG_TLSTICKET_REKEY:
-		IMSG_SIZE_CHECK(imsg, (&key));
-		memcpy(&key, imsg->data, sizeof(key));
+		if (imsg_get_data(imsg, &key, sizeof(key)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		/* apply to the right server */
 		if ((srv = server_byid(key.tt_id)) == NULL) {
 			log_debug("%s: invalid sever id", __func__);