[PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT
Jeremy Allison
jra at samba.org
Thu Feb 16 16:57:33 UTC 2017
On Thu, Feb 16, 2017 at 11:55:56AM +0100, Ralph Böhme wrote:
>
> damn, it fails in raw.open, I told you it was my first take on it. :)
>
> We forgot to set file_created for the caller. The attached version now passes
> raw.open.
Ah, that's better - thanks ! Pushed with a slight comment
change ("even though we don't use it again." deleted as
we *DO* use it again :-).
Thanks for your help on this !
Jeremy.
> From 686e4e132c07a1c0066f4120038deb85dbc94757 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> 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 <slow at samba.org>
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> Reviewed-by: Ralph Boehme <slow at samba.org>
> ---
> source3/smbd/open.c | 105 ++++++++++++++++++++++++++++------------------------
> 1 file changed, 57 insertions(+), 48 deletions(-)
>
> diff --git a/source3/smbd/open.c b/source3/smbd/open.c
> index 931d76d..8733e47 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,66 @@ 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, 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);
> +
> + if (NT_STATUS_IS_OK(status) && (!file_existed)) {
> + *file_created = true;
> + }
> +
> return status;
> }
>
> --
> 2.9.3
>
>
> From 9a5d44818c8092b783274fd57588dc1065ebd27a Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at 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
>
> Pair-programmed-with: Ralph Boehme <slow at samba.org>
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> Reviewed-by: Ralph Boehme <slow at 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 1e6d311..fd9932a 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 8e1c33d..4215eb0 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 1bd7d6e..846b675 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.9.3
>
More information about the samba-technical
mailing list