On 08/06/17(Thu) 15:23, Martin Pieuchot wrote:
> 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.
It turns out that the problem comes from connection_checker(). This
function did not get the fix applied by angelos@ in r1.58 of pf_key_v2.c.
Since 'conn' is given to exchange_establish() it must be allocated
dynamically.
Diff below also prevents a use-after-free in connection_record_passive()
and plugs memory leaks in pf_key_v2_connection_check().
Comments, oks?
Index: connection.c
===================================================================
RCS file: /cvs/src/sbin/isakmpd/connection.c,v
retrieving revision 1.37
diff -u -p -r1.37 connection.c
--- connection.c 22 Jan 2014 03:09:31 -0000 1.37
+++ connection.c 3 Jul 2017 09:03:53 -0000
@@ -146,6 +146,7 @@ connection_checker(void *vconn)
{
struct timeval now;
struct connection *conn = vconn;
+ char *name;
gettimeofday(&now, 0);
now.tv_sec += conf_get_num("General", "check-interval",
@@ -153,9 +154,16 @@ connection_checker(void *vconn)
conn->ev = timer_add_event("connection_checker",
connection_checker, conn, &now);
if (!conn->ev)
- log_print("connection_checker: could not add timer event");
- if (!ui_daemon_passive)
- pf_key_v2_connection_check(conn->name);
+ log_print("%s: could not add timer event", __func__);
+ if (ui_daemon_passive)
+ return;
+
+ name = strdup(conn->name);
+ if (!name) {
+ log_print("%s: strdup (\"%s\") failed", __func__, conn->name);
+ return;
+ }
+ pf_key_v2_connection_check(name);
}
/* Find the connection named NAME. */
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 3 Jul 2017 09:14:34 -0000
@@ -2141,9 +2141,11 @@ pf_key_v2_connection_check(char *conn)
LOG_DBG((LOG_SYSDEP, 70,
"pf_key_v2_connection_check: SA for %s missing", conn));
exchange_establish(conn, pf_key_v2_stayalive, conn, 0);
- } else
+ } else {
LOG_DBG((LOG_SYSDEP, 70, "pf_key_v2_connection_check: "
"SA for %s exists", conn));
+ free(conn);
+ }
}
/* Handle a PF_KEY lifetime expiration message PMSG. */
@@ -3144,8 +3146,8 @@ pf_key_v2_acquire(struct pf_key_v2_msg *
conf_end(af, 1);
/* Let's rock 'n roll. */
- pf_key_v2_connection_check(conn);
connection_record_passive(conn);
+ pf_key_v2_connection_check(conn);
conn = 0;
/* Fall-through to cleanup. */
