From aca3ba9755efe411fbdf7436eed1b52fd4e85e3d 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. 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 --- 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 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