[PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT

Ralph Böhme slow at samba.org
Thu Feb 16 10:55:56 UTC 2017


On Thu, Feb 16, 2017 at 10:21:35AM +0100, Ralph Böhme wrote:
> On Wed, Feb 15, 2017 at 04:15:48PM -0800, Jeremy Allison wrote:
> > On Wed, Feb 15, 2017 at 10:14:38PM +0100, Ralph Böhme wrote:
> > > Hi Jeremy,
> > > 
> > > On Wed, Feb 15, 2017 at 09:43:30AM -0800, Jeremy Allison wrote:
> > > > Took a lot of thinking about but here is the
> > > > patch I'm happy with to fix:
> > > > 
> > > > BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572
> > > > 
> > > > plus a regression test to ensure it stays fixed :-).
> > > > 
> > > > Details can be found in the commit and code comments.
> > > > 
> > > > Please review and push if happy !
> > > 
> > > as we'll be limitting the loop to two iterations maybe we could break it up into
> > > two simple subsequent calls? Attached is my first attempt at that, passes your
> > > new test and a "make test TESTS=samba3.smbtorture_s3.plain". What do you think?
> > > 
> > > I'll need a few more passes over it, not 100% sure the logic is still the same,
> > > this is just what I came up with in the first attempt.
> > 
> > OK, did some more thinking about the O_NOFOLLOW case and
> > it's we're probably not going to be able to put the while(1)
> > loop back anyway due to the few cases where we need to follow
> > the symlink server-side. So how about this - based on your
> > code (so dual ownership :-) that unrolls the loop but
> > also (IMHO) makes the logic clearer w.r.t. the
> > file_existed variable.
> > 
> > Feel free to push if happy !
> 
> happy with a minor indentation fix plus correct multiline if statement wrapping:
> 
> --- a/source3/torture/torture.c
> +++ b/source3/torture/torture.c
> @@ -9708,8 +9708,8 @@ static bool run_symlink_open_test(int dummy)
>          * we use O_NOFOLLOW on the server or not.
>          */
>         if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND) ||
> -                       NT_STATUS_EQUAL(status,
> -                               NT_STATUS_OBJECT_PATH_NOT_FOUND)) {
> +           NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_PATH_NOT_FOUND))
> +       {
>                 correct = true;
>         } else {
>                 printf("cli_ntcreate of %s returned %s - should return"
> 
> Otherwise lgtm, pushed. This is a really nice improvement of fd_open_atomic()
> imho.

damn, it fails in raw.open, I told you it was my first take on it. :)

We forgot to set file_created for the caller. The attached version now passes
raw.open.

Cheerio!
-slow
-------------- next part --------------
From 686e4e132c07a1c0066f4120038deb85dbc94757 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 15 Feb 2017 15:42:52 -0800
Subject: [PATCH 1/2] s3: smbd: Don't loop infinitely on bad-symlink
 resolution.

In the FILE_OPEN_IF case we have O_CREAT, but not
O_EXCL. Previously we went into a loop trying first
~(O_CREAT|O_EXCL), and if that returned ENOENT
try (O_CREAT|O_EXCL). We kept looping indefinately
until we got an error, or the file was created or
opened.

The big problem here is dangling symlinks. Opening
without O_NOFOLLOW means both bad symlink
and missing path return -1, ENOENT from open(). As POSIX
is pathname based it's not possible to tell
the difference between these two cases in a
non-racy way, so change to try only two attempts before
giving up.

We don't have this problem for the O_NOFOLLOW
case as we just return NT_STATUS_OBJECT_PATH_NOT_FOUND
mapped from the ELOOP POSIX error and immediately
returned.

Unroll the loop logic to two tries instead.

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

Pair-programmed-with: Ralph Boehme <slow at samba.org>

Signed-off-by: Jeremy Allison <jra at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 source3/smbd/open.c | 105 ++++++++++++++++++++++++++++------------------------
 1 file changed, 57 insertions(+), 48 deletions(-)

diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 931d76d..8733e47 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -640,7 +640,9 @@ static NTSTATUS fd_open_atomic(struct connection_struct *conn,
 			bool *file_created)
 {
 	NTSTATUS status = NT_STATUS_UNSUCCESSFUL;
+	NTSTATUS retry_status;
 	bool file_existed = VALID_STAT(fsp->fsp_name->st);
+	int curr_flags;
 
 	*file_created = false;
 
@@ -672,59 +674,66 @@ static NTSTATUS fd_open_atomic(struct connection_struct *conn,
 	 * we can never call O_CREAT without O_EXCL. So if
 	 * we think the file existed, try without O_CREAT|O_EXCL.
 	 * If we think the file didn't exist, try with
-	 * O_CREAT|O_EXCL. Keep bouncing between these two
-	 * requests until either the file is created, or
-	 * opened. Either way, we keep going until we get
-	 * a returnable result (error, or open/create).
+	 * O_CREAT|O_EXCL.
+	 *
+	 * The big problem here is dangling symlinks. Opening
+	 * without O_NOFOLLOW means both bad symlink
+	 * and missing path return -1, ENOENT from open(). As POSIX
+	 * is pathname based it's not possible to tell
+	 * the difference between these two cases in a
+	 * non-racy way, so change to try only two attempts before
+	 * giving up.
+	 *
+	 * We don't have this problem for the O_NOFOLLOW
+	 * case as it just returns NT_STATUS_OBJECT_PATH_NOT_FOUND
+	 * mapped from the ELOOP POSIX error.
 	 */
 
-	while(1) {
-		int curr_flags = flags;
+	curr_flags = flags;
 
-		if (file_existed) {
-			/* Just try open, do not create. */
-			curr_flags &= ~(O_CREAT);
-			status = fd_open(conn, fsp, curr_flags, mode);
-			if (NT_STATUS_EQUAL(status,
-					NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
-				/*
-				 * Someone deleted it in the meantime.
-				 * Retry with O_EXCL.
-				 */
-				file_existed = false;
-				DEBUG(10,("fd_open_atomic: file %s existed. "
-					"Retry.\n",
-					smb_fname_str_dbg(fsp->fsp_name)));
-					continue;
-			}
-		} else {
-			/* Try create exclusively, fail if it exists. */
-			curr_flags |= O_EXCL;
-			status = fd_open(conn, fsp, curr_flags, mode);
-			if (NT_STATUS_EQUAL(status,
-					NT_STATUS_OBJECT_NAME_COLLISION)) {
-				/*
-				 * Someone created it in the meantime.
-				 * Retry without O_CREAT.
-				 */
-				file_existed = true;
-				DEBUG(10,("fd_open_atomic: file %s "
-					"did not exist. Retry.\n",
-					smb_fname_str_dbg(fsp->fsp_name)));
-				continue;
-			}
-			if (NT_STATUS_IS_OK(status)) {
-				/*
-				 * Here we've opened with O_CREAT|O_EXCL
-				 * and got success. We *know* we created
-				 * this file.
-				 */
-				*file_created = true;
-			}
+	if (file_existed) {
+		curr_flags &= ~(O_CREAT);
+		retry_status = NT_STATUS_OBJECT_NAME_NOT_FOUND;
+	} else {
+		curr_flags |= O_EXCL;
+		retry_status = NT_STATUS_OBJECT_NAME_COLLISION;
+	}
+
+	status = fd_open(conn, fsp, curr_flags, mode);
+	if (NT_STATUS_IS_OK(status)) {
+		if (!file_existed) {
+			*file_created = true;
 		}
-		/* Create is done, or failed. */
-		break;
+		return NT_STATUS_OK;
 	}
+	if (!NT_STATUS_EQUAL(status, retry_status)) {
+		return status;
+	}
+
+	curr_flags = flags;
+
+	/*
+	 * Keep file_existed up to date for clarity, even
+	 * though we don't use it again.
+	 */
+	if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
+		file_existed = false;
+		curr_flags |= O_EXCL;
+		DBG_DEBUG("file %s did not exist. Retry.\n",
+			smb_fname_str_dbg(fsp->fsp_name));
+	} else {
+		file_existed = true;
+		curr_flags &= ~(O_CREAT);
+		DBG_DEBUG("file %s existed. Retry.\n",
+			smb_fname_str_dbg(fsp->fsp_name));
+	}
+
+	status = fd_open(conn, fsp, curr_flags, mode);
+
+	if (NT_STATUS_IS_OK(status) && (!file_existed)) {
+		*file_created = true;
+	}
+
 	return status;
 }
 
-- 
2.9.3


From 9a5d44818c8092b783274fd57588dc1065ebd27a Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 14 Feb 2017 12:59:58 -0800
Subject: [PATCH 2/2] s3: torture: Regression test for smbd trying to open an
 invalid symlink.

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

Pair-programmed-with: Ralph Boehme <slow at samba.org>

Signed-off-by: Jeremy Allison <jra at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 selftest/skip             |   1 +
 source3/selftest/tests.py |   2 +-
 source3/torture/torture.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/selftest/skip b/selftest/skip
index 1e6d311..fd9932a 100644
--- a/selftest/skip
+++ b/selftest/skip
@@ -48,6 +48,7 @@
 ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).POSIX-SYMLINK-EA # Fails against the s4 ntvfs server
 ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).POSIX-OFD-LOCK # Fails against the s4 ntvfs server
 ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).POSIX-STREAM-DELETE # Fails against the s4 ntvfs server
+^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).WINDOWS-BAD-SYMLINK # Fails against the s4 ntvfs server
 ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).RENAME-ACCESS # Fails against the s4 ntvfs server
 ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).OWNER-RIGHTS # Don't test against the s4 ntvfs server anymore
 ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).PIDHIGH # Fails against the s4 ntvfs server
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 8e1c33d..4215eb0 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -89,7 +89,7 @@ for t in tests:
     plantestsuite("samba3.smbtorture_s3.vfs_aio_fork(simpleserver).%s" % t, "simpleserver", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/vfs_aio_fork', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
 
 posix_tests = ["POSIX", "POSIX-APPEND", "POSIX-SYMLINK-ACL", "POSIX-SYMLINK-EA", "POSIX-OFD-LOCK",
-              "POSIX-STREAM-DELETE" ]
+              "POSIX-STREAM-DELETE", "WINDOWS-BAD-SYMLINK" ]
 
 for t in posix_tests:
     plantestsuite("samba3.smbtorture_s3.plain(nt4_dc).%s" % t, "nt4_dc", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/posix_share', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
diff --git a/source3/torture/torture.c b/source3/torture/torture.c
index 1bd7d6e..846b675 100644
--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -9643,6 +9643,106 @@ static bool run_pidhigh(int dummy)
 	return success;
 }
 
+/*
+  Test Windows open on a bad POSIX symlink.
+ */
+static bool run_symlink_open_test(int dummy)
+{
+	static struct cli_state *cli;
+	const char *fname = "non_existant_file";
+	const char *sname = "dangling_symlink";
+	uint16_t fnum = (uint16_t)-1;
+	bool correct = false;
+	NTSTATUS status;
+	TALLOC_CTX *frame = NULL;
+
+	frame = talloc_stackframe();
+
+	printf("Starting Windows bad symlink open test\n");
+
+	if (!torture_open_connection(&cli, 0)) {
+		TALLOC_FREE(frame);
+		return false;
+	}
+
+	smbXcli_conn_set_sockopt(cli->conn, sockops);
+
+	status = torture_setup_unix_extensions(cli);
+	if (!NT_STATUS_IS_OK(status)) {
+		TALLOC_FREE(frame);
+		return false;
+	}
+
+	/* Ensure nothing exists. */
+	cli_setatr(cli, fname, 0, 0);
+	cli_posix_unlink(cli, fname);
+	cli_setatr(cli, sname, 0, 0);
+	cli_posix_unlink(cli, sname);
+
+	/* Create a symlink pointing nowhere. */
+	status = cli_posix_symlink(cli, fname, sname);
+	if (!NT_STATUS_IS_OK(status)) {
+		printf("cli_posix_symlink of %s -> %s failed (%s)\n",
+			sname,
+			fname,
+			nt_errstr(status));
+		goto out;
+	}
+
+	/* Now ensure that a Windows open doesn't hang. */
+	status = cli_ntcreate(cli,
+			sname,
+			0,
+			FILE_READ_DATA|FILE_WRITE_DATA,
+			0,
+			FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE,
+			FILE_OPEN_IF,
+			0x0,
+			0x0,
+			&fnum,
+			NULL);
+
+	/*
+	 * We get either NT_STATUS_OBJECT_NAME_NOT_FOUND or
+	 * NT_STATUS_OBJECT_PATH_NOT_FOUND depending on if
+	 * we use O_NOFOLLOW on the server or not.
+	 */
+	if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND) ||
+	    NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_PATH_NOT_FOUND))
+	{
+		correct = true;
+	} else {
+		printf("cli_ntcreate of %s returned %s - should return"
+				" either (%s) or (%s)\n",
+			sname,
+			nt_errstr(status),
+			nt_errstr(NT_STATUS_OBJECT_NAME_NOT_FOUND),
+			nt_errstr(NT_STATUS_OBJECT_PATH_NOT_FOUND));
+		goto out;
+	}
+
+	correct = true;
+
+  out:
+
+	if (fnum != (uint16_t)-1) {
+		cli_close(cli, fnum);
+		fnum = (uint16_t)-1;
+	}
+
+	cli_setatr(cli, sname, 0, 0);
+	cli_posix_unlink(cli, sname);
+	cli_setatr(cli, fname, 0, 0);
+	cli_posix_unlink(cli, fname);
+
+	if (!torture_close_connection(cli)) {
+		correct = false;
+	}
+
+	TALLOC_FREE(frame);
+	return correct;
+}
+
 static bool run_local_substitute(int dummy)
 {
 	bool ok = true;
@@ -11205,6 +11305,7 @@ static struct {
 	{"POSIX-SYMLINK-EA", run_ea_symlink_test, 0},
 	{"POSIX-STREAM-DELETE", run_posix_stream_delete, 0},
 	{"POSIX-OFD-LOCK", run_posix_ofd_lock_test, 0},
+	{"WINDOWS-BAD-SYMLINK", run_symlink_open_test, 0},
 	{"CASE-INSENSITIVE-CREATE", run_case_insensitive_create, 0},
 	{"ASYNC-ECHO", run_async_echo, 0},
 	{ "UID-REGRESSION-TEST", run_uid_regression_test, 0},
-- 
2.9.3



More information about the samba-technical mailing list