From 8ce86de0e1d37f5436e5ae0def4025a58d92bebf Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 9 Mar 2016 15:55:44 +1300 Subject: [PATCH] smbd: Harden open directory handling against races The smb2.create.mkdir-dup test creates a race, and against an AD DC this can cause a flapping test if the lstat() and stat() calls are made either side of the chown() due to creation of a file by administrator. When we have an FD open, we can rely on the dev/inode not being re-used and so can have a more relaxed check. Signed-off-by: Andrew Bartlett --- source3/smbd/open.c | 103 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 64 insertions(+), 39 deletions(-) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index baebd7c..bc676f9 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -3397,6 +3397,7 @@ static NTSTATUS open_directory(connection_struct *conn, struct timespec mtimespec; int info = 0; bool ok; + bool need_lstat = false; if (is_ntfs_stream_smb_fname(smb_dname)) { DEBUG(2, ("open_directory: %s is a stream name!\n", @@ -3502,25 +3503,9 @@ static NTSTATUS open_directory(connection_struct *conn, return status; } - /* - * If mkdir_internal() returned - * NT_STATUS_OBJECT_NAME_COLLISION - * we still must lstat the path. - */ - - if (SMB_VFS_LSTAT(conn, smb_dname) - == -1) { - DEBUG(2, ("Could not stat " - "directory '%s' just " - "opened: %s\n", - smb_fname_str_dbg( - smb_dname), - strerror(errno))); - return map_nt_error_from_unix( - errno); - } - + need_lstat = true; info = FILE_WAS_OPENED; + } } @@ -3537,26 +3522,20 @@ static NTSTATUS open_directory(connection_struct *conn, return NT_STATUS_INVALID_PARAMETER; } - if(!S_ISDIR(smb_dname->st.st_ex_mode)) { - DEBUG(5,("open_directory: %s is not a directory !\n", - smb_fname_str_dbg(smb_dname))); - return NT_STATUS_NOT_A_DIRECTORY; - } - if (info == FILE_WAS_OPENED) { status = smbd_check_access_rights(conn, - smb_dname, - false, - access_mask); + smb_dname, + false, + access_mask); if (!NT_STATUS_IS_OK(status)) { DEBUG(10, ("open_directory: smbd_check_access_rights on " - "file %s failed with %s\n", - smb_fname_str_dbg(smb_dname), - nt_errstr(status))); + "file %s failed with %s\n", + smb_fname_str_dbg(smb_dname), + nt_errstr(status))); return status; } } - + status = file_new(req, conn, &fsp); if(!NT_STATUS_IS_OK(status)) { return status; @@ -3635,14 +3614,60 @@ static NTSTATUS open_directory(connection_struct *conn, return status; } - /* Ensure there was no race condition. */ - if (!check_same_stat(&smb_dname->st, &fsp->fsp_name->st)) { - DEBUG(5,("open_directory: stat struct differs for " - "directory %s.\n", - smb_fname_str_dbg(smb_dname))); - fd_close(fsp); - file_free(req, fsp); - return NT_STATUS_ACCESS_DENIED; + if(!S_ISDIR(fsp->fsp_name->st.st_ex_mode)) { + DEBUG(5,("open_directory: %s is not a directory !\n", + smb_fname_str_dbg(smb_dname))); + return NT_STATUS_NOT_A_DIRECTORY; + } + + if (need_lstat) { + /* + * If mkdir_internal() returned + * NT_STATUS_OBJECT_NAME_COLLISION + * we still must lstat the path. + * + * We do this after we open the file, so that we hold the + * dev/inode open and so can relax the stat check below. + */ + + if (SMB_VFS_LSTAT(conn, smb_dname) + == -1) { + DEBUG(2, ("Could not stat " + "directory '%s' just " + "opened: %s\n", + smb_fname_str_dbg( + smb_dname), + strerror(errno))); + fd_close(fsp); + file_free(req, fsp); + return map_nt_error_from_unix( + errno); + } + } + + /* Ensure there was no race condition. We can relax this check when we + have a valid FD for the directory, because while we hold the + directory open, the dev/inode can not be re-used.*/ + if (fsp->fh->fd != -1) { + if (!check_same_dev_ino(&smb_dname->st, &fsp->fsp_name->st)) { + samba_start_debugger(); + DEBUG(5,("open_directory: dev/inode differs for " + "directory %s.\n", + smb_fname_str_dbg(smb_dname))); + fd_close(fsp); + file_free(req, fsp); + return NT_STATUS_ACCESS_DENIED; + } + } else { + if (!check_same_stat(&smb_dname->st, &fsp->fsp_name->st)) { + samba_start_debugger(); + DEBUG(5,("open_directory: stat struct differs for " + "directory %s.\n", + smb_fname_str_dbg(smb_dname))); + fd_close(fsp); + file_free(req, fsp); + return NT_STATUS_ACCESS_DENIED; + } } lck = get_share_mode_lock(talloc_tos(), fsp->file_id, -- 2.5.0