[PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT

Jeremy Allison
Hi Ralph,

Took a lot of thinking about but here is the
patch I'm happy with to fix:

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572

plus a regression test to ensure it stays fixed :-).

Details can be found in the commit and code comments.

Please review and push if happy !

Cheers,

        Jeremy.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT

Ralph Böhme-2
Hi Jeremy,

On Wed, Feb 15, 2017 at 09:43:30AM -0800, Jeremy Allison wrote:

> Took a lot of thinking about but here is the
> patch I'm happy with to fix:
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572
>
> plus a regression test to ensure it stays fixed :-).
>
> Details can be found in the commit and code comments.
>
> Please review and push if happy !
as we'll be limitting the loop to two iterations maybe we could break it up into
two simple subsequent calls? Attached is my first attempt at that, passes your
new test and a "make test TESTS=samba3.smbtorture_s3.plain". What do you think?

I'll need a few more passes over it, not 100% sure the logic is still the same,
this is just what I came up with in the first attempt.

Cheerio!
-slow
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT

Jeremy Allison
On Wed, Feb 15, 2017 at 10:14:38PM +0100, Ralph Böhme wrote:

> Hi Jeremy,
>
> On Wed, Feb 15, 2017 at 09:43:30AM -0800, Jeremy Allison wrote:
> > Took a lot of thinking about but here is the
> > patch I'm happy with to fix:
> >
> > BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572
> >
> > plus a regression test to ensure it stays fixed :-).
> >
> > Details can be found in the commit and code comments.
> >
> > Please review and push if happy !
>
> as we'll be limitting the loop to two iterations maybe we could break it up into
> two simple subsequent calls? Attached is my first attempt at that, passes your
> new test and a "make test TESTS=samba3.smbtorture_s3.plain". What do you think?

I actually thought about doing that, but that's a slighly
bigger change than I wanted (as you've discovered, the
logic here needs careful examination :-).

In addition, if we move to always using O_NOFOLLOW, as
we should probably do in later code improvements, then
it will be easy to put the while(1) loop back in place
to completely eliminate the race again.

> I'll need a few more passes over it, not 100% sure the logic is still the same,
> this is just what I came up with in the first attempt.

I'll take a look over your change also, one concern
is the original code flips the state of bool file_existed
to match the return from the open, so it's always representing
the current state on disk.

As your code doesn't do that, it makes the following
logic correct :

 +     if (!file_existed) {
 +             curr_flags &= ~(O_CREAT);
 +     } else {
 +             curr_flags |= O_EXCL;
 +     }

but strange looking. (If !file_existed, remove O_CREAT - what ????)
as you need to remember it's referring to the initial state,
not the current state after the retry_status match.

I think I prefer my change for now.

Jeremy.

> From 76313894b4a2ae1d8df246e692a8011e974faccb Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <[hidden email]>
> 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 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
>
> WIP-by: Ralph Boehme <[hidden email]>
> ---
>  source3/smbd/open.c | 88 ++++++++++++++++++++++++-----------------------------
>  1 file changed, 39 insertions(+), 49 deletions(-)
>
> diff --git a/source3/smbd/open.c b/source3/smbd/open.c
> index 931d76d..7b651ca 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,47 @@ 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 we just return NT_STATUS_OBJECT_PATH_NOT_FOUND
> + * mapped from the ELOOP POSIX error and immediately
> + * terminate the loop.
>   */
>  
> - 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;
> +
> + if (!file_existed) {
> + curr_flags &= ~(O_CREAT);
> + } else {
> + curr_flags |= O_EXCL;
> + }
> +
> + status = fd_open(conn, fsp, curr_flags, mode);
> +
>   return status;
>  }
>  
> --
> 2.9.3
>
>
> From 2f11cba4a07d9bfce69ff62b6d5c65c752e20d95 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <[hidden email]>
> 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 <[hidden email]>
> ---
>  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..7e5198d 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
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT

Jeremy Allison
In reply to this post by Ralph Böhme-2
On Wed, Feb 15, 2017 at 10:14:38PM +0100, Ralph Böhme wrote:

> Hi Jeremy,
>
> On Wed, Feb 15, 2017 at 09:43:30AM -0800, Jeremy Allison wrote:
> > Took a lot of thinking about but here is the
> > patch I'm happy with to fix:
> >
> > BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572
> >
> > plus a regression test to ensure it stays fixed :-).
> >
> > Details can be found in the commit and code comments.
> >
> > Please review and push if happy !
>
> as we'll be limitting the loop to two iterations maybe we could break it up into
> two simple subsequent calls? Attached is my first attempt at that, passes your
> new test and a "make test TESTS=samba3.smbtorture_s3.plain". What do you think?
>
> I'll need a few more passes over it, not 100% sure the logic is still the same,
> this is just what I came up with in the first attempt.
OK, did some more thinking about the O_NOFOLLOW case and
it's we're probably not going to be able to put the while(1)
loop back anyway due to the few cases where we need to follow
the symlink server-side. So how about this - based on your
code (so dual ownership :-) that unrolls the loop but
also (IMHO) makes the logic clearer w.r.t. the
file_existed variable.

Feel free to push if happy !

Jeremy.

12572 (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT

Ralph Böhme-2
On Wed, Feb 15, 2017 at 04:15:48PM -0800, Jeremy Allison wrote:

> On Wed, Feb 15, 2017 at 10:14:38PM +0100, Ralph Böhme wrote:
> > Hi Jeremy,
> >
> > On Wed, Feb 15, 2017 at 09:43:30AM -0800, Jeremy Allison wrote:
> > > Took a lot of thinking about but here is the
> > > patch I'm happy with to fix:
> > >
> > > BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572
> > >
> > > plus a regression test to ensure it stays fixed :-).
> > >
> > > Details can be found in the commit and code comments.
> > >
> > > Please review and push if happy !
> >
> > as we'll be limitting the loop to two iterations maybe we could break it up into
> > two simple subsequent calls? Attached is my first attempt at that, passes your
> > new test and a "make test TESTS=samba3.smbtorture_s3.plain". What do you think?
> >
> > I'll need a few more passes over it, not 100% sure the logic is still the same,
> > this is just what I came up with in the first attempt.
>
> OK, did some more thinking about the O_NOFOLLOW case and
> it's we're probably not going to be able to put the while(1)
> loop back anyway due to the few cases where we need to follow
> the symlink server-side. So how about this - based on your
> code (so dual ownership :-) that unrolls the loop but
> also (IMHO) makes the logic clearer w.r.t. the
> file_existed variable.
>
> Feel free to push if happy !

happy with a minor indentation fix plus correct multiline if statement wrapping:

--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -9708,8 +9708,8 @@ static bool run_symlink_open_test(int dummy)
         * 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)) {
+           NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_PATH_NOT_FOUND))
+       {
                correct = true;
        } else {
                printf("cli_ntcreate of %s returned %s - should return"

Otherwise lgtm, pushed. This is a really nice improvement of fd_open_atomic()
imho.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT

Ralph Böhme-2
On Thu, Feb 16, 2017 at 10:21:35AM +0100, Ralph Böhme wrote:

> On Wed, Feb 15, 2017 at 04:15:48PM -0800, Jeremy Allison wrote:
> > On Wed, Feb 15, 2017 at 10:14:38PM +0100, Ralph Böhme wrote:
> > > Hi Jeremy,
> > >
> > > On Wed, Feb 15, 2017 at 09:43:30AM -0800, Jeremy Allison wrote:
> > > > Took a lot of thinking about but here is the
> > > > patch I'm happy with to fix:
> > > >
> > > > BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572
> > > >
> > > > plus a regression test to ensure it stays fixed :-).
> > > >
> > > > Details can be found in the commit and code comments.
> > > >
> > > > Please review and push if happy !
> > >
> > > as we'll be limitting the loop to two iterations maybe we could break it up into
> > > two simple subsequent calls? Attached is my first attempt at that, passes your
> > > new test and a "make test TESTS=samba3.smbtorture_s3.plain". What do you think?
> > >
> > > I'll need a few more passes over it, not 100% sure the logic is still the same,
> > > this is just what I came up with in the first attempt.
> >
> > OK, did some more thinking about the O_NOFOLLOW case and
> > it's we're probably not going to be able to put the while(1)
> > loop back anyway due to the few cases where we need to follow
> > the symlink server-side. So how about this - based on your
> > code (so dual ownership :-) that unrolls the loop but
> > also (IMHO) makes the logic clearer w.r.t. the
> > file_existed variable.
> >
> > Feel free to push if happy !
>
> happy with a minor indentation fix plus correct multiline if statement wrapping:
>
> --- a/source3/torture/torture.c
> +++ b/source3/torture/torture.c
> @@ -9708,8 +9708,8 @@ static bool run_symlink_open_test(int dummy)
>          * 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)) {
> +           NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_PATH_NOT_FOUND))
> +       {
>                 correct = true;
>         } else {
>                 printf("cli_ntcreate of %s returned %s - should return"
>
> Otherwise lgtm, pushed. This is a really nice improvement of fd_open_atomic()
> imho.
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.

Cheerio!
-slow

bug12572-master.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT

Jeremy Allison
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 <[hidden email]>
> 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 <[hidden email]>
>
> Signed-off-by: Jeremy Allison <[hidden email]>
> Signed-off-by: Ralph Boehme <[hidden email]>
> Reviewed-by: Ralph Boehme <[hidden email]>
> ---
>  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 <[hidden email]>
> 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 <[hidden email]>
>
> Signed-off-by: Jeremy Allison <[hidden email]>
> Signed-off-by: Ralph Boehme <[hidden email]>
> Reviewed-by: Ralph Boehme <[hidden email]>
> ---
>  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
>