On 03/07/17(Mon) 11:18, Martin Pieuchot wrote:
> 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().
hshoexer@ found that my new diff generalized a memory leak present in
pf_key_v2_acquire().
Updated diff below fixes that by freeing the memory in pf_key_v2_stayalive().
Comments, ok?
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 12:53:29 -0000
@@ -2131,6 +2131,7 @@ 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. */
@@ -2141,9 +2142,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 +3147,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. */
