From ab9445a0bb7169cd2945d734429f61b28c84baaa Mon Sep 17 00:00:00 2001 From: Ralph Boehme 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 Reviewed-by: David Disseldorp (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;iout.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 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 Reviewed-by: David Disseldorp (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 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 Reviewed-by: David Disseldorp Autobuild-User(master): David Disseldorp 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;idacl->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 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 Reviewed-by: Ralph Boehme (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 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 Reviewed-by: Ralph Boehme (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 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 Reviewed-by: Jeremy Allison (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;idacl->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 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 Reviewed-by: Ralph Boehme (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 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 Reviewed-by: Jeremy Allison (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 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 Reviewed-by: Jeremy Allison (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