From e46a88ce35e1aba9d9a344773bc97a9f3f2bd616 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 4 May 2009 08:31:40 -0700 Subject: Fix bug #6315 smbd crashes doing vfs_full_audit on IPC$ close event. The underlying problem is that once SMBulogoff is called, all server_info contexts associated with the vuid should become invalid, even if that's the context being currently used by the connection struct (tid). When the SMBtdis comes in it doesn't need a valid vuid value, but the code called inside vfs_full_audit always assumes that there is one (and hence a valid conn->server_info pointer) available. 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. --- source3/smbd/uid.c | 47 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c index b8ed321a45..72837ff967 100644 --- a/source3/smbd/uid.c +++ b/source3/smbd/uid.c @@ -52,6 +52,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. @@ -75,6 +95,7 @@ static bool check_user_ok(connection_struct *conn, for (i=0; ivuid_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; @@ -140,6 +161,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; } @@ -151,6 +173,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) @@ -164,11 +187,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; } -- cgit