Prevent deref-after-free when tdb_timeout() fires on invalid new tdb.

When receiving a pfkeyv2 SADB_ADD message, a newly created tdb can
fail in tdb_init(), which causes the tdb to not get added to the
global tdb list and an immediate dereference.  If a lifetime timeout
triggers on this tdb, it will unconditionally try to remove it from
the list and in the process deref once more than allowed,
causing a one bit corruption in the already freed up slot in the
tdb pool.

We resolve this issue by moving timeout_add() after tdb_init()
just before puttdb().  This means tdbs failing initialization
get discarded immediately as they only hold a single reference.
Valid tdbs get their timeouts activated just before we add them
to the tdb list, meaning the timeout can safely assume they are
linked.

Feedback from mvs@ and millert@
ok mvs@ mbuhl@

diff --git a/sys/net/pfkeyv2.c b/sys/net/pfkeyv2.c
index d4ca7c2..6916810 100644 (file)
--- a/sys/net/pfkeyv2.c
+++ b/sys/net/pfkeyv2.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: pfkeyv2.c,v 1.258 2023/09/29 18:40:08 tobhe Exp $ */
+/* $OpenBSD: pfkeyv2.c,v 1.259 2023/10/11 22:13:16 tobhe Exp $ */
 
 /*
  *     @(#)COPYRIGHT   1.1 (NRL) 17 January 1995
@@ -1391,6 +1391,9 @@ pfkeyv2_dosend(struct socket *so, void *message, int len)
 
                        /* Delete old version of the SA, insert new one */
                        tdb_delete(sa2);
+
+                       tdb_addtimeouts(newsa);
+
                        puttdb(newsa);
                } else {
                        /*
@@ -1423,6 +1426,8 @@ pfkeyv2_dosend(struct socket *so, void *message, int len)
 #endif
                        import_iface(sa2, headers[SADB_X_EXT_IFACE]);
 
+                       tdb_addtimeouts(sa2);
+
                        if (headers[SADB_EXT_ADDRESS_SRC] ||
                            headers[SADB_EXT_ADDRESS_PROXY]) {
                                mtx_enter(&tdb_sadb_mtx);
@@ -1565,6 +1570,8 @@ pfkeyv2_dosend(struct socket *so, void *message, int len)
                                goto ret;
                        }
 
+                       tdb_addtimeouts(newsa);
+
                        /* Add TDB in table */
                        puttdb(newsa);
                }

diff --git a/sys/net/pfkeyv2_convert.c b/sys/net/pfkeyv2_convert.c
index 83a1feb..2a7e1d9 100644 (file)
--- a/sys/net/pfkeyv2_convert.c
+++ b/sys/net/pfkeyv2_convert.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: pfkeyv2_convert.c,v 1.81 2023/09/16 09:33:27 mpi Exp $        */
+/*     $OpenBSD: pfkeyv2_convert.c,v 1.82 2023/10/11 22:13:16 tobhe Exp $      */
 /*
  * The author of this code is Angelos D. Keromytis (angelos@keromytis.org)
  *
@@ -302,9 +302,6 @@ import_lifetime(struct tdb *tdb, struct sadb_lifetime *sadb_lifetime, int type)
                if ((tdb->tdb_exp_timeout =
                    sadb_lifetime->sadb_lifetime_addtime) != 0) {
                        tdb->tdb_flags |= TDBF_TIMER;
-                       if (timeout_add_sec(&tdb->tdb_timer_tmo,
-                           tdb->tdb_exp_timeout))
-                               tdb_ref(tdb);
                } else
                        tdb->tdb_flags &= ~TDBF_TIMER;
 
@@ -331,9 +328,6 @@ import_lifetime(struct tdb *tdb, struct sadb_lifetime *sadb_lifetime, int type)
                if ((tdb->tdb_soft_timeout =
                    sadb_lifetime->sadb_lifetime_addtime) != 0) {
                        tdb->tdb_flags |= TDBF_SOFT_TIMER;
-                       if (timeout_add_sec(&tdb->tdb_stimer_tmo,
-                           tdb->tdb_soft_timeout))
-                               tdb_ref(tdb);
                } else
                        tdb->tdb_flags &= ~TDBF_SOFT_TIMER;
 

diff --git a/sys/netinet/ip_ipsp.c b/sys/netinet/ip_ipsp.c
index 90179f0..ee97b15 100644 (file)
--- a/sys/netinet/ip_ipsp.c
+++ b/sys/netinet/ip_ipsp.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: ip_ipsp.c,v 1.276 2023/08/07 03:43:57 dlg Exp $       */
+/*     $OpenBSD: ip_ipsp.c,v 1.277 2023/10/11 22:13:16 tobhe Exp $     */
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr),
@@ -716,6 +716,23 @@ tdb_firstuse(void *v)
        NET_UNLOCK();
 }
 
+void
+tdb_addtimeouts(struct tdb *tdbp)
+{
+       mtx_enter(&tdbp->tdb_mtx);
+       if (tdbp->tdb_flags & TDBF_TIMER) {
+               if (timeout_add_sec(&tdbp->tdb_timer_tmo,
+                   tdbp->tdb_exp_timeout))
+                       tdb_ref(tdbp);
+       }
+       if (tdbp->tdb_flags & TDBF_SOFT_TIMER) {
+               if (timeout_add_sec(&tdbp->tdb_stimer_tmo,
+                   tdbp->tdb_soft_timeout))
+                       tdb_ref(tdbp);
+       }
+       mtx_leave(&tdbp->tdb_mtx);
+}
+
 void
 tdb_soft_timeout(void *v)
 {

diff --git a/sys/netinet/ip_ipsp.h b/sys/netinet/ip_ipsp.h
index c24174d..d5100a5 100644 (file)
--- a/sys/netinet/ip_ipsp.h
+++ b/sys/netinet/ip_ipsp.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: ip_ipsp.h,v 1.242 2023/08/07 01:44:51 dlg Exp $       */
+/*     $OpenBSD: ip_ipsp.h,v 1.243 2023/10/11 22:13:16 tobhe Exp $     */
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr),
@@ -613,6 +613,7 @@ void        tdb_unlink(struct tdb *);
 void   tdb_unlink_locked(struct tdb *);
 void   tdb_cleanspd(struct tdb *);
 void   tdb_unbundle(struct tdb *);
+void   tdb_addtimeouts(struct tdb *);
 void   tdb_deltimeouts(struct tdb *);
 int    tdb_walk(u_int, int (*)(struct tdb *, void *, int), void *);
 void   tdb_printit(void *, int, int (*)(const char *, ...));