mirror of
https://github.com/samba-team/samba.git
synced 2025-03-03 12:58:35 +03:00
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.
This commit is contained in:
parent
d8de7e3193
commit
e46a88ce35
@ -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; 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;
|
||||
@ -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;
|
||||
}
|
||||
|
Loading…
x
Reference in New Issue
Block a user