The Samba-Bugzilla – Attachment 12929 Details for
Bug 12572
fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Better comment and commit message.
12572 (text/plain), 8.66 KB, created by
Jeremy Allison
on 2017-02-15 01:44:41 UTC
(
hide
)
Description:
Better comment and commit message.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2017-02-15 01:44:41 UTC
Size:
8.66 KB
patch
obsolete
>From aca3ba9755efe411fbdf7436eed1b52fd4e85e3d Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@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. > >If eventually we restructure the code to always use >O_NOFOLLOW then we'll be safe to go back >to the non-counted loop case. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/smbd/open.c | 31 ++++++++++++++++++++++++++----- > 1 file changed, 26 insertions(+), 5 deletions(-) > >diff --git a/source3/smbd/open.c b/source3/smbd/open.c >index 931d76df44f..733bb5582f3 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,33 @@ 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. >+ * >+ * If eventually we restructure the code to always use >+ * O_NOFOLLOW then we'll be safe to go back >+ * to the non-counted loop case. > */ > >- 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 91f5e394af316952acfa58557af032ca6ce37937 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@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@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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Actions:
View
Attachments on
bug 12572
:
12925
|
12928
|
12929
|
12942
|
12946
|
12948
|
12949