[PATCH] Follow-up patch for bug in dealing with "Owner Rights" ACEs when calculating maximum access
Jeremy Allison
jra at samba.org
Thu Feb 28 23:12:56 UTC 2019
On Thu, Feb 28, 2019 at 02:11:27PM -0800, Jeremy Allison via samba-technical wrote:
> On Thu, Feb 28, 2019 at 11:04:54PM +0100, David Disseldorp wrote:
> > >
> > > Is there a MS-DTYP reference for this that I'm missing ?
> >
> > Hmm, yeah I think you're right, considering owner_rights_allowed in the
> > TYPE_ACCESS_DENIED path looks wrong here. I'd guess it's a cut'n'paste
> > error from se_access_check()->bits_remaining. You can have a RB+ from
> > me for your follow up fix, but let's wait for Ralph's response :)
>
> It's more complex than that - I've been doing lots of Windows
> testing :-).
>
> Additional patchset incoming that should get us to match
> Windows (bizarre:-) behaviour here, once I've got us passing
> my new tests..
And here it is. Only remaining query I have is for the
second patch. I'm adding a test that currently can
only pass against Windows as it creates a pathological
3-element ACL with 2 ALLOWS followed by a DENY that
Samba can't map as we're going to the POSIX backend.
Right now I just note it as knownfail but still run
it. I could make us skip it if we know we're running
against Samba by adding a:
if (torture_setting_bool(tctx, "samba3", false)
check, but I thought it easier just to leave it as
knownfail until we get to allowing broken ACLs :-).
Please review and push, but this gets us as far
as I can tell 100% compatibility with Windows
(famous last words :-).
Please review and push if happy !
Jeremy.
-------------- next part --------------
From 1b3f5b9ebe0f789081e6d6332e4441b1a9dabfd9 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 28 Feb 2019 13:55:31 -0800
Subject: [PATCH 1/4] 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 at samba.org>
---
source4/torture/smb2/acls.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c
index b02d74367e3..0425d87bc04 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,
@@ -2470,7 +2478,8 @@ static bool test_owner_rights(struct torture_context *tctx,
/* 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,
+ SEC_RIGHTS_FILE_READ|SEC_STD_DELETE|
+ SEC_FILE_WRITE_DATA,
ret, done,
"Wrong maximum access\n");
--
2.21.0.rc2.261.ga7da99ff1b-goog
From fb3c51d22f4aa3f149cbf7a371be2ad82258d332 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 28 Feb 2019 14:37:09 -0800
Subject: [PATCH 2/4] 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 at samba.org>
---
selftest/knownfail.d/smb2.acls | 2 +
source4/torture/smb2/acls.c | 135 +++++++++++++++++++++++++++++++++
2 files changed, 137 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 0425d87bc04..60f05e1b99c 100644
--- a/source4/torture/smb2/acls.c
+++ b/source4/torture/smb2/acls.c
@@ -2496,6 +2496,139 @@ done:
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");
+
+ /* 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) &
+ (~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
*/
@@ -2516,6 +2649,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.21.0.rc2.261.ga7da99ff1b-goog
From 18d33ce958d412b211eb2e15a95a8ef1fa73b925 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 28 Feb 2019 14:59:01 -0800
Subject: [PATCH 3/4] 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, thus
giving me the info I need to put the correct
algorithm in access_check_max_allowed() in
the next commit.
Tested against Windows.
Note this test can't currently pass against
Samba as it creates a pathalogical ACL that
cannot be mapped to a POSIX backend.
Currently just mark as knownfail, but should
I use if (torture_setting_bool(tctx, "samba3", false)
to skip instead ?
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812
Signed-off-by: Jeremy Allison <jra at samba.org>
---
selftest/knownfail.d/smb2.acls | 2 +
source4/torture/smb2/acls.c | 150 +++++++++++++++++++++++++++++++++
2 files changed, 152 insertions(+)
diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls
index e1b98cec606..a01fc0097e4 100644
--- a/selftest/knownfail.d/smb2.acls
+++ b/selftest/knownfail.d/smb2.acls
@@ -1,2 +1,4 @@
^samba3.smb2.acls.OWNER-RIGHTS-DENY\(ad_dc\)
^samba3.smb2.acls.OWNER-RIGHTS-DENY\(nt4_dc\)
+^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 60f05e1b99c..00a625fce06 100644
--- a/source4/torture/smb2/acls.c
+++ b/source4/torture/smb2/acls.c
@@ -2629,6 +2629,154 @@ done:
return ret;
}
+/*
+ * test Owner Rights with a trailing DENY ACE, S-1-3-4
+ * NOTE: Samba *cannot* currently pass this as it creates
+ * a pathalogical set of ACE's (ALLOW followed by DENY)
+ * that the posix ACL code cannot cope with. Passes
+ * against Windows.
+ *
+ * Right now mark this as selftest/knownfail.d/smb2.acls,
+ * but should I just skip this test using
+ * if (torture_setting_bool(tctx, "samba3", false) ?
+ */
+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");
+
+ /* 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|
+ 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
*/
@@ -2651,6 +2799,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.21.0.rc2.261.ga7da99ff1b-goog
From aab2c73dee20e35e8948c005d3c8da9e442120f7 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 28 Feb 2019 15:05:51 -0800
Subject: [PATCH 4/4] libcli: Fix access_check_max_allowed() algorithm to match
Windows.
Fixes Owner Rights calculations.
We match completely as far I can can tell now.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812
Signed-off-by: Jeremy Allison <jra at samba.org>
---
libcli/security/access_check.c | 13 +++++++++++--
selftest/knownfail.d/smb2.acls | 2 --
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c
index 5d49b718f0c..6a9f1c6b872 100644
--- a/libcli/security/access_check.c
+++ b/libcli/security/access_check.c
@@ -133,8 +133,17 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd,
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);
+ /*
+ * Ignore bits we've already seen explitily
+ * allowed either by matching ACE or
+ * SID_OWNER_RIGHTS.
+ */
+ uint32_t already_granted = (granted |
+ owner_rights_allowed);
+
+ owner_rights_denied |= (ace->access_mask &
+ ~already_granted);
+
owner_rights_default = false;
}
continue;
diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls
index a01fc0097e4..f8541adc1b0 100644
--- a/selftest/knownfail.d/smb2.acls
+++ b/selftest/knownfail.d/smb2.acls
@@ -1,4 +1,2 @@
-^samba3.smb2.acls.OWNER-RIGHTS-DENY\(ad_dc\)
-^samba3.smb2.acls.OWNER-RIGHTS-DENY\(nt4_dc\)
^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(ad_dc\)
^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(nt4_dc\)
--
2.21.0.rc2.261.ga7da99ff1b-goog
More information about the samba-technical
mailing list