Attachment 15217 Details for Bug 13979 – git-am fix for master v3

[patch] git-am fix for master v3

bug-13979-wip-v3.patchset (text/plain), 10.29 KB, created by Jeremy Allison on 2019-06-04 03:14:42 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2019-06-04 03:14:42 UTC

Size: 10.29 KB

patch

obsolete

>From 78940cc5de97021fbaecad81cd653e7ece8f2a45 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Mon, 3 Jun 2019 19:08:49 -0700
>Subject: [PATCH 1/3] WIP. Ensure unbecome_root() doesn't call chdir() to
> restore conn->cwd_fname.
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/smbd/uid.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
>diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c
>index a4bcb747d37..732fd604610 100644
>--- a/source3/smbd/uid.c
>+++ b/source3/smbd/uid.c
>@@ -621,6 +621,29 @@ void smbd_become_root(void)
> void smbd_unbecome_root(void)
> {
> 	pop_sec_ctx();
>+
>+	/*
>+	 * We need this uglyness as become_root()/unbecome_root()
>+	 * pairs should only change credentials, and *NOT* change
>+	 * the current working directory. This is the case for
>+	 * all callers in smbd.
>+	 *
>+	 * If code inside become_root()/unbecome_root() pairs
>+	 * changes $cwd, it *must* restore it and check for
>+	 * the restored path being safely under the share after
>+	 * doing so.
>+	 */
>+
>+	/*
>+	 * Set current_user.done_chdir to false so pop_conn_ctx()
>+	 * never calls chdir() from unbecome_root().
>+	 *
>+	 * pop_conn_ctx() overwrites this with the contents of
>+	 * the previous connection context stack entry so we
>+	 * don't need to save it.
>+	 */
>+	current_user.done_chdir = false;
>+
> 	pop_conn_ctx();
> }
> 
>-- 
>2.17.1
>
>
>From 27587d54a474dc46d8989a61897c99f26c2cc1a3 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 31 May 2019 14:46:16 -0700
>Subject: [PATCH 2/3] WIP: prototype fix for mkdir races.
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/smbd/open.c | 184 ++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 162 insertions(+), 22 deletions(-)
>
>diff --git a/source3/smbd/open.c b/source3/smbd/open.c
>index 7a0db9396f9..4b2ce4712f8 100644
>--- a/source3/smbd/open.c
>+++ b/source3/smbd/open.c
>@@ -3895,6 +3895,115 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
> 	return NT_STATUS_OK;
> }
> 
>+struct safe_pathname_state {
>+	connection_struct *conn;
>+	struct smb_filename *old_cwd;
>+};
>+
>+static NTSTATUS safe_pathname_start(TALLOC_CTX *mem_ctx,
>+			connection_struct *conn,
>+			const struct smb_filename *smb_fname_in,
>+			struct smb_filename **pp_smb_fname_out,
>+			struct safe_pathname_state **pp_safe_pathname_state)
>+{
>+	struct safe_pathname_state *state = NULL;
>+	const char *final_component = NULL;
>+	char *parent_dir = NULL;
>+	bool ok = false;
>+	bool done_chdir = false;
>+	struct smb_filename parent_dir_fname = {0};
>+	struct smb_filename *smb_fname_rel = NULL;
>+	int ret = -1;
>+	NTSTATUS status;
>+
>+	/* This can *never* be done on a stream name. */
>+	if (smb_fname_in->stream_name != NULL) {
>+		return NT_STATUS_INVALID_PARAMETER;
>+	}
>+
>+	state = talloc_zero(mem_ctx, struct safe_pathname_state);
>+	if (state == NULL) {
>+		return NT_STATUS_NO_MEMORY;
>+	}
>+
>+	state->conn = conn;
>+
>+	ok = parent_dirname(state,
>+			    smb_fname_in->base_name,
>+			    &parent_dir,
>+			    &final_component);
>+	if (!ok) {
>+		status = NT_STATUS_NO_MEMORY;
>+		goto err;
>+	}
>+
>+	state->old_cwd = vfs_GetWd(state, conn);
>+	if (state->old_cwd == NULL) {
>+		status = map_nt_error_from_unix(errno);
>+		goto err;
>+	}
>+
>+	parent_dir_fname = (struct smb_filename) { .base_name = parent_dir };
>+
>+	/* Pin parent directory in place. */
>+	ret = vfs_ChDir(conn, &parent_dir_fname);
>+	if (ret == -1) {
>+		status = map_nt_error_from_unix(errno);
>+		goto err;
>+	}
>+
>+	done_chdir = true;
>+
>+	smb_fname_rel = synthetic_smb_fname(state,
>+				final_component,
>+				NULL,
>+				&smb_fname_in->st,
>+				smb_fname_in->flags);
>+	if (smb_fname_rel == NULL) {
>+		status = NT_STATUS_NO_MEMORY;
>+		goto err;
>+        }
>+
>+	/* Ensure the relative path is below the share. */
>+	status = check_reduced_name(conn, &parent_dir_fname, smb_fname_rel);
>+	if (!NT_STATUS_IS_OK(status)) {
>+		goto err;
>+	}
>+
>+	/*
>+	 * We are safe. Exit with the new $cwd.
>+	 * safe_pathname_end() will undo this.
>+	 */
>+
>+	*pp_safe_pathname_state = state;
>+	*pp_smb_fname_out = smb_fname_rel;
>+	return NT_STATUS_OK;
>+
>+  err:
>+
>+	if (done_chdir) {
>+		ret = vfs_ChDir(conn, state->old_cwd);
>+		if (ret == -1) {
>+			smb_panic("unable to get back to old directory\n");
>+		}
>+	}
>+	TALLOC_FREE(state);
>+	return status;
>+}
>+
>+static void safe_pathname_end(struct safe_pathname_state **pp_state)
>+{
>+	int ret = -1;
>+	int saved_errno = errno;
>+	/* Chdir back */
>+	ret = vfs_ChDir((*pp_state)->conn, (*pp_state)->old_cwd);
>+	if (ret == -1) {
>+		smb_panic("unable to get back to old directory\n");
>+	}
>+	TALLOC_FREE(*pp_state);
>+	errno = saved_errno;
>+}
>+
> static NTSTATUS mkdir_internal(connection_struct *conn,
> 			       struct smb_filename *smb_dname,
> 			       uint32_t file_attributes)
>@@ -3905,6 +4014,8 @@ 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 *smb_dname_rel = NULL;
>+	struct safe_pathname_state *safe_filename_state = NULL;
> 
> 	if (!CAN_WRITE(conn) || (access_mask & ~(conn->share_access))) {
> 		DEBUG(5,("mkdir_internal: failing share access "
>@@ -3924,48 +4035,65 @@ static NTSTATUS mkdir_internal(connection_struct *conn,
> 		mode = unix_mode(conn, FILE_ATTRIBUTE_DIRECTORY, smb_dname, parent_dir);
> 	}
> 
>-	status = check_parent_access(conn,
>+	status = safe_pathname_start(talloc_tos(),
>+					conn,
> 					smb_dname,
>+					&smb_dname_rel,
>+					&safe_filename_state);
>+	if (!NT_STATUS_IS_OK(status)) {
>+		DEBUG(5,("safe_pathname_start "
>+			"on directory %s for path %s returned %s\n",
>+			parent_dir,
>+			smb_dname->base_name,
>+			nt_errstr(status) ));
>+		return status;
>+	}
>+
>+	status = check_parent_access(conn,
>+					smb_dname_rel,
> 					access_mask);
> 	if(!NT_STATUS_IS_OK(status)) {
>-		DEBUG(5,("mkdir_internal: check_parent_access "
>+		DEBUG(5,("check_parent_access "
> 			"on directory %s for path %s returned %s\n",
> 			parent_dir,
> 			smb_dname->base_name,
> 			nt_errstr(status) ));
>-		return status;
>+		goto err;
> 	}
> 
>-	if (SMB_VFS_MKDIR(conn, smb_dname, mode) != 0) {
>-		return map_nt_error_from_unix(errno);
>+	if (SMB_VFS_MKDIR(conn, smb_dname_rel, mode) != 0) {
>+		status = map_nt_error_from_unix(errno);
>+		goto err;
> 	}
> 
> 	/* Ensure we're checking for a symlink here.... */
> 	/* We don't want to get caught by a symlink racer. */
> 
>-	if (SMB_VFS_LSTAT(conn, smb_dname) == -1) {
>+	if (SMB_VFS_LSTAT(conn, smb_dname_rel) == -1) {
> 		DEBUG(2, ("Could not stat directory '%s' just created: %s\n",
> 			  smb_fname_str_dbg(smb_dname), strerror(errno)));
>-		return map_nt_error_from_unix(errno);
>+		status = map_nt_error_from_unix(errno);
>+		goto err;
> 	}
> 
>-	if (!S_ISDIR(smb_dname->st.st_ex_mode)) {
>+	if (!S_ISDIR(smb_dname_rel->st.st_ex_mode)) {
> 		DEBUG(0, ("Directory '%s' just created is not a directory !\n",
> 			  smb_fname_str_dbg(smb_dname)));
>-		return NT_STATUS_NOT_A_DIRECTORY;
>+		status = NT_STATUS_NOT_A_DIRECTORY;
>+		goto err;
> 	}
> 
> 	if (lp_store_dos_attributes(SNUM(conn))) {
> 		if (!posix_open) {
>-			file_set_dosmode(conn, smb_dname,
>+			file_set_dosmode(conn, smb_dname_rel,
> 					 file_attributes | FILE_ATTRIBUTE_DIRECTORY,
>-					 parent_dir, true);
>+					 ".", true);
> 		}
> 	}
> 
> 	if (lp_inherit_permissions(SNUM(conn))) {
>-		inherit_access_posix_acl(conn, parent_dir,
>-					 smb_dname, mode);
>+		inherit_access_posix_acl(conn, ".",
>+					 smb_dname_rel, mode);
> 		need_re_stat = true;
> 	}
> 
>@@ -3977,34 +4105,46 @@ static NTSTATUS mkdir_internal(connection_struct *conn,
> 		 * dir.
> 		 */
> 		if ((mode & ~(S_IRWXU|S_IRWXG|S_IRWXO)) &&
>-		    (mode & ~smb_dname->st.st_ex_mode)) {
>-			SMB_VFS_CHMOD(conn, smb_dname,
>-				      (smb_dname->st.st_ex_mode |
>-					  (mode & ~smb_dname->st.st_ex_mode)));
>+		    (mode & ~smb_dname_rel->st.st_ex_mode)) {
>+			SMB_VFS_CHMOD(conn, smb_dname_rel,
>+				      (smb_dname_rel->st.st_ex_mode |
>+					  (mode & ~smb_dname_rel->st.st_ex_mode)));
> 			need_re_stat = true;
> 		}
> 	}
> 
> 	/* Change the owner if required. */
> 	if (lp_inherit_owner(SNUM(conn)) != INHERIT_OWNER_NO) {
>-		change_dir_owner_to_parent(conn, parent_dir,
>-					   smb_dname,
>-					   &smb_dname->st);
>+		change_dir_owner_to_parent(conn, ".",
>+					   smb_dname_rel,
>+					   &smb_dname_rel->st);
> 		need_re_stat = true;
> 	}
> 
> 	if (need_re_stat) {
>-		if (SMB_VFS_LSTAT(conn, smb_dname) == -1) {
>+		if (SMB_VFS_LSTAT(conn, smb_dname_rel) == -1) {
>+			status = map_nt_error_from_unix(errno);
> 			DEBUG(2, ("Could not stat directory '%s' just created: %s\n",
> 			  smb_fname_str_dbg(smb_dname), strerror(errno)));
>-			return map_nt_error_from_unix(errno);
>+			goto err;
> 		}
> 	}
> 
>+	/* Ensure returned stat is up to date. */
>+	smb_dname->st = smb_dname_rel->st;
>+
>+	/* Go back to the previous $cwd. */
>+	safe_pathname_end(&safe_filename_state);
>+
> 	notify_fname(conn, NOTIFY_ACTION_ADDED, FILE_NOTIFY_CHANGE_DIR_NAME,
> 		     smb_dname->base_name);
> 
> 	return NT_STATUS_OK;
>+
>+  err:
>+
>+	safe_pathname_end(&safe_filename_state);
>+	return status;
> }
> 
> /****************************************************************************
>-- 
>2.17.1
>
>
>From 4baa6b45f516f6da768f30f8a06fdbc2a8eaf26e Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Mon, 3 Jun 2019 19:55:41 -0700
>Subject: [PATCH 3/3] WIP: Ensure open_dir_safely() returns the correct errno
> from check_path().
>
>This function will be rewritten to use safe_pathname_start()/safe_pathname_end()
>once that function is moved as a utility function into vfs.c
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/smbd/dir.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
>index f05d7a290e5..422b8f4c297 100644
>--- a/source3/smbd/dir.c
>+++ b/source3/smbd/dir.c
>@@ -1722,6 +1722,9 @@ static struct smb_Dir *open_dir_safely(TALLOC_CTX *ctx,
> 	}
> 
> 	if (vfs_ChDir(conn, smb_dname) == -1) {
>+		DBG_NOTICE("vfs_ChDir to %s returned %s\n",
>+			smb_dname->base_name,
>+			strerror(errno));
> 		goto out;
> 	}
> 
>@@ -1740,6 +1743,10 @@ static struct smb_Dir *open_dir_safely(TALLOC_CTX *ctx,
> 	 */
> 	status = check_name(conn, smb_fname_cwd);
> 	if (!NT_STATUS_IS_OK(status)) {
>+		DBG_NOTICE("check_name: %s -> '.' returned %s\n",
>+			smb_dname->base_name,
>+			nt_errstr(status));
>+		errno = map_errno_from_nt_status(status);
> 		goto out;
> 	}
> 
>@@ -1750,6 +1757,9 @@ static struct smb_Dir *open_dir_safely(TALLOC_CTX *ctx,
> 				attr);
> 
> 	if (dir_hnd == NULL) {
>+		DBG_NOTICE("OpenDir_internal %s -> '.' returned %s\n",
>+			smb_dname->base_name,
>+			strerror(errno));
> 		goto out;
> 	}
> 
>-- 
>2.17.1
>

Actions: View

Attachments on bug 13979: 15205 | 15212 | 15213 | 15214 | 15215 | 15217 | 16812 | 16815 | 16987 | 16988 | 17072