Fix bug #6315 smbd crashes doing vfs_full_audit on IPC$ close event. The underlying...

This is actually a bug inside the vfs_full_audit and other code inside Samba,
which should only indirect conn->server_info on calls which require AS_USER to
be set in our process table. I could fix all these issues, but there's no
guarentee that someone might not add more code that fails this assumption, as
it's a hard assumption to break (it's usually true).

So what I've done is to ensure that on SMBulogoff the previously used
conn->server_info struct is kept around to be used for print debugging purposes
(it won't be used to change to an invalid user context, as such calls need
AS_USER set). This isn't strictly correct, as there's no association with the
(now invalid) context being freed and the call that causes conn->server_info to
be indirected, but it's good enough for most cases.

The hard part was to ensure that once a valid context is used again (via new
sessionsetupX calls, or new calls on a still valid vuid on this tid) that we
don't leak memory by simply replacing the stored conn->server_info pointer. We
would never actually leak the memory (as all conn->server_info pointers are
talloc children of conn), but with the previous patch a malicious client could
cause many server_info structs to be talloced by the right combination of SMB
calls. This new patch introduces free_conn_server_info_if_unused(), which
protects against the above.
Jeremy.

This was commit e46a88ce35e1aba9d9a344773bc97a9f3f2bd616 in master.

diff --git a/source/smbd/uid.c b/source/smbd/uid.c
index bec820b719920f5c38343a3379c4c9928158b654..02aadbe59e6658f041926959392c832a975b77ff 100644 (file)
--- a/source/smbd/uid.c
+++ b/source/smbd/uid.c
@@ -54,6 +54,26 @@ bool change_to_guest(void)
        return True;
 }
 
+/****************************************************************************
+ talloc free the conn->server_info if not used in the vuid cache.
+****************************************************************************/
+
+static void free_conn_server_info_if_unused(connection_struct *conn)
+{
+       unsigned int i;
+
+       for (i = 0; i < VUID_CACHE_SIZE; i++) {
+               struct vuid_cache_entry *ent;
+               ent = &conn->vuid_cache.array[i];
+               if (ent->vuid != UID_FIELD_INVALID &&
+                               conn->server_info == ent->server_info) {
+                       return;
+               }
+       }
+       /* Not used, safe to free. */
+       TALLOC_FREE(conn->server_info);
+}
+
 /*******************************************************************
  Check if a username is OK.
 
@@ -77,6 +97,7 @@ static bool check_user_ok(connection_struct *conn,
                for (i=0; i<VUID_CACHE_SIZE; i++) {
                        ent = &conn->vuid_cache.array[i];
                        if (ent->vuid == vuid) {
+                               free_conn_server_info_if_unused(conn);
                                conn->server_info = ent->server_info;
                                conn->read_only = ent->read_only;
                                conn->admin_user = ent->admin_user;
@@ -142,6 +163,7 @@ static bool check_user_ok(connection_struct *conn,
                ent->vuid = vuid;
                ent->read_only = readonly_share;
                ent->admin_user = admin_user;
+               free_conn_server_info_if_unused(conn);
                conn->server_info = ent->server_info;
        }
 
@@ -153,6 +175,7 @@ static bool check_user_ok(connection_struct *conn,
 
 /****************************************************************************
  Clear a vuid out of the connection's vuid cache
+ This is only called on SMBulogoff.
 ****************************************************************************/
 
 void conn_clear_vuid_cache(connection_struct *conn, uint16_t vuid)
@@ -166,11 +189,29 @@ void conn_clear_vuid_cache(connection_struct *conn, uint16_t vuid)
 
                if (ent->vuid == vuid) {
                        ent->vuid = UID_FIELD_INVALID;
-                       /* Ensure we're not freeing an active pointer. */
+                       /*
+                        * We need to keep conn->server_info around
+                        * if it's equal to ent->server_info as a SMBulogoff
+                        * is often followed by a SMBtdis (with an invalid
+                        * vuid). The debug code (or regular code in
+                        * vfs_full_audit) wants to refer to the
+                        * conn->server_info pointer to print debug
+                        * statements. Theoretically this is a bug,
+                        * as once the vuid is gone the server_info
+                        * on the conn struct isn't valid any more,
+                        * but there's enough code that assumes
+                        * conn->server_info is never null that
+                        * it's easier to hold onto the old pointer
+                        * until we get a new sessionsetupX.
+                        * As everything is hung off the
+                        * conn pointer as a talloc context we're not
+                        * leaking memory here. See bug #6315. JRA.
+                        */
                        if (conn->server_info == ent->server_info) {
-                               conn->server_info = NULL;
+                               ent->server_info = NULL;
+                       } else {
+                               TALLOC_FREE(ent->server_info);
                        }
-                       TALLOC_FREE(ent->server_info);
                        ent->read_only = False;
                        ent->admin_user = False;
                }