Re: isakmpd(8) use-after-free
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=149597472223216=2
> >
> > The trace indicates that argument given to pf_key_v2_stayalive() is no
> > longer valid:
> >
> > #0 conf_get_str (section=0xa8735b03f80 ' > 0xa8735b04000 out of bounds>, tag=0xa8459272809 "Phase") at
> > /usr/src/sbin/isakmpd/conf.c:94
> > #1 0x0a84590293b4 in pf_key_v2_remove_conf (section=0xa8735b03f80 '
> > ) at
> > /usr/src/sbin/isakmpd/pf_key_v2.c:1905
> > #2 0x0a845902956a 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.c22 Jan 2014 03:09:31 - 1.37
+++ connection.c3 Jul 2017 09:03:53 -
@@ -146,6 +146,7 @@ connection_checker(void *vconn)
{
struct timeval now;
struct connection *conn = vconn;
+ char *name;
gettimeofday(, 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, );
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 - 1.198
+++ pf_key_v2.c 3 Jul 2017 12:53:29 -
@@ -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. */
Re: isakmpd(8) use-after-free
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=149597472223216=2
>
> The trace indicates that argument given to pf_key_v2_stayalive() is no
> longer valid:
>
> #0 conf_get_str (section=0xa8735b03f80 ' 0xa8735b04000 out of bounds>, tag=0xa8459272809 "Phase") at
> /usr/src/sbin/isakmpd/conf.c:94
> #1 0x0a84590293b4 in pf_key_v2_remove_conf (section=0xa8735b03f80 '
> ) at
> /usr/src/sbin/isakmpd/pf_key_v2.c:1905
> #2 0x0a845902956a 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.c22 Jan 2014 03:09:31 - 1.37
+++ connection.c3 Jul 2017 09:03:53 -
@@ -146,6 +146,7 @@ connection_checker(void *vconn)
{
struct timeval now;
struct connection *conn = vconn;
+ char *name;
gettimeofday(, 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, );
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 - 1.198
+++ pf_key_v2.c 3 Jul 2017 09:14:34 -
@@ -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. */
isakmpd(8) use-after-free
Michał Koc reported a crash on misc@, turns out it's a use-after-free:
http://marc.info/?l=openbsd-misc=149597472223216=2
The trace indicates that argument given to pf_key_v2_stayalive() is no
longer valid:
#0 conf_get_str (section=0xa8735b03f80 ' , tag=0xa8459272809 "Phase") at
/usr/src/sbin/isakmpd/conf.c:94
#1 0x0a84590293b4 in pf_key_v2_remove_conf (section=0xa8735b03f80 '
) at
/usr/src/sbin/isakmpd/pf_key_v2.c:1905
#2 0x0a845902956a 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 - 1.198
+++ pf_key_v2.c 8 Jun 2017 11:00:47 -
@@ -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));
