mkdir-dup test flapping

Jeremy Allison jra at samba.org
Wed Mar 9 04:31:04 UTC 2016


On Wed, Mar 09, 2016 at 04:11:05PM +1300, Douglas Bagnall wrote:
> We looked at this some more, and Andrew seemed to understand and wrote
> the attached patch.

Great catch you guys ! Thanks for taking a look,
I've been busy on other stuff for the past few
weeks.

Let me go through the issue and proposed code
*really* carefully :-). The open code can
be really tricky (as you've already found).

Thanks !

Jeremy.

> The issue appears to be in this call:
> 
> 3638            /* Ensure there was no race condition. */
> 3639            if (!check_same_stat(&smb_dname->st, &fsp->fsp_name->st)) {
> 3640                    samba_start_debugger();
> 3641                    DEBUG(5,("open_directory: stat struct differs for "
> 3642                            "directory %s.\n",
> 3643                            smb_fname_str_dbg(smb_dname)));
> 3644                    fd_close(fsp);
> (gdb) p smb_dname->st
> $1 = {st_ex_dev = 64512, st_ex_ino = 44701075, st_ex_mode = 16877,
>   st_ex_nlink = 2, st_ex_uid = 50133, st_ex_gid = 50133, st_ex_rdev = 0,
>   st_ex_size = 0, st_ex_atime = {tv_sec = 1457488009, tv_nsec = 820161188},
>   st_ex_mtime = {tv_sec = 1457488009, tv_nsec = 820161188}, st_ex_ctime = {
>     tv_sec = 1457488009, tv_nsec = 820161188}, st_ex_btime = {
>     tv_sec = 1457488009, tv_nsec = 820161188},
>   st_ex_calculated_birthtime = true, st_ex_blksize = 4096, st_ex_blocks = 8,
>   st_ex_flags = 0, st_ex_mask = 0}
> (gdb) p fsp->fsp_name->st
> $2 = {st_ex_dev = 64512, st_ex_ino = 44701075, st_ex_mode = 16877,
>   st_ex_nlink = 2, st_ex_uid = 3000000, st_ex_gid = 65531, st_ex_rdev = 0,
>   st_ex_size = 0, st_ex_atime = {tv_sec = 1457488009, tv_nsec = 820161188},
>   st_ex_mtime = {tv_sec = 1457488009, tv_nsec = 820161188}, st_ex_ctime = {
>     tv_sec = 1457488009, tv_nsec = 820161188}, st_ex_btime = {
>     tv_sec = 1457488009, tv_nsec = 820161188},
>   st_ex_calculated_birthtime = true, st_ex_blksize = 4096, st_ex_blocks = 8,
>   st_ex_flags = 0, st_ex_mask = 0}
> (gdb)
> 
> where you can see the st_ex_uid and st_ex_gid fields differ:
> 
> douglasb at douglasb-desktop:~/src/samba (flap *)$ bin/wbinfo --uid-info=50133
> ADDOMAIN/administrator:*:50133:65531::/home/ADDOMAIN/administrator:/bin/false
> douglasb at douglasb-desktop:~/src/samba (flap *)$ bin/wbinfo --uid-info=3000000
> BUILTIN/administrators:*:3000000:3000000::/home/BUILTIN/administrators:/bin/false
> 
> The issue is that after the mkdir, we run:
> 
> 		} else if (lp_inherit_acls(SNUM(conn))) {
> 			/* Inherit from parent. Errors here are not fatal. */
> 			status = inherit_new_acl(fsp);
> 			if (!NT_STATUS_IS_OK(status)) {
> 
> while in the other process that is doing the open of the existing
> directory, we do:
> 
> 					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);
> 					}
> 
> And then only a few lines later we do:
> 
>     !check_same_stat(&smb_dname->st, &fsp->fsp_name->st)) {
> 
> The inherit_new_acl() can change the owner of the directory in the
> time it takes between the first stat() and the second one. This is
> seen only in the AD DC because of special logic that makes
> BUILTIN\Administrators the owner of files created by any
> administrative user.
> 
> We need to decide if the change in owner is enough reason to refuse
> the open(), or if we should just continue.
> 
> On the basis that the owner change is not significant, we reworked the
> code to do the lstat() after the fstat(), and not compare the owner.
> 
> Another way to avoid the flapping would be to run this test as someone
> other than Administrator.
> 
> Douglas and Andrew

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




More information about the samba-technical mailing list