From: Stefan Metzmacher <metze@samba.org>
Date: Thu, 1 Aug 2024 12:37:55 +0000 (+0200)
Subject: s3:smbd: let mkdir_internal() work more atomically using a temporary name
X-Git-Tag: tdb-1.4.13~1315
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7baeeece2dd563d404352e5f3e34661d1cbc2b03;p=thirdparty%2Fsamba.git

s3:smbd: let mkdir_internal() work more atomically using a temporary name

Currently we do this in mkdir_internal():

mkdirat(client_name);
if (EEXIST) {
   return EEXIST;
}
prepare_acls(client_name);

Note 'prepare_acls()' is a placeholder for the complex steps
it is doing to prepare the directory. During these steps
we have the problem that other clients already see
the directory and are able to create files or subdirectories
in it and these may not inherit the correct acls as
the their parent directory is not created completely.

I think I found a good strategie even without relying on
renameat2(RENAME_NOREPLACE).

We would do this instead:

tmp_name = ".::TMPNAME:D:$PID:client_name"
mkdirat(tmp_name, mode=client_mode);
prepare_acls(tmp_name);
mkdirat(client_name, mode=0);
if (EEXIST) {
   unlinkat(tmp_name);
   return EEXIST;
}
renameat(tmp_name, client_name);

So instead of having a long windows during prepare_acls,
we just have a short window between mkdirat(client_name, mode=0)
and renameat(tmp_name, client_name);
And in that short window the directory with the client_name
has a mode of 0, so it's not possible for other clients
to create files or subdirs in it.

As the mkdirat(client_name, mode=0) still catches
EEXIST the race where two clients try to create
the same client_name is closed as before,
so we don't need any other protection.

Following patches will make use of renameat2(RENAME_NOREPLACE),
but this already a very good improvement.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15693

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
---

diff --git a/selftest/knownfail.d/samba3.smb2.create.mkdir-visible b/selftest/knownfail.d/samba3.smb2.create.mkdir-visible
deleted file mode 100644
index 9d16b7ca9d0..00000000000
--- a/selftest/knownfail.d/samba3.smb2.create.mkdir-visible
+++ /dev/null
@@ -1 +0,0 @@
-^samba3.smb2.create.mkdir-visible
diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
index 76eb5756dc8..147515fe2f5 100644
--- a/source3/smbd/dir.c
+++ b/source3/smbd/dir.c
@@ -1186,11 +1186,67 @@ const char *ReadDirName(struct smb_Dir *dir_hnd, char **ptalloced)
 				    dir_hnd->fsp,
 				    dir_hnd->dir,
 				    &talloced))) {
+		int unlink_flags = INT_MAX;
 		/* Ignore . and .. - we've already returned them. */
 		if (ISDOT(n) || ISDOTDOT(n)) {
 			TALLOC_FREE(talloced);
 			continue;
 		}
+		/*
+		 * ignore tmp directories, see mkdir_internals()
+		 */
+		if (IS_SMBD_TMPNAME(n, &unlink_flags)) {
+			struct smb_filename *atname = NULL;
+			const char *fp = NULL;
+			int ret;
+
+			atname = synthetic_smb_fname(talloc_tos(),
+						     n,
+						     NULL,
+						     NULL,
+						     0,
+						     0);
+			if (atname == NULL) {
+				TALLOC_FREE(talloced);
+				continue;
+			}
+			fp = full_path_from_dirfsp_at_basename(atname,
+							       dir_hnd->fsp,
+							       n);
+			if (fp == NULL) {
+				TALLOC_FREE(atname);
+				TALLOC_FREE(talloced);
+				continue;
+			}
+
+			if (unlink_flags == INT_MAX) {
+				DBG_NOTICE("ignoring %s\n", fp);
+				TALLOC_FREE(atname);
+				TALLOC_FREE(talloced);
+				continue;
+			}
+
+			/*
+			 * We remove the stale tmpname
+			 * as root and ignore any errors
+			 */
+			DBG_NOTICE("unlink stale %s\n", fp);
+			become_root();
+			ret = SMB_VFS_UNLINKAT(conn,
+					       dir_hnd->fsp,
+					       atname,
+					       unlink_flags);
+			unbecome_root();
+			if (ret == 0) {
+				DBG_NOTICE("unlinked stale %s\n", fp);
+			} else {
+				DBG_WARNING("failed to unlink stale %s: %s\n",
+					    fp, strerror(errno));
+			}
+			TALLOC_FREE(atname);
+			TALLOC_FREE(talloced);
+			continue;
+		}
 		*ptalloced = talloced;
 		dir_hnd->file_number++;
 		return n;
diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
index 7d0d924dd73..8180f336893 100644
--- a/source3/smbd/globals.h
+++ b/source3/smbd/globals.h
@@ -842,4 +842,13 @@ struct aio_extra {
 	bool write_through;
 };
 
+#define SMBD_TMPNAME_PREFIX ".::TMPNAME:"
+#define SMBD_TMPDIR_PREFIX  SMBD_TMPNAME_PREFIX "D:"
+#define IS_SMBD_TMPNAME_PREFIX(__n) \
+	unlikely(__n[0] == '.' && __n[1] == ':' && \
+	 strncmp(__n, SMBD_TMPNAME_PREFIX, sizeof(SMBD_TMPNAME_PREFIX) -1) == 0)
+#define IS_SMBD_TMPNAME(__n, __unlink_flags) \
+	(IS_SMBD_TMPNAME_PREFIX(__n) && \
+	 smbd_is_tmpname(__n, __unlink_flags))
+
 #endif /* _SOURCE3_SMBD_GLOBALS_H_ */
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index d560f64f9e6..108a22caa44 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -4600,6 +4600,61 @@ static NTSTATUS apply_new_nt_acl(struct files_struct *dirfsp,
 	return NT_STATUS_OK;
 }
 
+bool smbd_is_tmpname(const char *n, int *_unlink_flags)
+{
+	const char *p = n;
+	int unlink_flags = INT_MAX;
+	struct server_id id;
+	bool exists;
+
+	if (_unlink_flags != NULL) {
+		*_unlink_flags = INT_MAX;
+	}
+
+	if (!IS_SMBD_TMPNAME_PREFIX(n)) {
+		return false;
+	}
+	p += sizeof(SMBD_TMPNAME_PREFIX) - 1;
+	switch (p[0]) {
+	case 'D':
+		unlink_flags = AT_REMOVEDIR;
+		break;
+	default:
+		return false;
+	}
+	p += 1;
+	if (p[0] != ':') {
+		return false;
+	}
+	p += 1;
+
+	id = server_id_from_string_ex(get_my_vnn(), '%', p);
+	if (id.pid == UINT64_MAX) {
+		return false;
+	}
+	if (id.unique_id == 0) {
+		return false;
+	}
+	if (id.unique_id == SERVERID_UNIQUE_ID_NOT_TO_VERIFY) {
+		return false;
+	}
+
+	if (_unlink_flags == NULL) {
+		return true;
+	}
+
+	exists = serverid_exists(&id);
+	if (!exists) {
+		/*
+		 * let the caller know it's stale
+		 * and should be removed
+		 */
+		*_unlink_flags = unlink_flags;
+	}
+
+	return true;
+}
+
 static NTSTATUS mkdir_internal(connection_struct *conn,
 			       struct smb_filename *parent_dir_fname, /* parent. */
 			       struct smb_filename *smb_fname_atname, /* atname relative to parent. */
@@ -4616,6 +4671,15 @@ static NTSTATUS mkdir_internal(connection_struct *conn,
 	bool posix_open = false;
 	bool need_re_stat = false;
 	uint32_t access_mask = SEC_DIR_ADD_SUBDIR;
+	struct smb_filename *first_atname = NULL;
+	struct smb_filename *tmp_atname = NULL;
+	char *orig_dname = NULL;
+	char *tmp_dname = NULL;
+	int vfs_use_tmp = lp_vfs_mkdir_use_tmp_name(SNUM(conn));
+	bool need_tmpname = false;
+	struct server_id id = messaging_server_id(conn->sconn->msg_ctx);
+	struct server_id_buf idbuf;
+	char *idstr = server_id_str_buf_unique_ex(id, '%', &idbuf);
 	struct vfs_open_how how = { .flags = O_RDONLY|O_DIRECTORY, };
 	int ret;
 
@@ -4651,18 +4715,86 @@ static NTSTATUS mkdir_internal(connection_struct *conn,
 		if (directory_has_default_acl_fsp(parent_dir_fname->fsp)) {
 			mode = (0777 & lp_directory_mask(SNUM(conn)));
 		}
+		need_tmpname = true;
+	} else if (lp_store_dos_attributes(SNUM(conn))) {
+		need_tmpname = true;
+	} else if (lp_inherit_permissions(SNUM(conn))) {
+		need_tmpname = true;
+	} else if (lp_inherit_owner(SNUM(conn)) != INHERIT_OWNER_NO) {
+		need_tmpname = true;
+	} else if (lp_nt_acl_support(SNUM(conn)) && sd != NULL) {
+		need_tmpname = true;
+	}
+
+	if (vfs_use_tmp != Auto) {
+		need_tmpname = vfs_use_tmp;
+	}
+
+	if (!need_tmpname) {
+		first_atname = smb_fname_atname;
+		goto mkdir_first;
+	}
+
+	/*
+	 * In order to avoid races where other clients could
+	 * see the directory before it is setup completely
+	 * we use a temporary name and rename it
+	 * when everything is ready.
+	 */
+
+	orig_dname = smb_dname->base_name;
+
+	tmp_atname = cp_smb_filename(frame,
+				     smb_fname_atname);
+	if (tmp_atname == NULL) {
+		TALLOC_FREE(frame);
+		return NT_STATUS_NO_MEMORY;
+	}
+	TALLOC_FREE(tmp_atname->base_name);
+	tmp_atname->base_name = talloc_asprintf(tmp_atname,
+						"%s%s:%s",
+						SMBD_TMPDIR_PREFIX,
+						idstr,
+						smb_fname_atname->base_name);
+	if (tmp_atname == NULL) {
+		TALLOC_FREE(frame);
+		return NT_STATUS_NO_MEMORY;
+	}
+	SMB_ASSERT(smbd_is_tmpname(tmp_atname->base_name, NULL));
+	if (!ISDOT(parent_dir_fname->base_name)) {
+		tmp_dname = talloc_asprintf(frame,
+					    "%s/%s",
+					    parent_dir_fname->base_name,
+					    tmp_atname->base_name);
+		if (tmp_dname == NULL) {
+			TALLOC_FREE(frame);
+			return NT_STATUS_NO_MEMORY;
+		}
+	} else {
+		tmp_dname = talloc_strdup(frame, tmp_atname->base_name);
+		if (tmp_dname == NULL) {
+			TALLOC_FREE(frame);
+			return NT_STATUS_NO_MEMORY;
+		}
 	}
 
+	smb_dname->base_name = tmp_dname;
+
+	DBG_DEBUG("temporary replace '%s' by '%s'\n",
+		  orig_dname, tmp_dname);
+
+	first_atname = tmp_atname;
+
+mkdir_first:
 	ret = SMB_VFS_MKDIRAT(conn,
 			      parent_dir_fname->fsp,
-			      smb_fname_atname,
+			      first_atname,
 			      mode);
 	if (ret != 0) {
 		status = map_nt_error_from_unix(errno);
 		DBG_NOTICE("MKDIRAT failed for '%s': %s\n",
 			   smb_fname_str_dbg(smb_dname), nt_errstr(status));
-		TALLOC_FREE(frame);
-		return status;
+		goto restore_orig;
 	}
 
 	/*
@@ -4671,12 +4803,11 @@ static NTSTATUS mkdir_internal(connection_struct *conn,
 	 */
 	fsp->fsp_flags.is_pathref = true;
 
-	status = fd_openat(parent_dir_fname->fsp, smb_fname_atname, fsp, &how);
+	status = fd_openat(parent_dir_fname->fsp, first_atname, fsp, &how);
 	if (!NT_STATUS_IS_OK(status)) {
 		DBG_ERR("fd_openat() failed for '%s': %s\n",
 			smb_fname_str_dbg(smb_dname), nt_errstr(status));
-		TALLOC_FREE(frame);
-		return status;
+		goto restore_orig;
 	}
 
 	/* Ensure we're checking for a symlink here.... */
@@ -4686,16 +4817,14 @@ static NTSTATUS mkdir_internal(connection_struct *conn,
 	if (!NT_STATUS_IS_OK(status)) {
 		DBG_ERR("Could not stat directory '%s' just created: %s\n",
 			smb_fname_str_dbg(smb_dname), nt_errstr(status));
-		TALLOC_FREE(frame);
-		return status;
+		goto restore_orig;
 	}
 
 	if (!S_ISDIR(smb_dname->st.st_ex_mode)) {
-		status = NT_STATUS_NOT_A_DIRECTORY;
 		DBG_ERR("Directory '%s' just created is not a directory !\n",
 			smb_fname_str_dbg(smb_dname));
-		TALLOC_FREE(frame);
-		return status;
+		status = NT_STATUS_NOT_A_DIRECTORY;
+		goto restore_orig;
 	}
 
 	if (lp_store_dos_attributes(SNUM(conn))) {
@@ -4740,8 +4869,7 @@ static NTSTATUS mkdir_internal(connection_struct *conn,
 		if (!NT_STATUS_IS_OK(status)) {
 			DBG_ERR("Could not stat directory '%s' just created: %s\n",
 				smb_fname_str_dbg(smb_dname), nt_errstr(status));
-			TALLOC_FREE(frame);
-			return status;
+			goto restore_orig;
 		}
 	}
 
@@ -4753,11 +4881,71 @@ static NTSTATUS mkdir_internal(connection_struct *conn,
 			DBG_WARNING("apply_new_nt_acl() failed for %s with %s\n",
 				    fsp_str_dbg(fsp),
 				    nt_errstr(status));
-			TALLOC_FREE(frame);
-			return status;
+			goto do_unlink;
+		}
+	}
+
+	if (!need_tmpname) {
+		goto done;
+	}
+
+	/*
+	 * A rename needs a valid stat for the source,
+	 * see vfs_fruit.c ...
+	 */
+	tmp_atname->st = smb_dname->st;
+
+	{
+		/*
+		 * This is the strategie we use without having
+		 * renameat2(RENAME_NOREPLACE):
+		 *
+		 * renameat() is able to replace a directory if the source is
+		 * also a directory.
+		 *
+		 * So in order to avoid races as much as possible we do a
+		 * mkdirat() with mode 0 in order to catch EEXIST almost
+		 * atomically, when this code runs by two processes at the same
+		 * time.
+		 *
+		 * Then a renameat() makes the temporary directory available for
+		 * clients.
+		 *
+		 * This a much smaller window where the other clients may see
+		 * the incomplete directory, which has a mode of 0.
+		 */
+
+		DBG_DEBUG("MKDIRAT/RENAMEAT '%s' -> '%s'\n",
+			  tmp_dname, orig_dname);
+
+		ret = SMB_VFS_MKDIRAT(conn,
+				      parent_dir_fname->fsp,
+				      smb_fname_atname,
+				      0);
+		if (ret != 0) {
+			status = map_nt_error_from_unix(errno);
+			DBG_NOTICE("MKDIRAT failed for '%s': %s\n",
+				   orig_dname, nt_errstr(status));
+			goto do_unlink;
 		}
+
+		ret = SMB_VFS_RENAMEAT(conn,
+				       parent_dir_fname->fsp,
+				       tmp_atname,
+				       parent_dir_fname->fsp,
+				       smb_fname_atname);
 	}
 
+	if (ret != 0) {
+		status = map_nt_error_from_unix(errno);
+		DBG_NOTICE("RENAMEAT failed for '%s' -> '%s': %s\n",
+			   tmp_dname, orig_dname, nt_errstr(status));
+		goto do_unlink;
+	}
+	smb_fname_atname->st = tmp_atname->st;
+	smb_dname->base_name = orig_dname;
+
+done:
 	DBG_DEBUG("Created directory '%s'\n",
 		  smb_fname_str_dbg(smb_dname));
 
@@ -4766,6 +4954,38 @@ static NTSTATUS mkdir_internal(connection_struct *conn,
 
 	TALLOC_FREE(frame);
 	return NT_STATUS_OK;
+
+do_unlink:
+	DBG_NOTICE("%s: rollback and unlink '%s'\n",
+		   nt_errstr(status),
+		   tmp_dname);
+	ret = SMB_VFS_UNLINKAT(conn,
+			       parent_dir_fname->fsp,
+			       tmp_atname,
+			       AT_REMOVEDIR);
+	if (ret == 0) {
+		DBG_NOTICE("SMB_VFS_UNLINKAT(%s): OK\n",
+			   tmp_dname);
+	} else {
+		NTSTATUS status2 = map_nt_error_from_unix(errno);
+		DBG_WARNING("SMB_VFS_UNLINKAT(%s) ignoring %s\n",
+			    tmp_dname, nt_errstr(status2));
+	}
+
+restore_orig:
+	if (!need_tmpname) {
+		TALLOC_FREE(frame);
+		return status;
+	}
+	DBG_NOTICE("%s: restoring '%s' -> '%s'\n",
+		   nt_errstr(status),
+		   tmp_dname,
+		   orig_dname);
+	SET_STAT_INVALID(smb_fname_atname->st);
+	smb_dname->base_name = orig_dname;
+	SET_STAT_INVALID(smb_dname->st);
+	TALLOC_FREE(frame);
+	return status;
 }
 
 /****************************************************************************
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 90387aeb6c3..429d8875f0a 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -657,6 +657,7 @@ NTSTATUS smbd_check_access_rights_fsp(struct files_struct *dirfsp,
 				      uint32_t access_mask);
 NTSTATUS check_parent_access_fsp(struct files_struct *fsp,
 				uint32_t access_mask);
+bool smbd_is_tmpname(const char *n, int *_unlink_flags);
 NTSTATUS fd_openat(const struct files_struct *dirfsp,
 		   struct smb_filename *smb_fname,
 		   files_struct *fsp,
diff --git a/source3/smbd/smb2_reply.c b/source3/smbd/smb2_reply.c
index 2b65fb30d76..1248c6f0a75 100644
--- a/source3/smbd/smb2_reply.c
+++ b/source3/smbd/smb2_reply.c
@@ -166,6 +166,8 @@ NTSTATUS check_path_syntax(char *path, bool posix_path)
 					s++;
 					continue;
 				}
+			} else if (IS_SMBD_TMPNAME(s, NULL)) {
+				return NT_STATUS_OBJECT_NAME_INVALID;
 			}
 
 		}