[PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT
Ralph Böhme
slow at samba.org
Thu Feb 16 10:55:56 UTC 2017
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
-------------- next part --------------
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