The Samba-Bugzilla – Attachment 12731 Details for
Bug 12460
rename_internals_fsp missing ACL permission-check on destination folder
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for master.
bug-12460.master (text/plain), 11.85 KB, created by
Jeremy Allison
on 2016-12-06 17:52:01 UTC
(
hide
)
Description:
git-am fix for master.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2016-12-06 17:52:01 UTC
Size:
11.85 KB
patch
obsolete
>From 9b3a347240f48a5b7445ca274ab483ee7704dc2f Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@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@gmail.com>. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=12460 > >Signed-off-by: Jeremy Allison <jra@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@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@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@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@gmail.com>. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=12460 > >Signed-off-by: Jeremy Allison <jra@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@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@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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Actions:
View
Attachments on
bug 12460
: 12731 |
12748
|
12749