[PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT
Jeremy Allison
jra at samba.org
Thu Feb 16 00:15:48 UTC 2017
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 !
Jeremy.
-------------- next part --------------
From d606a1662e797a2788e7c5b1023dbff19b976422 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
Signed-off-by: Ralph Boehme <slow at samba.org>
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/open.c | 96 ++++++++++++++++++++++++++---------------------------
1 file changed, 47 insertions(+), 49 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 931d76df44f..19ad47fc752 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,55 @@ 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;
- }
- }
- /* Create is done, or failed. */
- break;
+ 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_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);
return status;
}
--
2.11.0.483.g087da7b7c-goog
From d265f0921727e9b7f059f7fe60532e1de5e3f4c7 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
Signed-off-by: Jeremy Allison <jra 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 1e6d311f3c1..fd9932a47c0 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 8e1c33d66f0..4215eb043bf 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 1bd7d6e1393..7e5198dc117 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.11.0.483.g087da7b7c-goog
More information about the samba-technical
mailing list