From: Alexander Bluhm <alexander.bluhm@gmx.net>
Subject: syslogd use after free
To: tech@openbsd.org
Date: Sat, 6 Jan 2024 13:30:35 +0100

Hi,

My regress test has found a use after free of TLS context at syslogd
shutdown.  I think I have introduced the bug in October when I added
f_ev event field.  There I missed that we have to disable f_bufev
now.  Also I am not sure whether f_bufev is not NULL in all error
conditions.  Better add a check.

ok?

bluhm

Index: usr.sbin/syslogd/syslogd.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
diff -u -p -r1.279 syslogd.c
--- usr.sbin/syslogd/syslogd.c	19 Oct 2023 22:16:10 -0000	1.279
+++ usr.sbin/syslogd/syslogd.c	6 Jan 2024 12:26:58 -0000
@@ -1427,6 +1427,7 @@ tcp_errorcb(struct bufferevent *bufev, s
 		tls_free(f->f_un.f_forw.f_ctx);
 		f->f_un.f_forw.f_ctx = NULL;
 	}
+	bufferevent_disable(bufev, EV_READ|EV_WRITE);
 	close(f->f_file);
 	f->f_file = -1;
 
@@ -1521,6 +1522,7 @@ tcp_connectcb(int fd, short event, void 
 		tls_free(f->f_un.f_forw.f_ctx);
 		f->f_un.f_forw.f_ctx = NULL;
 	}
+	bufferevent_disable(bufev, EV_READ|EV_WRITE);
 	close(f->f_file);
 	f->f_file = -1;
 	loghost_retry(f);
@@ -2421,9 +2423,14 @@ init(void)
 			/* FALLTHROUGH */
 		case F_FORWTCP:
 			evtimer_del(&f->f_un.f_forw.f_ev);
-			tcpbuf_dropped += f->f_dropped +
-			     tcpbuf_countmsg(f->f_un.f_forw.f_bufev);
-			bufferevent_free(f->f_un.f_forw.f_bufev);
+			tcpbuf_dropped += f->f_dropped;
+			if (f->f_un.f_forw.f_bufev) {
+				bufferevent_disable(f->f_un.f_forw.f_bufev,
+				    EV_READ|EV_WRITE);
+				tcpbuf_dropped +=
+				     tcpbuf_countmsg(f->f_un.f_forw.f_bufev);
+				bufferevent_free(f->f_un.f_forw.f_bufev);
+			}
 			free(f->f_un.f_forw.f_ipproto);
 			free(f->f_un.f_forw.f_host);
 			free(f->f_un.f_forw.f_port);