Michał Koc reported a crash on misc@, turns out it's a use-after-free:
http://marc.info/?l=openbsd-misc&m=149597472223216&w=2
The trace indicates that argument given to pf_key_v2_stayalive() is no
longer valid:
#0 conf_get_str (section=0xa8735b03f80 ' <repeats 128 times> <Address
0xa8735b04000 out of bounds>, tag=0xa8459272809 "Phase") at
/usr/src/sbin/isakmpd/conf.c:94
#1 0x00000a84590293b4 in pf_key_v2_remove_conf (section=0xa8735b03f80 '
<repeats 128 times> <Address 0xa8735b04000 out of bounds>) at
/usr/src/sbin/isakmpd/pf_key_v2.c:1905
#2 0x00000a845902956a in pf_key_v2_stayalive (exchange=0xa86a6f44200,
vconn=0xa8735b03f80, fail=1) at /usr/src/sbin/isakmpd/pf_key_v2.c:2131
In r1.58 of pf_key_v2.c angelos@ move the argument given to
pf_key_v2_connection_check(), the one used after free, from
the stack to the heap:
Dynamically allocate conn, as this is given to the exchange; cleanup
conf space on failure to establish dynamic SA. ok niklas@
I don't understand the whole magic of function pointers in exchange.c
but what's interesting is that in his diff he stopped dereferencing
'exchange->name'.
But in pf_key_v2_connection_check() the 'conn' argument is passed as
'name' and as 'arg'... So the diff below fixes Michał's problem. I'd
appreciate if more people could test it and check if isakmpd(8) do not
leaking more memory than it already does.
Note that this diff do not fix the 'conn' leak introduced in the above
mentioned commit when a connection exist and exchange_establish() is
not called.
Comments, oks?
Index: pf_key_v2.c
===================================================================
RCS file: /cvs/src/sbin/isakmpd/pf_key_v2.c,v
retrieving revision 1.198
diff -u -p -r1.198 pf_key_v2.c
--- pf_key_v2.c 28 Feb 2017 16:46:27 -0000 1.198
+++ pf_key_v2.c 8 Jun 2017 11:00:47 -0000
@@ -2131,16 +2131,25 @@ pf_key_v2_stayalive(struct exchange *exc
pf_key_v2_remove_conf(conn);
pf_key_v2_remove_conf(conn);
}
+ free(conn);
}
/* Check if a connection CONN exists, otherwise establish it. */
void
pf_key_v2_connection_check(char *conn)
{
+ char *conn2 = NULL;
+
if (!sa_lookup_by_name(conn, 2)) {
+ conn2 = strdup(conn); /* will be freed in pf_key_v2_stayalive */
+ if (!conn2) {
+ LOG_DBG((LOG_SYSDEP, 70, "pf_key_v2_connection_check: "
+ "strdup(%s) failed", conn));
+ return;
+ }
LOG_DBG((LOG_SYSDEP, 70,
- "pf_key_v2_connection_check: SA for %s missing", conn));
- exchange_establish(conn, pf_key_v2_stayalive, conn, 0);
+ "pf_key_v2_connection_check: SA for %s missing", conn2));
+ exchange_establish(conn, pf_key_v2_stayalive, conn2, 0);
} else
LOG_DBG((LOG_SYSDEP, 70, "pf_key_v2_connection_check: "
"SA for %s exists", conn));
