From d40533311f1a281b361ca9aecfcbd46a2567a086 Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 Pair-programmed-with: Ralph Boehme Signed-off-by: Jeremy Allison Signed-off-by: Ralph Boehme Reviewed-by: Ralph Boehme (cherry picked from commit 10c3e3923022485c720f322ca4f0aca5d7501310) --- source3/smbd/open.c | 104 ++++++++++++++++++++++++++++------------------------ 1 file changed, 56 insertions(+), 48 deletions(-) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 42db659d402..5f6f048cff9 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -639,7 +639,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; @@ -671,59 +673,65 @@ 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; - } + 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_IS_OK(status)) { + if (!file_existed) { + *file_created = true; } - /* Create is done, or failed. */ - break; + return NT_STATUS_OK; } + if (!NT_STATUS_EQUAL(status, retry_status)) { + return status; + } + + curr_flags = flags; + + /* + * Keep file_existed up to date for clarity. + */ + 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); + + if (NT_STATUS_IS_OK(status) && (!file_existed)) { + *file_created = true; + } + return status; } -- 2.11.0.483.g087da7b7c-goog From ab3561b83abd86decfde6e56c7142d4f91918625 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 Pair-programmed-with: Ralph Boehme Signed-off-by: Jeremy Allison Signed-off-by: Ralph Boehme Reviewed-by: Ralph Boehme Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Thu Feb 16 22:06:51 CET 2017 on sn-devel-144 (cherry picked from commit 40d813cdb312fd8378db310543e0778193a1a684) --- 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 b72da85ec5e..c34924803da 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -84,7 +84,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 95274b1bf1b..cafc1a8c3d4 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -9497,6 +9497,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; @@ -11059,6 +11159,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