[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
Wed Feb 15 17:43:30 UTC 2017
Hi Ralph,
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 !
Cheers,
Jeremy.
-------------- next part --------------
From 8db34e9e83f9f22380bd8e8f8b836b7347a78817 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 13 Feb 2017 14:12:48 -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 must use an even number as the
terminating loop value as we think the file
already exists and must return
NT_STATUS_OBJECT_NAME_NOT_FOUND for both the
file got deleted and bad symlink case rather
than EEXIST -> NT_STATUS_OBJECT_NAME_COLLISION.
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
terminate the loop.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/open.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 931d76df44f..f98bca0f5d5 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -641,6 +641,7 @@ static NTSTATUS fd_open_atomic(struct connection_struct *conn,
{
NTSTATUS status = NT_STATUS_UNSUCCESSFUL;
bool file_existed = VALID_STAT(fsp->fsp_name->st);
+ unsigned int loop_counter = 0;
*file_created = false;
@@ -672,13 +673,29 @@ 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 must use an even number as the
+ * terminating loop value as we think the file
+ * already exists and must return
+ * NT_STATUS_OBJECT_NAME_NOT_FOUND for both the
+ * file got deleted and bad symlink case rather
+ * than NT_STATUS_OBJECT_NAME_COLLISION mapped from
+ * POSIX EEXIST.
+ *
+ * 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
+ * terminate the loop.
*/
- while(1) {
+ for(loop_counter = 0; loop_counter < 2; loop_counter++) {
int curr_flags = flags;
if (file_existed) {
--
2.11.0.483.g087da7b7c-goog
From 92265a54f5ecf7ca3dd44c0c546cdf749a2f0014 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