[Samba] 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
mailing list