From d606a1662e797a2788e7c5b1023dbff19b976422 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 Signed-off-by: Ralph Boehme Signed-off-by: Jeremy Allison --- source3/smbd/open.c | 96 ++++++++++++++++++++++++++--------------------------- 1 file changed, 47 insertions(+), 49 deletions(-) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 931d76df44f..19ad47fc752 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -640,7 +640,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; @@ -672,59 +674,55 @@ 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; - } - } - /* Create is done, or failed. */ - break; + 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_EQUAL(status, retry_status)) { + return status; + } + + curr_flags = flags; + + /* + * Keep file_existed up to date for clarity, even + * though we don't use it again. + */ + 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); return status; } -- 2.11.0.483.g087da7b7c-goog From d265f0921727e9b7f059f7fe60532e1de5e3f4c7 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