[SCM] Samba Shared Repository - branch master updated
Noel Power
npower at samba.org
Wed Aug 17 09:55:02 UTC 2022
The branch, master has been updated
via f92bacbe216 s3/smbd: Use after free when iterating smbd_server_connection->connections
via 0bdfb5a5e60 s3/smbd: Use after free when iterating smbd_server_connection->connections
from 123f1c07c41 s3:utils remove documentation of -l as alias for --long
https://git.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit f92bacbe216d2d74ea3ccf3fe0df5c1cc9860996
Author: Jeremy Allison <jra at samba.org>
Date: Fri Jul 22 16:28:03 2022 +0100
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 at suse.com>.
Signed-off-by: Jeremy Allison <jra at samba.org>
Reviewed-by: Noel Power <npower at samba.org>
Autobuild-User(master): Noel Power <npower at samba.org>
Autobuild-Date(master): Wed Aug 17 09:54:06 UTC 2022 on sn-devel-184
commit 0bdfb5a5e60df214c088df0782c4a1bcc2a4944a
Author: Jeremy Allison <jra at samba.org>
Date: Tue Aug 16 13:51:27 2022 -0700
s3/smbd: Use after free when iterating smbd_server_connection->connections
In SMB2 smbd_smb2_tree_connect() we create a new conn struct
inside make_connection_smb2() then move the ownership to tcon using:
tcon->compat = talloc_move(tcon, &compat_conn);
so the lifetime of tcon->compat is tied directly to tcon.
Inside smbXsrv_tcon_disconnect() we have:
908 ok = chdir_current_service(tcon->compat);
909 if (!ok) {
910 status = NT_STATUS_INTERNAL_ERROR;
911 DEBUG(0, ("smbXsrv_tcon_disconnect(0x%08x, '%s'): "
912 "chdir_current_service() failed: %s\n",
913 tcon->global->tcon_global_id,
914 tcon->global->share_name,
915 nt_errstr(status)));
916 tcon->compat = NULL;
917 return status;
918 }
919
920 close_cnum(tcon->compat, vuid);
921 tcon->compat = NULL;
If chdir_current_service(tcon->compat) fails, we return status without ever having
called close_cnum(tcon->compat, vuid), leaving the conn pointer left in the linked
list sconn->connections.
The caller frees tcon and (by ownership) tcon->compat, still leaving the
freed tcon->compat pointer on the sconn->connections linked list.
When deadtime_fn() fires and walks the sconn->connections list it
indirects this freed pointer. We must call close_cnum() on error also.
Valgrind trace from Noel Power <noel.power at suse.com> is:
==6432== Invalid read of size 8
==6432== at 0x52CED3A: conn_lastused_update (conn_idle.c:38)
==6432== by 0x52CEDB1: conn_idle_all (conn_idle.c:54)
==6432== by 0x5329971: deadtime_fn (smb2_process.c:1566)
==6432== by 0x5DA2339: smbd_idle_event_handler (util_event.c:45)
==6432== by 0x685F2F8: tevent_common_invoke_timer_handler (tevent_timed.c:376)
==6432== Address 0x19074b88 is 232 bytes inside a block of size 328 free'd
==6432== at 0x4C3451B: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==6432== by 0x5B38521: _tc_free_internal (talloc.c:1222)
==6432== by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
==6432== by 0x5B38404: _tc_free_internal (talloc.c:1184)
==6432== by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
==6432== by 0x5B38404: _tc_free_internal (talloc.c:1184)
==6432== by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
==6432== by 0x5B38404: _tc_free_internal (talloc.c:1184)
==6432== by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
==6432== by 0x5B38404: _tc_free_internal (talloc.c:1184)
==6432== by 0x5B385C5: _talloc_free_internal (talloc.c:1248)
==6432== by 0x5B3988D: _talloc_free (talloc.c:1792)
==6432== by 0x5349B22: smbd_smb2_flush_send_queue (smb2_server.c:4828)
==6432== Block was alloc'd at
==6432== at 0x4C332EF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==6432== by 0x5B378D9: __talloc_with_prefix (talloc.c:783)
==6432== by 0x5B37A73: __talloc (talloc.c:825)
==6432== by 0x5B37E0C: _talloc_named_const (talloc.c:982)
==6432== by 0x5B3A8ED: _talloc_zero (talloc.c:2421)
==6432== by 0x539873A: conn_new (conn.c:70)
==6432== by 0x532D692: make_connection_smb2 (smb2_service.c:909)
==6432== by 0x5352B5E: smbd_smb2_tree_connect (smb2_tcon.c:344)
https://bugzilla.samba.org/show_bug.cgi?id=15128
Signed-off-by: Jeremy Allison <jra at samba.org>
Reviewed-by: Noel Power <npower at samba.org>
-----------------------------------------------------------------------
Summary of changes:
source3/smbd/conn.c | 36 +++++++++++++++++++++++++-----------
source3/smbd/smbXsrv_tcon.c | 9 +++++++++
2 files changed, 34 insertions(+), 11 deletions(-)
Changeset truncated at 500 lines:
diff --git a/source3/smbd/conn.c b/source3/smbd/conn.c
index 044242d5697..776d7af4c12 100644
--- 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);
}
/*
diff --git a/source3/smbd/smbXsrv_tcon.c b/source3/smbd/smbXsrv_tcon.c
index 6b105522855..b515b19e88f 100644
--- a/source3/smbd/smbXsrv_tcon.c
+++ b/source3/smbd/smbXsrv_tcon.c
@@ -913,6 +913,15 @@ NTSTATUS smbXsrv_tcon_disconnect(struct smbXsrv_tcon *tcon, uint64_t vuid)
tcon->global->tcon_global_id,
tcon->global->share_name,
nt_errstr(status)));
+ /*
+ * We must call close_cnum() on
+ * error, as the caller is going
+ * to free tcon and tcon->compat
+ * so we must ensure tcon->compat is
+ * removed from the linked list
+ * conn->sconn->connections.
+ */
+ close_cnum(tcon->compat, vuid);
tcon->compat = NULL;
return status;
}
--
Samba Shared Repository
More information about the samba-cvs
mailing list