Attachment 11907 Details for Bug 11780 – Original proposed patch

[patch] Original proposed patch

0001-smbd-Harden-open-directory-handling-against-races.patch (text/plain), 4.61 KB, created by Jeremy Allison on 2016-03-09 17:26:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2016-03-09 17:26:30 UTC

Size: 4.61 KB

patch

obsolete

>From 8ce86de0e1d37f5436e5ae0def4025a58d92bebf Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>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 <abartlet@samba.org>
>---
> 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
>

Actions: View

Attachments on bug 11780: 11907 | 11908 | 11910