[PATCH] Fix missing permission check on destination directory on rename.
Jeremy Allison
jra at samba.org
Tue Dec 6 17:51:05 UTC 2016
Bit of a corner case, took me a while to code
up a good regression test case, but fixes an
issue if there is a missmatch between the
underlying file system permissions and the
stored NT ACLs.
Fixes bug:
https://bugzilla.samba.org/show_bug.cgi?id=12460
Inspired by a fix from Michael Zeis <mzeis.quantum at gmail.com>.
Please review and push if happy.
Cheers,
Jeremy.
-------------- next part --------------
From 9b3a347240f48a5b7445ca274ab483ee7704dc2f Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 5 Dec 2016 14:13:14 -0800
Subject: [PATCH 1/4] s3: smbd: rename - missing early error exit if source and
destination prefixes are different.
Noticed by Michael Zeis <mzeis.quantum at gmail.com>.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12460
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/reply.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 0aec433..6ddfa4f 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -6812,6 +6812,7 @@ NTSTATUS rename_internals_fsp(connection_struct *conn,
if (rename_path_prefix_equal(fsp->fsp_name, smb_fname_dst)) {
status = NT_STATUS_ACCESS_DENIED;
+ goto out;
}
lck = get_existing_share_mode_lock(talloc_tos(), fsp->file_id);
--
2.8.0.rc3.226.g39d4020
From fa0da0375b336071bfa6fccfdf243f178f2e28d6 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 5 Dec 2016 14:32:03 -0800
Subject: [PATCH 2/4] s3: smbd: Make check_parent_access() available to rename
code.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12460
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/open.c | 2 +-
source3/smbd/proto.h | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index c6de2dc..42db659 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -235,7 +235,7 @@ NTSTATUS smbd_check_access_rights(struct connection_struct *conn,
return NT_STATUS_OK;
}
-static NTSTATUS check_parent_access(struct connection_struct *conn,
+NTSTATUS check_parent_access(struct connection_struct *conn,
struct smb_filename *smb_fname,
uint32_t access_mask)
{
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 352d28c..50ede9d 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -642,6 +642,9 @@ NTSTATUS smbd_check_access_rights(struct connection_struct *conn,
const struct smb_filename *smb_fname,
bool use_privs,
uint32_t access_mask);
+NTSTATUS check_parent_access(struct connection_struct *conn,
+ struct smb_filename *smb_fname,
+ uint32_t access_mask);
NTSTATUS fd_open(struct connection_struct *conn, files_struct *fsp,
int flags, mode_t mode);
NTSTATUS fd_close(files_struct *fsp);
--
2.8.0.rc3.226.g39d4020
From 1d14f66fcc94c1131b9f615fc1ff5bef9e7d08cf Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 5 Dec 2016 14:32:55 -0800
Subject: [PATCH 3/4] s3: smbd: Add missing permissions check on destination
folder.
Based on code from Michael Zeis <mzeis.quantum at gmail.com>.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12460
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/reply.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 6ddfa4f..6acbaca 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -6615,6 +6615,7 @@ NTSTATUS rename_internals_fsp(connection_struct *conn,
struct smb_filename *smb_fname_dst = NULL;
NTSTATUS status = NT_STATUS_OK;
struct share_mode_lock *lck = NULL;
+ uint32_t access_mask = SEC_DIR_ADD_FILE;
bool dst_exists, old_is_stream, new_is_stream;
status = check_name(conn, smb_fname_dst_in->base_name);
@@ -6815,6 +6816,22 @@ NTSTATUS rename_internals_fsp(connection_struct *conn,
goto out;
}
+ /* Do we have rights to move into the destination ? */
+ if (S_ISDIR(fsp->fsp_name->st.st_ex_mode)) {
+ /* We're moving a directory. */
+ access_mask = SEC_DIR_ADD_SUBDIR;
+ }
+ status = check_parent_access(conn,
+ smb_fname_dst,
+ access_mask);
+ if (!NT_STATUS_IS_OK(status)) {
+ DBG_INFO("check_parent_access on "
+ "dst %s returned %s\n",
+ smb_fname_str_dbg(smb_fname_dst),
+ nt_errstr(status));
+ goto out;
+ }
+
lck = get_existing_share_mode_lock(talloc_tos(), fsp->file_id);
/*
--
2.8.0.rc3.226.g39d4020
From 4753809de728a3d73d19f97d05df8742d76d2663 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 5 Dec 2016 14:34:18 -0800
Subject: [PATCH 4/4] s3: torture: Regression test case for permissions check
on rename.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12460
Signed-off-by: Jeremy Allison <jra at samba.org>
---
selftest/skip | 1 +
source3/selftest/tests.py | 5 ++
source3/torture/torture.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 198 insertions(+)
diff --git a/selftest/skip b/selftest/skip
index ebef0e8..0893962 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\).RENAME-ACCESS # Fails against the s4 ntvfs server
^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).PIDHIGH # Fails against the s4 ntvfs server
^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).NTTRANS-FSCTL # Fails against the s4 ntvfs server
^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).SMB2-NEGPROT # Fails against the s4 ntvfs server
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 7a4e2d7..a678c77 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -68,6 +68,11 @@ for t in tests:
plantestsuite("samba3.smbtorture_s3.crypt_server(nt4_dc).%s" % t, "nt4_dc", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/tmpenc', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
plantestsuite("samba3.smbtorture_s3.plain(ad_dc_ntvfs).%s" % t, "ad_dc_ntvfs", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/tmp', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
+#
+# RENAME-ACCESS needs to run against a special share - acl_xattr_ign_sysacl_windows
+#
+plantestsuite("samba3.smbtorture_s3.plain(nt4_dc).%s" % "RENAME-ACCESS","nt4_dc", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), "RENAME-ACCESS", '//$SERVER_IP/acl_xattr_ign_sysacl_windows', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
+plantestsuite("samba3.smbtorture_s3.crypt_client(nt4_dc).%s" % "RENAME-ACCESS", "nt4_dc", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), "RENAME-ACCESS", '//$SERVER_IP/acl_xattr_ign_sysacl_windows', '$USERNAME', '$PASSWORD', smbtorture3, "-e", "-l $LOCAL_PATH"])
# non-crypt only
tests = ["OPLOCK-CANCEL"]
diff --git a/source3/torture/torture.c b/source3/torture/torture.c
index c7fd5a0..ba50462 100644
--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -4895,6 +4895,197 @@ static bool run_rename(int dummy)
return correct;
}
+/*
+ Test rename into a directory with an ACL denying it.
+ */
+static bool run_rename_access(int dummy)
+{
+ static struct cli_state *cli = NULL;
+ static struct cli_state *posix_cli = NULL;
+ const char *src = "test.txt";
+ const char *dname = "dir";
+ const char *dst = "dir\\test.txt";
+ const char *dsrc = "test.dir";
+ const char *ddst = "dir\\test.dir";
+ uint16_t fnum = (uint16_t)-1;
+ struct security_descriptor *sd = NULL;
+ struct security_descriptor *newsd = NULL;
+ NTSTATUS status;
+ TALLOC_CTX *frame = NULL;
+
+ frame = talloc_stackframe();
+ printf("starting rename access test\n");
+
+ /* Windows connection. */
+ if (!torture_open_connection(&cli, 0)) {
+ goto fail;
+ }
+
+ smbXcli_conn_set_sockopt(cli->conn, sockops);
+
+ /* Posix connection. */
+ if (!torture_open_connection(&posix_cli, 0)) {
+ goto fail;
+ }
+
+ smbXcli_conn_set_sockopt(posix_cli->conn, sockops);
+
+ status = torture_setup_unix_extensions(posix_cli);
+ if (!NT_STATUS_IS_OK(status)) {
+ goto fail;
+ }
+
+ /* Start with a clean slate. */
+ cli_unlink(cli, src, FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
+ cli_unlink(cli, dst, FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
+ cli_rmdir(cli, dsrc);
+ cli_rmdir(cli, ddst);
+ cli_rmdir(cli, dname);
+
+ /*
+ * Setup the destination directory with a DENY ACE to
+ * prevent new files within it.
+ */
+ status = cli_ntcreate(cli,
+ dname,
+ 0,
+ FILE_READ_ATTRIBUTES|READ_CONTROL_ACCESS|
+ WRITE_DAC_ACCESS|FILE_READ_DATA|
+ WRITE_OWNER_ACCESS,
+ FILE_ATTRIBUTE_DIRECTORY,
+ FILE_SHARE_READ|FILE_SHARE_WRITE,
+ FILE_CREATE,
+ FILE_DIRECTORY_FILE,
+ 0,
+ &fnum,
+ NULL);
+ if (!NT_STATUS_IS_OK(status)) {
+ printf("Create of %s - %s\n", dname, nt_errstr(status));
+ goto fail;
+ }
+
+ status = cli_query_secdesc(cli,
+ fnum,
+ frame,
+ &sd);
+ if (!NT_STATUS_IS_OK(status)) {
+ printf("cli_query_secdesc failed for %s (%s)\n",
+ dname, nt_errstr(status));
+ goto fail;
+ }
+
+ newsd = security_descriptor_dacl_create(frame,
+ 0,
+ NULL,
+ NULL,
+ SID_WORLD,
+ SEC_ACE_TYPE_ACCESS_DENIED,
+ SEC_DIR_ADD_FILE|SEC_DIR_ADD_SUBDIR,
+ 0,
+ NULL);
+ if (newsd == NULL) {
+ goto fail;
+ }
+ sd->dacl = security_acl_concatenate(frame,
+ newsd->dacl,
+ sd->dacl);
+ if (sd->dacl == NULL) {
+ goto fail;
+ }
+ status = cli_set_secdesc(cli, fnum, sd);
+ if (!NT_STATUS_IS_OK(status)) {
+ printf("cli_set_secdesc failed for %s (%s)\n",
+ dname, nt_errstr(status));
+ goto fail;
+ }
+ status = cli_close(cli, fnum);
+ if (!NT_STATUS_IS_OK(status)) {
+ printf("close failed for %s (%s)\n",
+ dname, nt_errstr(status));
+ goto fail;
+ }
+ /* Now go around the back and chmod to 777 via POSIX. */
+ status = cli_posix_chmod(posix_cli, dname, 0777);
+ if (!NT_STATUS_IS_OK(status)) {
+ printf("cli_posix_chmod failed for %s (%s)\n",
+ dname, nt_errstr(status));
+ goto fail;
+ }
+
+ /* Check we can't create a file within dname via Windows. */
+ status = cli_openx(cli, dst, O_RDWR|O_CREAT|O_EXCL, DENY_NONE, &fnum);
+ if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
+ cli_close(posix_cli, fnum);
+ printf("Create of %s should be ACCESS denied, was %s\n",
+ dst, nt_errstr(status));
+ goto fail;
+ }
+
+ /* Make the sample file/directory. */
+ status = cli_openx(cli, src, O_RDWR|O_CREAT|O_EXCL, DENY_NONE, &fnum);
+ if (!NT_STATUS_IS_OK(status)) {
+ printf("open of %s failed (%s)\n", src, nt_errstr(status));
+ goto fail;
+ }
+ status = cli_close(cli, fnum);
+ if (!NT_STATUS_IS_OK(status)) {
+ printf("cli_close failed (%s)\n", nt_errstr(status));
+ goto fail;
+ }
+
+ status = cli_mkdir(cli, dsrc);
+ if (!NT_STATUS_IS_OK(status)) {
+ printf("cli_mkdir of %s failed (%s)\n",
+ dsrc, nt_errstr(status));
+ goto fail;
+ }
+
+ /*
+ * OK - renames of the new file and directory into the
+ * dst directory should fail.
+ */
+
+ status = cli_rename(cli, src, dst);
+ if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
+ printf("rename of %s -> %s should be ACCESS denied, was %s\n",
+ src, dst, nt_errstr(status));
+ goto fail;
+ }
+ status = cli_rename(cli, dsrc, ddst);
+ if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
+ printf("rename of %s -> %s should be ACCESS denied, was %s\n",
+ src, dst, nt_errstr(status));
+ goto fail;
+ }
+
+ TALLOC_FREE(frame);
+ return true;
+
+ fail:
+
+ if (posix_cli) {
+ torture_close_connection(posix_cli);
+ }
+
+ if (cli) {
+ if (fnum != -1) {
+ cli_close(cli, fnum);
+ }
+ cli_unlink(cli, src,
+ FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
+ cli_unlink(cli, dst,
+ FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
+ cli_rmdir(cli, dsrc);
+ cli_rmdir(cli, ddst);
+ cli_rmdir(cli, dname);
+
+ torture_close_connection(cli);
+ }
+
+ TALLOC_FREE(frame);
+ return false;
+}
+
static bool run_pipe_number(int dummy)
{
struct cli_state *cli1;
@@ -10453,6 +10644,7 @@ static struct {
#endif
{"XCOPY", run_xcopy, 0},
{"RENAME", run_rename, 0},
+ {"RENAME-ACCESS", run_rename_access, 0},
{"DELETE", run_deletetest, 0},
{"WILDDELETE", run_wild_deletetest, 0},
{"DELETE-LN", run_deletetest_ln, 0},
--
2.8.0.rc3.226.g39d4020
More information about the samba-technical
mailing list