s3/smbd: Use after free when iterating smbd_server_connection->connections

Change conn_free() to just use a destructor. We now
catch any other places where we may have forgetten to
call conn_free() - it's implicit on talloc_free(conn).

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15128

Based on code from Noel Power <noel.power@suse.com>.

Signed-off-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Noel Power <npower@samba.org>
Autobuild-User(master): Noel Power <npower@samba.org>
Autobuild-Date(master): Wed Aug 17 09:54:06 UTC 2022 on sn-devel-184

diff --git a/source3/smbd/conn.c b/source3/smbd/conn.c
index 044242d569756b2fcf85319a9b792d6badd21ddb..776d7af4c127b86995c16fbfc03beecd5c3d8eff 100644 (file)
--- a/source3/smbd/conn.c
+++ b/source3/smbd/conn.c
@@ -24,6 +24,25 @@
 #include "smbd/globals.h"
 #include "lib/util/bitmap.h"
 
+static void conn_free_internal(connection_struct *conn);
+
+/****************************************************************************
+ * Remove a conn struct from conn->sconn->connections
+ * if not already done.
+****************************************************************************/
+
+static int conn_struct_destructor(connection_struct *conn)
+{
+        if (conn->sconn != NULL) {
+               DLIST_REMOVE(conn->sconn->connections, conn);
+               SMB_ASSERT(conn->sconn->num_connections > 0);
+               conn->sconn->num_connections--;
+               conn->sconn = NULL;
+       }
+       conn_free_internal(conn);
+       return 0;
+}
+
 /****************************************************************************
  Return the number of open connections.
 ****************************************************************************/
@@ -115,6 +134,11 @@ connection_struct *conn_new(struct smbd_server_connection *sconn)
        DLIST_ADD(sconn->connections, conn);
        sconn->num_connections++;
 
+       /*
+        * Catches the case where someone forgets to call
+        * conn_free().
+        */
+       talloc_set_destructor(conn, conn_struct_destructor);
        return conn;
 }
 
@@ -212,7 +236,6 @@ static void conn_free_internal(connection_struct *conn)
        free_namearray(conn->aio_write_behind_list);
 
        ZERO_STRUCTP(conn);
-       talloc_destroy(conn);
 }
 
 /****************************************************************************
@@ -221,16 +244,7 @@ static void conn_free_internal(connection_struct *conn)
 
 void conn_free(connection_struct *conn)
 {
-       if (conn->sconn == NULL) {
-               conn_free_internal(conn);
-               return;
-       }
-
-       DLIST_REMOVE(conn->sconn->connections, conn);
-       SMB_ASSERT(conn->sconn->num_connections > 0);
-       conn->sconn->num_connections--;
-
-       conn_free_internal(conn);
+       TALLOC_FREE(conn);
 }
 
 /*