The Samba-Bugzilla – Attachment 14908 Details for
Bug 13812
access_check_max_allowed() doesn't process "Owner Rights" ACEs
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch for 4.8, 4.9 and 4.10 cherry-picked from master
bug13812-v48,v49,v410.patch (text/plain), 39.66 KB, created by
Ralph Böhme
on 2019-03-07 08:52:52 UTC
(
hide
)
Description:
Patch for 4.8, 4.9 and 4.10 cherry-picked from master
Filename:
MIME Type:
Creator:
Ralph Böhme
Created:
2019-03-07 08:52:52 UTC
Size:
39.66 KB
patch
obsolete
>From ab9445a0bb7169cd2945d734429f61b28c84baaa Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Thu, 28 Feb 2019 14:47:18 +0100 >Subject: [PATCH 1/9] s4:libcli: remember return code from maximum access > >Bug: https://bugzilla.samba.org/show_bug.cgi?id=13812 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: David Disseldorp <ddiss@samba.org> >(cherry picked from commit 9f4ee05295827c9a607e1f63694a17906f777176) >--- > source4/libcli/raw/interfaces.h | 1 + > source4/libcli/smb2/create.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > >diff --git a/source4/libcli/raw/interfaces.h b/source4/libcli/raw/interfaces.h >index 732ba1512dc..43a53f834df 100644 >--- a/source4/libcli/raw/interfaces.h >+++ b/source4/libcli/raw/interfaces.h >@@ -1779,6 +1779,7 @@ union smb_open { > /* uint32_t blob_size; */ > > /* optional return values matching tagged values in the call */ >+ uint32_t maximal_access_status; > uint32_t maximal_access; > uint8_t on_disk_id[32]; > struct smb2_lease lease_response; >diff --git a/source4/libcli/smb2/create.c b/source4/libcli/smb2/create.c >index 550069a6cea..eb0f6a421cd 100644 >--- a/source4/libcli/smb2/create.c >+++ b/source4/libcli/smb2/create.c >@@ -360,12 +360,12 @@ NTSTATUS smb2_create_recv(struct smb2_request *req, TALLOC_CTX *mem_ctx, struct > /* pull out the parsed blobs */ > for (i=0;i<io->out.blobs.num_blobs;i++) { > if (strcmp(io->out.blobs.blobs[i].tag, SMB2_CREATE_TAG_MXAC) == 0) { >- /* TODO: this also contains a status field in >- first 4 bytes */ > if (io->out.blobs.blobs[i].data.length != 8) { > smb2_request_destroy(req); > return NT_STATUS_INVALID_NETWORK_RESPONSE; > } >+ io->out.maximal_access_status = >+ IVAL(io->out.blobs.blobs[i].data.data, 0); > io->out.maximal_access = IVAL(io->out.blobs.blobs[i].data.data, 4); > } > if (strcmp(io->out.blobs.blobs[i].tag, SMB2_CREATE_TAG_QFID) == 0) { >-- >2.17.2 > > >From e69ddadbfe07c6f7630c1e6109aa30c8139d2bf9 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Thu, 28 Feb 2019 14:48:02 +0100 >Subject: [PATCH 2/9] s4:torture: add a Maximum Access check with an Owner > Rights ACE > >Bug: https://bugzilla.samba.org/show_bug.cgi?id=13812 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: David Disseldorp <ddiss@samba.org> >(cherry picked from commit 3ca38d2cd1189a5040e13ddab016063280be2b4d) >--- > selftest/knownfail.d/smb2.acls | 2 + > source4/torture/smb2/acls.c | 125 +++++++++++++++++++++++++++++++++ > 2 files changed, 127 insertions(+) > create mode 100644 selftest/knownfail.d/smb2.acls > >diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls >new file mode 100644 >index 00000000000..733a79381ac >--- /dev/null >+++ b/selftest/knownfail.d/smb2.acls >@@ -0,0 +1,2 @@ >+^samba3.smb2.acls.OWNER-RIGHTS\(ad_dc\) >+^samba3.smb2.acls.OWNER-RIGHTS\(nt4_dc\) >diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c >index 6178e211034..b02d74367e3 100644 >--- a/source4/torture/smb2/acls.c >+++ b/source4/torture/smb2/acls.c >@@ -2363,6 +2363,130 @@ static bool test_access_based(struct torture_context *tctx, > return ret; > } > >+/* >+ * test Owner Rights, S-1-3-4 >+ */ >+static bool test_owner_rights(struct torture_context *tctx, >+ struct smb2_tree *tree) >+{ >+ const char *fname = BASEDIR "\\owner_right.txt"; >+ struct smb2_create cr; >+ struct smb2_handle handle = {{0}}; >+ union smb_fileinfo gi; >+ union smb_setfileinfo si; >+ struct security_descriptor *sd_orig = NULL; >+ struct security_descriptor *sd = NULL; >+ const char *owner_sid = NULL; >+ NTSTATUS mxac_status; >+ NTSTATUS status; >+ bool ret = true; >+ >+ smb2_deltree(tree, BASEDIR); >+ >+ ret = smb2_util_setup_dir(tctx, tree, BASEDIR); >+ torture_assert_goto(tctx, ret, ret, done, >+ "smb2_util_setup_dir failed\n"); >+ >+ torture_comment(tctx, "TESTING OWNER RIGHTS\n"); >+ >+ cr = (struct smb2_create) { >+ .in.desired_access = SEC_STD_READ_CONTROL | >+ SEC_STD_WRITE_DAC |SEC_STD_WRITE_OWNER, >+ .in.file_attributes = FILE_ATTRIBUTE_NORMAL, >+ .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, >+ .in.create_disposition = NTCREATEX_DISP_OPEN_IF, >+ .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, >+ .in.fname = fname, >+ }; >+ >+ status = smb2_create(tree, tctx, &cr); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_create failed\n"); >+ handle = cr.out.file.handle; >+ >+ torture_comment(tctx, "get the original sd\n"); >+ >+ gi = (union smb_fileinfo) { >+ .query_secdesc.level = RAW_FILEINFO_SEC_DESC, >+ .query_secdesc.in.file.handle = handle, >+ .query_secdesc.in.secinfo_flags = SECINFO_DACL|SECINFO_OWNER, >+ }; >+ >+ status = smb2_getinfo_file(tree, tctx, &gi); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_getinfo_file failed\n"); >+ >+ sd_orig = gi.query_secdesc.out.sd; >+ owner_sid = dom_sid_string(tctx, sd_orig->owner_sid); >+ >+ sd = security_descriptor_dacl_create(tctx, 0, NULL, NULL, >+ owner_sid, >+ SEC_ACE_TYPE_ACCESS_ALLOWED, >+ SEC_RIGHTS_FILE_READ, >+ 0, >+ SID_OWNER_RIGHTS, >+ SEC_ACE_TYPE_ACCESS_ALLOWED, >+ SEC_RIGHTS_FILE_READ, >+ 0, >+ NULL); >+ torture_assert_not_null_goto(tctx, sd, ret, done, >+ "SD create failed\n"); >+ >+ si = (union smb_setfileinfo) { >+ .set_secdesc.level = RAW_SFILEINFO_SEC_DESC, >+ .set_secdesc.in.file.handle = handle, >+ .set_secdesc.in.secinfo_flags = SECINFO_DACL, >+ .set_secdesc.in.sd = sd, >+ }; >+ >+ status = smb2_setinfo_file(tree, &si); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_setinfo_file failed\n"); >+ >+ status = smb2_util_close(tree, handle); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_util_close failed\n"); >+ ZERO_STRUCT(handle); >+ >+ cr = (struct smb2_create) { >+ .in.desired_access = SEC_STD_READ_CONTROL, >+ .in.file_attributes = FILE_ATTRIBUTE_NORMAL, >+ .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, >+ .in.create_disposition = NTCREATEX_DISP_OPEN_IF, >+ .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, >+ .in.query_maximal_access = true, >+ .in.fname = fname, >+ }; >+ >+ status = smb2_create(tree, tctx, &cr); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_setinfo_file failed\n"); >+ handle = cr.out.file.handle; >+ >+ mxac_status = NT_STATUS(cr.out.maximal_access_status); >+ torture_assert_ntstatus_ok_goto(tctx, mxac_status, ret, done, >+ "smb2_setinfo_file failed\n"); >+ >+ /* SEC_STD_DELETE comes from the parent directory */ >+ torture_assert_int_equal_goto(tctx, >+ cr.out.maximal_access, >+ SEC_RIGHTS_FILE_READ|SEC_STD_DELETE, >+ ret, done, >+ "Wrong maximum access\n"); >+ >+ status = smb2_util_close(tree, handle); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_util_close failed\n"); >+ ZERO_STRUCT(handle); >+ >+done: >+ if (!smb2_util_handle_empty(handle)) { >+ smb2_util_close(tree, handle); >+ } >+ smb2_deltree(tree, BASEDIR); >+ return ret; >+} >+ > /* > basic testing of SMB2 ACLs > */ >@@ -2382,6 +2506,7 @@ struct torture_suite *torture_smb2_acls_init(TALLOC_CTX *ctx) > torture_suite_add_1smb2_test(suite, "GETSET", test_sd_get_set); > #endif > torture_suite_add_1smb2_test(suite, "ACCESSBASED", test_access_based); >+ torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS", test_owner_rights); > > suite->description = talloc_strdup(suite, "SMB2-ACLS tests"); > >-- >2.17.2 > > >From 39b5c2826e11ebe6897743a659cf7f48745cdc51 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Wed, 27 Feb 2019 18:07:03 +0100 >Subject: [PATCH 3/9] libcli/security: add "Owner Rights" calculation to > access_check_max_allowed() > >This was missing in 44590c1b70c0a24f853c02d5fcdb3c609401e2ca. > >Bug: https://bugzilla.samba.org/show_bug.cgi?id=13812 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: David Disseldorp <ddiss@samba.org> > >Autobuild-User(master): David Disseldorp <ddiss@samba.org> >Autobuild-Date(master): Thu Feb 28 19:18:16 UTC 2019 on sn-devel-144 > >(cherry picked from commit 5cf0764bc4b65dbc59d8626760dbe946a2234833) >--- > libcli/security/access_check.c | 33 ++++++++++++++++++++++++++++----- > selftest/knownfail.d/smb2.acls | 2 -- > 2 files changed, 28 insertions(+), 7 deletions(-) > delete mode 100644 selftest/knownfail.d/smb2.acls > >diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c >index 03a7dca4adf..5d49b718f0c 100644 >--- a/libcli/security/access_check.c >+++ b/libcli/security/access_check.c >@@ -110,13 +110,15 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd, > { > uint32_t denied = 0, granted = 0; > unsigned i; >- >- if (security_token_has_sid(token, sd->owner_sid)) { >- granted |= SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL; >- } >+ uint32_t owner_rights_allowed = 0; >+ uint32_t owner_rights_denied = 0; >+ bool owner_rights_default = true; > > if (sd->dacl == NULL) { >- return granted & ~denied; >+ if (security_token_has_sid(token, sd->owner_sid)) { >+ granted |= SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL; >+ } >+ return granted; > } > > for (i = 0;i<sd->dacl->num_aces; i++) { >@@ -126,6 +128,18 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd, > continue; > } > >+ if (dom_sid_equal(&ace->trustee, &global_sid_Owner_Rights)) { >+ if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED) { >+ owner_rights_allowed |= ace->access_mask; >+ owner_rights_default = false; >+ } else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) { >+ owner_rights_denied |= (owner_rights_allowed & >+ ace->access_mask); >+ owner_rights_default = false; >+ } >+ continue; >+ } >+ > if (!security_token_has_sid(token, &ace->trustee)) { > continue; > } >@@ -143,6 +157,15 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd, > } > } > >+ if (security_token_has_sid(token, sd->owner_sid)) { >+ if (owner_rights_default) { >+ granted |= SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL; >+ } else { >+ granted |= owner_rights_allowed; >+ granted &= ~owner_rights_denied; >+ } >+ } >+ > return granted & ~denied; > } > >diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls >deleted file mode 100644 >index 733a79381ac..00000000000 >--- a/selftest/knownfail.d/smb2.acls >+++ /dev/null >@@ -1,2 +0,0 @@ >-^samba3.smb2.acls.OWNER-RIGHTS\(ad_dc\) >-^samba3.smb2.acls.OWNER-RIGHTS\(nt4_dc\) >-- >2.17.2 > > >From d98e4c42dc3b5c729922b4484649569d0ab0b4ba Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 28 Feb 2019 13:55:31 -0800 >Subject: [PATCH 4/9] s4:torture: Fix the test_owner_rights() test to show > permissions are additive. > >Tested against Windows. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit 2e181e34c48c879235c5dc64bd7ab2b59781810c) >--- > source4/torture/smb2/acls.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > >diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c >index b02d74367e3..c45125b30dc 100644 >--- a/source4/torture/smb2/acls.c >+++ b/source4/torture/smb2/acls.c >@@ -2419,6 +2419,14 @@ static bool test_owner_rights(struct torture_context *tctx, > sd_orig = gi.query_secdesc.out.sd; > owner_sid = dom_sid_string(tctx, sd_orig->owner_sid); > >+ /* >+ * Add a 2 element ACL >+ * SEC_RIGHTS_FILE_READ for the owner, >+ * SEC_FILE_WRITE_DATA for SID_OWNER_RIGHTS. >+ * >+ * Proves that the owner and SID_OWNER_RIGHTS >+ * ACE entries are additive. >+ */ > sd = security_descriptor_dacl_create(tctx, 0, NULL, NULL, > owner_sid, > SEC_ACE_TYPE_ACCESS_ALLOWED, >@@ -2426,7 +2434,7 @@ static bool test_owner_rights(struct torture_context *tctx, > 0, > SID_OWNER_RIGHTS, > SEC_ACE_TYPE_ACCESS_ALLOWED, >- SEC_RIGHTS_FILE_READ, >+ SEC_FILE_WRITE_DATA, > 0, > NULL); > torture_assert_not_null_goto(tctx, sd, ret, done, >@@ -2467,10 +2475,14 @@ static bool test_owner_rights(struct torture_context *tctx, > torture_assert_ntstatus_ok_goto(tctx, mxac_status, ret, done, > "smb2_setinfo_file failed\n"); > >- /* SEC_STD_DELETE comes from the parent directory */ >+ /* >+ * For some reasons Windows 2016 doesn't set SEC_STD_DELETE but we >+ * do. Mask it out so the test passes against Samba and Windows. >+ */ > torture_assert_int_equal_goto(tctx, >- cr.out.maximal_access, >- SEC_RIGHTS_FILE_READ|SEC_STD_DELETE, >+ cr.out.maximal_access & ~SEC_STD_DELETE, >+ SEC_RIGHTS_FILE_READ | >+ SEC_FILE_WRITE_DATA, > ret, done, > "Wrong maximum access\n"); > >-- >2.17.2 > > >From 4c2eb788bec457e90d9f838afa724ecae923dc0d Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 28 Feb 2019 14:37:09 -0800 >Subject: [PATCH 5/9] s4:torture: Add test_owner_rights_deny(). > >Shows that owner and SID_OWNER_RIGHTS ACE >entries interact in max permissions requests. > >Tested against Windows. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit fadc4c1bc5fcc3b2d9daea44ef8daf8a8ae0fbe2) >--- > selftest/knownfail.d/smb2.acls | 2 + > source4/torture/smb2/acls.c | 137 +++++++++++++++++++++++++++++++++ > 2 files changed, 139 insertions(+) > create mode 100644 selftest/knownfail.d/smb2.acls > >diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls >new file mode 100644 >index 00000000000..e1b98cec606 >--- /dev/null >+++ b/selftest/knownfail.d/smb2.acls >@@ -0,0 +1,2 @@ >+^samba3.smb2.acls.OWNER-RIGHTS-DENY\(ad_dc\) >+^samba3.smb2.acls.OWNER-RIGHTS-DENY\(nt4_dc\) >diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c >index c45125b30dc..0f2d5345e72 100644 >--- a/source4/torture/smb2/acls.c >+++ b/source4/torture/smb2/acls.c >@@ -2499,6 +2499,141 @@ static bool test_owner_rights(struct torture_context *tctx, > return ret; > } > >+/* >+ * test Owner Rights with a leading DENY ACE, S-1-3-4 >+ */ >+static bool test_owner_rights_deny(struct torture_context *tctx, >+ struct smb2_tree *tree) >+{ >+ const char *fname = BASEDIR "\\owner_right_deny.txt"; >+ struct smb2_create cr; >+ struct smb2_handle handle = {{0}}; >+ union smb_fileinfo gi; >+ union smb_setfileinfo si; >+ struct security_descriptor *sd_orig = NULL; >+ struct security_descriptor *sd = NULL; >+ const char *owner_sid = NULL; >+ NTSTATUS mxac_status; >+ NTSTATUS status; >+ bool ret = true; >+ >+ smb2_deltree(tree, BASEDIR); >+ >+ ret = smb2_util_setup_dir(tctx, tree, BASEDIR); >+ torture_assert_goto(tctx, ret, ret, done, >+ "smb2_util_setup_dir failed\n"); >+ >+ torture_comment(tctx, "TESTING OWNER RIGHTS DENY\n"); >+ >+ cr = (struct smb2_create) { >+ .in.desired_access = SEC_STD_READ_CONTROL | >+ SEC_STD_WRITE_DAC |SEC_STD_WRITE_OWNER, >+ .in.file_attributes = FILE_ATTRIBUTE_NORMAL, >+ .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, >+ .in.create_disposition = NTCREATEX_DISP_OPEN_IF, >+ .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, >+ .in.fname = fname, >+ }; >+ >+ status = smb2_create(tree, tctx, &cr); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_create failed\n"); >+ handle = cr.out.file.handle; >+ >+ torture_comment(tctx, "get the original sd\n"); >+ >+ gi = (union smb_fileinfo) { >+ .query_secdesc.level = RAW_FILEINFO_SEC_DESC, >+ .query_secdesc.in.file.handle = handle, >+ .query_secdesc.in.secinfo_flags = SECINFO_DACL|SECINFO_OWNER, >+ }; >+ >+ status = smb2_getinfo_file(tree, tctx, &gi); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_getinfo_file failed\n"); >+ >+ sd_orig = gi.query_secdesc.out.sd; >+ owner_sid = dom_sid_string(tctx, sd_orig->owner_sid); >+ >+ /* >+ * Add a 2 element ACL >+ * DENY SEC_FILE_DATA_READ for SID_OWNER_RIGHTS >+ * SEC_FILE_READ_DATA for the owner. >+ * >+ * Proves that the owner and SID_OWNER_RIGHTS >+ * ACE entries are additive. >+ */ >+ sd = security_descriptor_dacl_create(tctx, 0, NULL, NULL, >+ SID_OWNER_RIGHTS, >+ SEC_ACE_TYPE_ACCESS_DENIED, >+ SEC_FILE_READ_DATA, >+ 0, >+ owner_sid, >+ SEC_ACE_TYPE_ACCESS_ALLOWED, >+ SEC_RIGHTS_FILE_READ, >+ 0, >+ NULL); >+ torture_assert_not_null_goto(tctx, sd, ret, done, >+ "SD create failed\n"); >+ >+ si = (union smb_setfileinfo) { >+ .set_secdesc.level = RAW_SFILEINFO_SEC_DESC, >+ .set_secdesc.in.file.handle = handle, >+ .set_secdesc.in.secinfo_flags = SECINFO_DACL, >+ .set_secdesc.in.sd = sd, >+ }; >+ >+ status = smb2_setinfo_file(tree, &si); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_setinfo_file failed\n"); >+ >+ status = smb2_util_close(tree, handle); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_util_close failed\n"); >+ ZERO_STRUCT(handle); >+ >+ cr = (struct smb2_create) { >+ .in.desired_access = SEC_STD_READ_CONTROL, >+ .in.file_attributes = FILE_ATTRIBUTE_NORMAL, >+ .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, >+ .in.create_disposition = NTCREATEX_DISP_OPEN_IF, >+ .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, >+ .in.query_maximal_access = true, >+ .in.fname = fname, >+ }; >+ >+ status = smb2_create(tree, tctx, &cr); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_setinfo_file failed\n"); >+ handle = cr.out.file.handle; >+ >+ mxac_status = NT_STATUS(cr.out.maximal_access_status); >+ torture_assert_ntstatus_ok_goto(tctx, mxac_status, ret, done, >+ "smb2_setinfo_file failed\n"); >+ >+ /* >+ * For some reasons Windows 2016 doesn't set SEC_STD_DELETE but we >+ * do. Mask it out so the test passes against Samba and Windows. >+ */ >+ torture_assert_int_equal_goto(tctx, >+ cr.out.maximal_access & ~SEC_STD_DELETE, >+ SEC_RIGHTS_FILE_READ & ~SEC_FILE_READ_DATA, >+ ret, done, >+ "Wrong maximum access\n"); >+ >+ status = smb2_util_close(tree, handle); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_util_close failed\n"); >+ ZERO_STRUCT(handle); >+ >+done: >+ if (!smb2_util_handle_empty(handle)) { >+ smb2_util_close(tree, handle); >+ } >+ smb2_deltree(tree, BASEDIR); >+ return ret; >+} >+ > /* > basic testing of SMB2 ACLs > */ >@@ -2519,6 +2654,8 @@ struct torture_suite *torture_smb2_acls_init(TALLOC_CTX *ctx) > #endif > torture_suite_add_1smb2_test(suite, "ACCESSBASED", test_access_based); > torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS", test_owner_rights); >+ torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS-DENY", >+ test_owner_rights_deny); > > suite->description = talloc_strdup(suite, "SMB2-ACLS tests"); > >-- >2.17.2 > > >From 94a83ea15eb2296efd698c5db5ceebeb785a6501 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Fri, 1 Mar 2019 18:20:35 +0100 >Subject: [PATCH 6/9] libcli/security: correct access check and maximum access > calculation for Owner Rights ACEs > >We basically must process the Owner Rights ACEs as any other ACE wrt to the >order of adding granted permissions and checking denied permissions. According >to MS-DTYP 2.5.3.2 Owner Rights ACEs must be evaluated in the main loop over >the ACEs in an ACL and the corresponding access_mask must be directly applied >to bits_remaining. We currently defer this to after the loop over the ACEs in >ACL, this is wrong. > >We just have to do some initial magic to determine if an ACL contains and >Owner Rights ACEs, and in case it doesn't we grant SEC_STD_WRITE_DAC | >SEC_STD_READ_CONTROL at the *beginning*. MS-DTYP: > >-- the owner of an object is always granted READ_CONTROL and WRITE_DAC. >CALL SidInToken(Token, SecurityDescriptor.Owner, PrincipalSelfSubst) >IF SidInToken returns True THEN > IF DACL does not contain ACEs from object owner THEN > Remove READ_CONTROL and WRITE_DAC from RemainingAccess > Set GrantedAccess to GrantedAccess or READ_CONTROL or WRITE_OWNER > END IF >END IF > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(cherry picked from commit 9722f75757c0e38c7f42c7cc310d56aa6eaf6392) >--- > libcli/security/access_check.c | 140 +++++++++++++++++---------------- > selftest/knownfail.d/smb2.acls | 2 - > 2 files changed, 73 insertions(+), 69 deletions(-) > delete mode 100644 selftest/knownfail.d/smb2.acls > >diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c >index 5d49b718f0c..d1d57eecef2 100644 >--- a/libcli/security/access_check.c >+++ b/libcli/security/access_check.c >@@ -109,10 +109,9 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd, > const struct security_token *token) > { > uint32_t denied = 0, granted = 0; >+ bool am_owner = false; >+ bool have_owner_rights_ace = false; > unsigned i; >- uint32_t owner_rights_allowed = 0; >- uint32_t owner_rights_denied = 0; >- bool owner_rights_default = true; > > if (sd->dacl == NULL) { > if (security_token_has_sid(token, sd->owner_sid)) { >@@ -121,26 +120,50 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd, > return granted; > } > >+ if (security_token_has_sid(token, sd->owner_sid)) { >+ /* >+ * Check for explicit owner rights: if there are none, we remove >+ * the default owner right SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL >+ * from remaining_access. Otherwise we just process the >+ * explicitly granted rights when processing the ACEs. >+ */ >+ am_owner = true; >+ >+ for (i=0; i < sd->dacl->num_aces; i++) { >+ struct security_ace *ace = &sd->dacl->aces[i]; >+ >+ if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) { >+ continue; >+ } >+ >+ have_owner_rights_ace = dom_sid_equal( >+ &ace->trustee, &global_sid_Owner_Rights); >+ if (have_owner_rights_ace) { >+ break; >+ } >+ } >+ } >+ >+ if (am_owner && !have_owner_rights_ace) { >+ granted |= SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL; >+ } >+ > for (i = 0;i<sd->dacl->num_aces; i++) { > struct security_ace *ace = &sd->dacl->aces[i]; >+ bool is_owner_rights_ace = false; > > if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) { > continue; > } > >- if (dom_sid_equal(&ace->trustee, &global_sid_Owner_Rights)) { >- if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED) { >- owner_rights_allowed |= ace->access_mask; >- owner_rights_default = false; >- } else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) { >- owner_rights_denied |= (owner_rights_allowed & >- ace->access_mask); >- owner_rights_default = false; >- } >- continue; >+ if (am_owner) { >+ is_owner_rights_ace = dom_sid_equal( >+ &ace->trustee, &global_sid_Owner_Rights); > } > >- if (!security_token_has_sid(token, &ace->trustee)) { >+ if (!is_owner_rights_ace && >+ !security_token_has_sid(token, &ace->trustee)) >+ { > continue; > } > >@@ -157,15 +180,6 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd, > } > } > >- if (security_token_has_sid(token, sd->owner_sid)) { >- if (owner_rights_default) { >- granted |= SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL; >- } else { >- granted |= owner_rights_allowed; >- granted &= ~owner_rights_denied; >- } >- } >- > return granted & ~denied; > } > >@@ -182,16 +196,8 @@ NTSTATUS se_access_check(const struct security_descriptor *sd, > uint32_t i; > uint32_t bits_remaining; > uint32_t explicitly_denied_bits = 0; >- /* >- * Up until Windows Server 2008, owner always had these rights. Now >- * we have to use Owner Rights perms if they are on the file. >- * >- * In addition we have to accumulate these bits and apply them >- * correctly. See bug #8795 >- */ >- uint32_t owner_rights_allowed = 0; >- uint32_t owner_rights_denied = 0; >- bool owner_rights_default = true; >+ bool am_owner = false; >+ bool have_owner_rights_ace = false; > > *access_granted = access_desired; > bits_remaining = access_desired; >@@ -221,35 +227,50 @@ NTSTATUS se_access_check(const struct security_descriptor *sd, > goto done; > } > >+ if (security_token_has_sid(token, sd->owner_sid)) { >+ /* >+ * Check for explicit owner rights: if there are none, we remove >+ * the default owner right SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL >+ * from remaining_access. Otherwise we just process the >+ * explicitly granted rights when processing the ACEs. >+ */ >+ am_owner = true; >+ >+ for (i=0; i < sd->dacl->num_aces; i++) { >+ struct security_ace *ace = &sd->dacl->aces[i]; >+ >+ if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) { >+ continue; >+ } >+ >+ have_owner_rights_ace = dom_sid_equal( >+ &ace->trustee, &global_sid_Owner_Rights); >+ if (have_owner_rights_ace) { >+ break; >+ } >+ } >+ } >+ if (am_owner && !have_owner_rights_ace) { >+ bits_remaining &= ~(SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL); >+ } >+ > /* check each ace in turn. */ > for (i=0; bits_remaining && i < sd->dacl->num_aces; i++) { > struct security_ace *ace = &sd->dacl->aces[i]; >+ bool is_owner_rights_ace = false; > > if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) { > continue; > } > >- /* >- * We need the Owner Rights permissions to ensure we >- * give or deny the correct permissions to the owner. Replace >- * owner_rights with the perms here if it is present. >- * >- * We don't care if we are not the owner because that is taken >- * care of below when we check if our token has the owner SID. >- * >- */ >- if (dom_sid_equal(&ace->trustee, &global_sid_Owner_Rights)) { >- if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED) { >- owner_rights_allowed |= ace->access_mask; >- owner_rights_default = false; >- } else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) { >- owner_rights_denied |= (bits_remaining & ace->access_mask); >- owner_rights_default = false; >- } >- continue; >+ if (am_owner) { >+ is_owner_rights_ace = dom_sid_equal( >+ &ace->trustee, &global_sid_Owner_Rights); > } > >- if (!security_token_has_sid(token, &ace->trustee)) { >+ if (!is_owner_rights_ace && >+ !security_token_has_sid(token, &ace->trustee)) >+ { > continue; > } > >@@ -269,21 +290,6 @@ NTSTATUS se_access_check(const struct security_descriptor *sd, > /* Explicitly denied bits always override */ > bits_remaining |= explicitly_denied_bits; > >- /* The owner always gets owner rights as defined above. */ >- if (security_token_has_sid(token, sd->owner_sid)) { >- if (owner_rights_default) { >- /* >- * Just remove them, no need to check if they are >- * there. >- */ >- bits_remaining &= ~(SEC_STD_WRITE_DAC | >- SEC_STD_READ_CONTROL); >- } else { >- bits_remaining &= ~owner_rights_allowed; >- bits_remaining |= owner_rights_denied; >- } >- } >- > /* > * We check privileges here because they override even DENY entries. > */ >diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls >deleted file mode 100644 >index e1b98cec606..00000000000 >--- a/selftest/knownfail.d/smb2.acls >+++ /dev/null >@@ -1,2 +0,0 @@ >-^samba3.smb2.acls.OWNER-RIGHTS-DENY\(ad_dc\) >-^samba3.smb2.acls.OWNER-RIGHTS-DENY\(nt4_dc\) >-- >2.17.2 > > >From 5bcf2c3143ec00005108d82c5cbc84aa1b43b13a Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 28 Feb 2019 14:59:01 -0800 >Subject: [PATCH 7/9] s4:torture: Add test_owner_rights_deny1(). > >Creates a 3-element ALLOW + ALLOW + DENY ACE showing that when >calculating maximum access already seen allow bits are not removed. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit 0ebd8c99aed28a0ba43a22c429837f66f7e94409) >--- > selftest/knownfail.d/smb2.acls | 2 + > source4/torture/smb2/acls.c | 144 +++++++++++++++++++++++++++++++++ > 2 files changed, 146 insertions(+) > create mode 100644 selftest/knownfail.d/smb2.acls > >diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls >new file mode 100644 >index 00000000000..f8541adc1b0 >--- /dev/null >+++ b/selftest/knownfail.d/smb2.acls >@@ -0,0 +1,2 @@ >+^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(ad_dc\) >+^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(nt4_dc\) >diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c >index 0f2d5345e72..916a582eac5 100644 >--- a/source4/torture/smb2/acls.c >+++ b/source4/torture/smb2/acls.c >@@ -2634,6 +2634,148 @@ static bool test_owner_rights_deny(struct torture_context *tctx, > return ret; > } > >+/* >+ * test Owner Rights with a trailing DENY ACE, S-1-3-4 >+ */ >+static bool test_owner_rights_deny1(struct torture_context *tctx, >+ struct smb2_tree *tree) >+{ >+ const char *fname = BASEDIR "\\owner_right_deny1.txt"; >+ struct smb2_create cr; >+ struct smb2_handle handle = {{0}}; >+ union smb_fileinfo gi; >+ union smb_setfileinfo si; >+ struct security_descriptor *sd_orig = NULL; >+ struct security_descriptor *sd = NULL; >+ const char *owner_sid = NULL; >+ NTSTATUS mxac_status; >+ NTSTATUS status; >+ bool ret = true; >+ >+ smb2_deltree(tree, BASEDIR); >+ >+ ret = smb2_util_setup_dir(tctx, tree, BASEDIR); >+ torture_assert_goto(tctx, ret, ret, done, >+ "smb2_util_setup_dir failed\n"); >+ >+ torture_comment(tctx, "TESTING OWNER RIGHTS DENY1\n"); >+ >+ cr = (struct smb2_create) { >+ .in.desired_access = SEC_STD_READ_CONTROL | >+ SEC_STD_WRITE_DAC |SEC_STD_WRITE_OWNER, >+ .in.file_attributes = FILE_ATTRIBUTE_NORMAL, >+ .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, >+ .in.create_disposition = NTCREATEX_DISP_OPEN_IF, >+ .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, >+ .in.fname = fname, >+ }; >+ >+ status = smb2_create(tree, tctx, &cr); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_create failed\n"); >+ handle = cr.out.file.handle; >+ >+ torture_comment(tctx, "get the original sd\n"); >+ >+ gi = (union smb_fileinfo) { >+ .query_secdesc.level = RAW_FILEINFO_SEC_DESC, >+ .query_secdesc.in.file.handle = handle, >+ .query_secdesc.in.secinfo_flags = SECINFO_DACL|SECINFO_OWNER, >+ }; >+ >+ status = smb2_getinfo_file(tree, tctx, &gi); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_getinfo_file failed\n"); >+ >+ sd_orig = gi.query_secdesc.out.sd; >+ owner_sid = dom_sid_string(tctx, sd_orig->owner_sid); >+ >+ /* >+ * Add a 3 element ACL >+ * >+ * SEC_RIGHTS_FILE_READ allow for owner. >+ * SEC_FILE_WRITE_DATA allow for SID-OWNER-RIGHTS. >+ * SEC_FILE_WRITE_DATA|SEC_FILE_READ_DATA) deny for SID-OWNER-RIGHTS. >+ * >+ * Shows on Windows that trailing DENY entries don't >+ * override granted permissions in max access calculations. >+ */ >+ sd = security_descriptor_dacl_create(tctx, 0, NULL, NULL, >+ owner_sid, >+ SEC_ACE_TYPE_ACCESS_ALLOWED, >+ SEC_RIGHTS_FILE_READ, >+ 0, >+ SID_OWNER_RIGHTS, >+ SEC_ACE_TYPE_ACCESS_ALLOWED, >+ SEC_FILE_WRITE_DATA, >+ 0, >+ SID_OWNER_RIGHTS, >+ SEC_ACE_TYPE_ACCESS_DENIED, >+ (SEC_FILE_WRITE_DATA| >+ SEC_FILE_READ_DATA), >+ 0, >+ NULL); >+ torture_assert_not_null_goto(tctx, sd, ret, done, >+ "SD create failed\n"); >+ >+ si = (union smb_setfileinfo) { >+ .set_secdesc.level = RAW_SFILEINFO_SEC_DESC, >+ .set_secdesc.in.file.handle = handle, >+ .set_secdesc.in.secinfo_flags = SECINFO_DACL, >+ .set_secdesc.in.sd = sd, >+ }; >+ >+ status = smb2_setinfo_file(tree, &si); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_setinfo_file failed\n"); >+ >+ status = smb2_util_close(tree, handle); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_util_close failed\n"); >+ ZERO_STRUCT(handle); >+ >+ cr = (struct smb2_create) { >+ .in.desired_access = SEC_STD_READ_CONTROL, >+ .in.file_attributes = FILE_ATTRIBUTE_NORMAL, >+ .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, >+ .in.create_disposition = NTCREATEX_DISP_OPEN_IF, >+ .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, >+ .in.query_maximal_access = true, >+ .in.fname = fname, >+ }; >+ >+ status = smb2_create(tree, tctx, &cr); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_setinfo_file failed\n"); >+ handle = cr.out.file.handle; >+ >+ mxac_status = NT_STATUS(cr.out.maximal_access_status); >+ torture_assert_ntstatus_ok_goto(tctx, mxac_status, ret, done, >+ "smb2_setinfo_file failed\n"); >+ >+ /* >+ * For some reasons Windows 2016 doesn't set SEC_STD_DELETE but we >+ * do. Mask it out so the test passes against Samba and Windows. >+ */ >+ torture_assert_int_equal_goto(tctx, >+ cr.out.maximal_access & ~SEC_STD_DELETE, >+ SEC_RIGHTS_FILE_READ | SEC_FILE_WRITE_DATA, >+ ret, done, >+ "Wrong maximum access\n"); >+ >+ status = smb2_util_close(tree, handle); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_util_close failed\n"); >+ ZERO_STRUCT(handle); >+ >+done: >+ if (!smb2_util_handle_empty(handle)) { >+ smb2_util_close(tree, handle); >+ } >+ smb2_deltree(tree, BASEDIR); >+ return ret; >+} >+ > /* > basic testing of SMB2 ACLs > */ >@@ -2656,6 +2798,8 @@ struct torture_suite *torture_smb2_acls_init(TALLOC_CTX *ctx) > torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS", test_owner_rights); > torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS-DENY", > test_owner_rights_deny); >+ torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS-DENY1", >+ test_owner_rights_deny1); > > suite->description = talloc_strdup(suite, "SMB2-ACLS tests"); > >-- >2.17.2 > > >From 50919fc4392a5fb221b2919028d91b3c5deffea3 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Sun, 3 Mar 2019 08:33:51 +0100 >Subject: [PATCH 8/9] s4:torture: Add test_deny1(). > >Creates a 2-element ALLOW + DENY ACE showing that when calculating >effective permissions and maximum access already seen allow bits are not >removed. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(cherry picked from commit b205d695d769e910a91bec87451dec189ec33740) >--- > selftest/knownfail.d/smb2.acls | 2 + > source4/torture/smb2/acls.c | 140 +++++++++++++++++++++++++++++++++ > 2 files changed, 142 insertions(+) > >diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls >index f8541adc1b0..b76a3c719ce 100644 >--- a/selftest/knownfail.d/smb2.acls >+++ b/selftest/knownfail.d/smb2.acls >@@ -1,2 +1,4 @@ > ^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(ad_dc\) > ^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(nt4_dc\) >+^samba3.smb2.acls.DENY1\(ad_dc\) >+^samba3.smb2.acls.DENY1\(nt4_dc\) >diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c >index 916a582eac5..7bccce803f0 100644 >--- a/source4/torture/smb2/acls.c >+++ b/source4/torture/smb2/acls.c >@@ -2776,6 +2776,144 @@ static bool test_owner_rights_deny1(struct torture_context *tctx, > return ret; > } > >+/* >+ * test that shows that a DENY ACE doesn't remove rights granted >+ * by a previous ALLOW ACE. >+ */ >+static bool test_deny1(struct torture_context *tctx, >+ struct smb2_tree *tree) >+{ >+ const char *fname = BASEDIR "\\test_deny1.txt"; >+ struct smb2_create cr; >+ struct smb2_handle handle = {{0}}; >+ union smb_fileinfo gi; >+ union smb_setfileinfo si; >+ struct security_descriptor *sd_orig = NULL; >+ struct security_descriptor *sd = NULL; >+ const char *owner_sid = NULL; >+ NTSTATUS mxac_status; >+ NTSTATUS status; >+ bool ret = true; >+ >+ smb2_deltree(tree, BASEDIR); >+ >+ ret = smb2_util_setup_dir(tctx, tree, BASEDIR); >+ torture_assert_goto(tctx, ret, ret, done, >+ "smb2_util_setup_dir failed\n"); >+ >+ cr = (struct smb2_create) { >+ .in.desired_access = SEC_STD_READ_CONTROL | >+ SEC_STD_WRITE_DAC |SEC_STD_WRITE_OWNER, >+ .in.file_attributes = FILE_ATTRIBUTE_NORMAL, >+ .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, >+ .in.create_disposition = NTCREATEX_DISP_OPEN_IF, >+ .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, >+ .in.fname = fname, >+ }; >+ >+ status = smb2_create(tree, tctx, &cr); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_create failed\n"); >+ handle = cr.out.file.handle; >+ >+ torture_comment(tctx, "get the original sd\n"); >+ >+ gi = (union smb_fileinfo) { >+ .query_secdesc.level = RAW_FILEINFO_SEC_DESC, >+ .query_secdesc.in.file.handle = handle, >+ .query_secdesc.in.secinfo_flags = SECINFO_DACL|SECINFO_OWNER, >+ }; >+ >+ status = smb2_getinfo_file(tree, tctx, &gi); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_getinfo_file failed\n"); >+ >+ sd_orig = gi.query_secdesc.out.sd; >+ owner_sid = dom_sid_string(tctx, sd_orig->owner_sid); >+ >+ /* >+ * Add a 2 element ACL >+ * >+ * SEC_RIGHTS_FILE_READ|SEC_FILE_WRITE_DATA allow for owner. >+ * SEC_FILE_WRITE_DATA deny for owner >+ * >+ * Shows on Windows that trailing DENY entries don't >+ * override granted permissions. >+ */ >+ sd = security_descriptor_dacl_create(tctx, 0, NULL, NULL, >+ owner_sid, >+ SEC_ACE_TYPE_ACCESS_ALLOWED, >+ SEC_RIGHTS_FILE_READ|SEC_FILE_WRITE_DATA, >+ 0, >+ owner_sid, >+ SEC_ACE_TYPE_ACCESS_DENIED, >+ SEC_FILE_WRITE_DATA, >+ 0, >+ NULL); >+ torture_assert_not_null_goto(tctx, sd, ret, done, >+ "SD create failed\n"); >+ >+ si = (union smb_setfileinfo) { >+ .set_secdesc.level = RAW_SFILEINFO_SEC_DESC, >+ .set_secdesc.in.file.handle = handle, >+ .set_secdesc.in.secinfo_flags = SECINFO_DACL, >+ .set_secdesc.in.sd = sd, >+ }; >+ >+ status = smb2_setinfo_file(tree, &si); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_setinfo_file failed\n"); >+ >+ status = smb2_util_close(tree, handle); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_util_close failed\n"); >+ ZERO_STRUCT(handle); >+ >+ cr = (struct smb2_create) { >+ .in.desired_access = SEC_STD_READ_CONTROL | SEC_FILE_WRITE_DATA, >+ .in.file_attributes = FILE_ATTRIBUTE_NORMAL, >+ .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, >+ .in.create_disposition = NTCREATEX_DISP_OPEN_IF, >+ .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, >+ .in.query_maximal_access = true, >+ .in.fname = fname, >+ }; >+ >+ status = smb2_create(tree, tctx, &cr); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_create failed\n"); >+ handle = cr.out.file.handle; >+ >+ mxac_status = NT_STATUS(cr.out.maximal_access_status); >+ torture_assert_ntstatus_ok_goto(tctx, mxac_status, ret, done, >+ "Wrong maximum access status\n"); >+ >+ /* >+ * For some reasons Windows 2016 doesn't set SEC_STD_DELETE but we >+ * do. Mask it out so the test passes against Samba and Windows. >+ * SEC_STD_WRITE_DAC comes from being the owner. >+ */ >+ torture_assert_int_equal_goto(tctx, >+ cr.out.maximal_access & ~SEC_STD_DELETE, >+ SEC_RIGHTS_FILE_READ | >+ SEC_FILE_WRITE_DATA | >+ SEC_STD_WRITE_DAC, >+ ret, done, >+ "Wrong maximum access\n"); >+ >+ status = smb2_util_close(tree, handle); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_util_close failed\n"); >+ ZERO_STRUCT(handle); >+ >+done: >+ if (!smb2_util_handle_empty(handle)) { >+ smb2_util_close(tree, handle); >+ } >+ smb2_deltree(tree, BASEDIR); >+ return ret; >+} >+ > /* > basic testing of SMB2 ACLs > */ >@@ -2800,6 +2938,8 @@ struct torture_suite *torture_smb2_acls_init(TALLOC_CTX *ctx) > test_owner_rights_deny); > torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS-DENY1", > test_owner_rights_deny1); >+ torture_suite_add_1smb2_test(suite, "DENY1", >+ test_deny1); > > suite->description = talloc_strdup(suite, "SMB2-ACLS tests"); > >-- >2.17.2 > > >From d4f46135cf23cfe612b48eed97001545a9b0c7d6 Mon Sep 17 00:00:00 2001 >From: Ralph Boehme <slow@samba.org> >Date: Fri, 1 Mar 2019 18:57:23 +0100 >Subject: [PATCH 9/9] libcli/security: fix handling of deny type ACEs in > access_check_max_allowed() > >Deny ACEs must always be evaluated against explicitly granted rights >from previous ACEs. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812 > >Signed-off-by: Ralph Boehme <slow@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(cherry picked from commit 8d355dd9769e8990ce998b4c9f28977669b43616) >--- > libcli/security/access_check.c | 2 +- > selftest/knownfail.d/smb2.acls | 4 ---- > 2 files changed, 1 insertion(+), 5 deletions(-) > delete mode 100644 selftest/knownfail.d/smb2.acls > >diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c >index d1d57eecef2..322f4fdb0c6 100644 >--- a/libcli/security/access_check.c >+++ b/libcli/security/access_check.c >@@ -173,7 +173,7 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd, > break; > case SEC_ACE_TYPE_ACCESS_DENIED: > case SEC_ACE_TYPE_ACCESS_DENIED_OBJECT: >- denied |= ace->access_mask; >+ denied |= ~granted & ace->access_mask; > break; > default: /* Other ACE types not handled/supported */ > break; >diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls >deleted file mode 100644 >index b76a3c719ce..00000000000 >--- a/selftest/knownfail.d/smb2.acls >+++ /dev/null >@@ -1,4 +0,0 @@ >-^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(ad_dc\) >-^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(nt4_dc\) >-^samba3.smb2.acls.DENY1\(ad_dc\) >-^samba3.smb2.acls.DENY1\(nt4_dc\) >-- >2.17.2 >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=14908">View the attachment on a separate page</a>.</b>
View Attachment As Raw
Flags:
jra
:
review+
Actions:
View
Attachments on
bug 13812
: 14908