From af8503a4a28097fb9c943492b6faac97913ce844 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 13 Feb 2017 14:12:48 -0800 Subject: [PATCH 1/2] s3: smbd: Don't loop infinitely on bad-symlink resolution. If we think the file didn't exist, try the open with O_CREAT|O_EXCL. Alternate between these two requests until either the file is created, or opened or we get an error. Try 2 times before giving up and returning the last error we got. The big problem are symlinks here. Opening without O_NOFOLLOW means both bad symlink and missing path return -1, ENOENT. As POSIX is pathname based it's not possible to tell the difference between these two cases in a non-racy way, so just try 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 --- source3/smbd/open.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 931d76df44f..51bf4ede689 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,32 @@ 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 + * O_CREAT|O_EXCL. Alternate 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). + * opened or we get an error. Try 2 times before giving + * up and returning the last error we got. + * + * The big problem are symlinks here. Opening + * without O_NOFOLLOW means both bad symlink + * and missing path return -1, ENOENT. As POSIX + * is pathname based it's not possible to tell + * the difference between these two cases in a + * non-racy way, so just try 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 59bc8233ea0404e1f3d1cc893241680392e39e4a Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 --- 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