[PATCH] Follow-up patch for bug in dealing with "Owner Rights" ACEs when calculating maximum access
Ralph Böhme
slow at samba.org
Fri Mar 1 17:42:05 UTC 2019
On Fri, Mar 01, 2019 at 05:59:20PM +0100, Ralph Böhme wrote:
>On Fri, Mar 01, 2019 at 08:42:19AM -0800, Jeremy Allison wrote:
>>I'll take a look, but if you've allowed these new tests
>>to pass whilst simplifying the algorithm I'm all in favour :-).
>
>yeah, but it's a really subtle change with security implications.
>
>I still have to do a larger rewrite, so I think you can ignore the
>patches for now. I'll update here once I have something in shape for
>curious eyes. :)
ok, this ones looks much better. WIP patchset attached.
Passes make test TESTS=smbs.acls, CI is running here:
https://gitlab.com/samba-team/devel/samba/pipelines/49898599
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
-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG-Fingerprint FAE2C6088A24252051C559E4AA1E9B7126399E46
-------------- next part --------------
From b068155416184d335bce4f489f650f0807d96a6e Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 1 Mar 2019 09:48:25 +0100
Subject: [PATCH 1/8] s3:libsmb: add cli_smb2_query_mxac()
---
source3/libsmb/cli_smb2_fnum.c | 95 ++++++++++++++++++++++++++++++++++
source3/libsmb/cli_smb2_fnum.h | 3 ++
2 files changed, 98 insertions(+)
diff --git a/source3/libsmb/cli_smb2_fnum.c b/source3/libsmb/cli_smb2_fnum.c
index 795dc557c45..8a1df16e514 100644
--- a/source3/libsmb/cli_smb2_fnum.c
+++ b/source3/libsmb/cli_smb2_fnum.c
@@ -2781,6 +2781,101 @@ NTSTATUS cli_smb2_set_security_descriptor(struct cli_state *cli,
return status;
}
+/***************************************************************
+ Wrapper that allows SMB2 to query a security descriptor.
+ Synchronous only.
+***************************************************************/
+
+NTSTATUS cli_smb2_query_mxac(struct cli_state *cli,
+ const char *fname,
+ uint32_t *_mxac)
+{
+ TALLOC_CTX *frame = talloc_stackframe();
+ struct smb2_create_blobs in_cblobs = {0};
+ struct smb2_create_blobs out_cblobs = {0};
+ DATA_BLOB *mxac_blob = NULL;
+ struct smb_create_returns cr;
+ uint16_t fnum;
+ int i;
+ NTSTATUS mxac_status;
+ NTSTATUS status;
+
+ if (smbXcli_conn_has_async_calls(cli->conn)) {
+ /*
+ * Can't use sync call while an async call is in flight
+ */
+ status = NT_STATUS_INVALID_PARAMETER;
+ goto out;
+ }
+
+ if (smbXcli_conn_protocol(cli->conn) < PROTOCOL_SMB2_02) {
+ status = NT_STATUS_INVALID_PARAMETER;
+ goto out;
+ }
+
+ status = smb2_create_blob_add(frame,
+ &in_cblobs,
+ SMB2_CREATE_TAG_MXAC,
+ data_blob(NULL, 0));
+ if (!NT_STATUS_IS_OK(status)) {
+ goto out;
+ }
+
+
+ status = cli_smb2_create_fnum(
+ cli,
+ fname,
+ 0, /* create_flags */
+ SMB2_IMPERSONATION_IMPERSONATION,
+ FILE_READ_ATTRIBUTES, /* desired_access */
+ 0, /* file attributes */
+ FILE_SHARE_READ|FILE_SHARE_WRITE, /* share_access */
+ FILE_OPEN, /* create_disposition */
+ 0, /* create_options */
+ &in_cblobs,
+ &fnum,
+ &cr,
+ frame,
+ &out_cblobs);
+ if (!NT_STATUS_IS_OK(status)) {
+ goto out;
+ }
+
+ status = cli_smb2_close_fnum(cli, fnum);
+ if (!NT_STATUS_IS_OK(status)) {
+ goto out;
+ }
+
+ for (i = 0; i < out_cblobs.num_blobs; i++) {
+ if (strcmp(out_cblobs.blobs[i].tag, SMB2_CREATE_TAG_MXAC) != 0) {
+ continue;
+ }
+ mxac_blob = &out_cblobs.blobs[i].data;
+ break;
+ }
+ if (mxac_blob == NULL) {
+ status = NT_STATUS_INVALID_NETWORK_RESPONSE;
+ goto out;
+ }
+ if (mxac_blob->length != 8) {
+ status = NT_STATUS_INVALID_NETWORK_RESPONSE;
+ goto out;
+ }
+
+ mxac_status = NT_STATUS(IVAL(mxac_blob->data, 0));
+ if (!NT_STATUS_IS_OK(mxac_status)) {
+ status = mxac_status;
+ goto out;
+ }
+
+ *_mxac = IVAL(mxac_blob->data, 4);
+ status = NT_STATUS_OK;
+
+out:
+ TALLOC_FREE(frame);
+ return status;
+}
+
/***************************************************************
Wrapper that allows SMB2 to rename a file.
Synchronous only.
diff --git a/source3/libsmb/cli_smb2_fnum.h b/source3/libsmb/cli_smb2_fnum.h
index 62ae093862f..fd57bec2938 100644
--- a/source3/libsmb/cli_smb2_fnum.h
+++ b/source3/libsmb/cli_smb2_fnum.h
@@ -173,6 +173,9 @@ NTSTATUS cli_smb2_set_security_descriptor(struct cli_state *cli,
uint16_t fnum,
uint32_t sec_info,
const struct security_descriptor *sd);
+NTSTATUS cli_smb2_query_mxac(struct cli_state *cli,
+ const char *fname,
+ uint32_t *mxac);
NTSTATUS cli_smb2_rename(struct cli_state *cli,
const char *fname_src,
const char *fname_dst,
--
2.17.2
From 7ab8d8c3c8dc22e875dfd30b4877e44dc6dc0329 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 1 Mar 2019 09:49:17 +0100
Subject: [PATCH 2/8] s3:libsmb: add cli_query_mxac()
Works only for SMB2. Is there a way to do this over SMB1?
---
source3/libsmb/clisecdesc.c | 12 ++++++++++++
source3/libsmb/proto.h | 4 ++++
2 files changed, 16 insertions(+)
diff --git a/source3/libsmb/clisecdesc.c b/source3/libsmb/clisecdesc.c
index c11e4b322ef..f9a7a057ed6 100644
--- a/source3/libsmb/clisecdesc.c
+++ b/source3/libsmb/clisecdesc.c
@@ -91,6 +91,18 @@ NTSTATUS cli_query_secdesc(struct cli_state *cli, uint16_t fnum,
return cli_query_security_descriptor(cli, fnum, sec_info, mem_ctx, sd);
}
+NTSTATUS cli_query_mxac(struct cli_state *cli,
+ const char *filename,
+ uint32_t *mxac)
+{
+ if (smbXcli_conn_protocol(cli->conn) < PROTOCOL_SMB2_02) {
+ return NT_STATUS_NOT_SUPPORTED;
+ }
+
+ return cli_smb2_query_mxac(cli, filename, mxac);
+}
+
+
/****************************************************************************
set the security descriptor for a open file
****************************************************************************/
diff --git a/source3/libsmb/proto.h b/source3/libsmb/proto.h
index b0cfcb5aa90..e1a54ef75f9 100644
--- a/source3/libsmb/proto.h
+++ b/source3/libsmb/proto.h
@@ -927,6 +927,10 @@ NTSTATUS cli_set_security_descriptor(struct cli_state *cli,
NTSTATUS cli_set_secdesc(struct cli_state *cli, uint16_t fnum,
const struct security_descriptor *sd);
+NTSTATUS cli_query_mxac(struct cli_state *cli,
+ const char *filename,
+ uint32_t *mxac);
+
/* The following definitions come from libsmb/clistr.c */
size_t clistr_pull_talloc(TALLOC_CTX *ctx,
--
2.17.2
From c5e96348c9d9a4c828b2ac30ad364ef5a3b67b7a Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 27 Feb 2019 16:45:07 +0100
Subject: [PATCH 3/8] smbcacls -x
---
source3/utils/smbcacls.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/source3/utils/smbcacls.c b/source3/utils/smbcacls.c
index a3a40e9eeb9..b61d11df860 100644
--- a/source3/utils/smbcacls.c
+++ b/source3/utils/smbcacls.c
@@ -40,6 +40,7 @@ static int test_args;
static int sddl;
static int query_sec_info = -1;
static int set_sec_info = -1;
+static bool want_mxac;
static const char *domain_sid = NULL;
@@ -359,12 +360,33 @@ static bool set_secdesc(struct cli_state *cli, const char *filename,
return result;
}
+/*****************************************************
+get maximum access for a file
+*******************************************************/
+static int cacl_mxac(struct cli_state *cli, const char *filename)
+{
+ NTSTATUS status;
+ uint32_t mxac;
+
+ status = cli_query_mxac(cli, filename, &mxac);
+ if (!NT_STATUS_IS_OK(status)) {
+ printf("Failed to get mxac: %s\n", nt_errstr(status));
+ return EXIT_FAILED;
+ }
+
+ printf("Maximum access: 0x%x\n", mxac);
+
+ return EXIT_OK;
+}
+
+
/*****************************************************
dump the acls for a file
*******************************************************/
static int cacl_dump(struct cli_state *cli, const char *filename, bool numeric)
{
struct security_descriptor *sd;
+ int ret;
if (test_args) {
return EXIT_OK;
@@ -386,6 +408,13 @@ static int cacl_dump(struct cli_state *cli, const char *filename, bool numeric)
sec_desc_print(cli, stdout, sd, numeric);
}
+ if (want_mxac) {
+ ret = cacl_mxac(cli, filename);
+ if (ret != EXIT_OK) {
+ return ret;
+ }
+ }
+
return EXIT_OK;
}
@@ -910,6 +939,14 @@ int main(int argc, char *argv[])
.descrip = "Set the max protocol level",
.argDescrip = "LEVEL",
},
+ {
+ .longName = "maximum-access",
+ .shortName = 'x',
+ .argInfo = POPT_ARG_NONE,
+ .arg = NULL,
+ .val = 'x',
+ .descrip = "Query maximum persmissions",
+ },
POPT_COMMON_SAMBA
POPT_COMMON_CONNECTION
POPT_COMMON_CREDENTIALS
@@ -975,6 +1012,9 @@ int main(int argc, char *argv[])
case 'm':
lp_set_cmdline("client max protocol", poptGetOptArg(pc));
break;
+ case 'x':
+ want_mxac = true;
+ break;
}
}
--
2.17.2
From dcb7bfba3e48c94bc571c5c07fdcab96eaad6b46 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 4/8] 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.17.2
From d6960c3c908a6373620ca4ff15d98a7bfd1312fc 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 5/8] 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 @@ 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");
+
+ /* 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.17.2
From a59a39874915c6f4edef7e2489df6f1b58a59a80 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 6/8] 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 @@ static bool test_owner_rights_deny(struct torture_context *tctx,
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.17.2
From 7f3e8433aa36e114c3343ee59df5e65456d82a10 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 1 Mar 2019 18:06:48 +0100
Subject: [PATCH 7/8] s4:torture: add a test with additional bits in
SEC_FLAG_MAXIMUM_ALLOWED
---
source4/torture/smb2/acls.c | 111 ++++++++++++++++++++++++++++++++++++
1 file changed, 111 insertions(+)
diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c
index 00a625fce06..c513b021fd3 100644
--- a/source4/torture/smb2/acls.c
+++ b/source4/torture/smb2/acls.c
@@ -2777,6 +2777,115 @@ static bool test_owner_rights_deny1(struct torture_context *tctx,
return ret;
}
+/*
+ * test SEC_FLAG_MAXIMUM_ALLOWED with not-granted access
+ *
+ * When access_mask contains SEC_FLAG_MAXIMUM_ALLOWED, the server must still
+ * proces other bits from access_mask. Eg if access_mask contains a right that
+ * the requester doesn't have, the function must validate that against the
+ * effective permissions.
+ */
+static bool test_mxac_not_granted(struct torture_context *tctx,
+ struct smb2_tree *tree)
+{
+ const char *fname = BASEDIR "\\test_mxac_not_granted.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 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);
+
+ sd = security_descriptor_dacl_create(tctx, 0, NULL, NULL,
+ owner_sid,
+ SEC_ACE_TYPE_ACCESS_ALLOWED,
+ 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_FLAG_MAXIMUM_ALLOWED |
+ 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.fname = fname,
+ };
+
+ status = smb2_create(tree, tctx, &cr);
+ torture_assert_ntstatus_equal_goto(tctx, status,
+ NT_STATUS_ACCESS_DENIED,
+ ret, done,
+ "Wrong smb2_create result\n");
+
+done:
+ if (!smb2_util_handle_empty(handle)) {
+ smb2_util_close(tree, handle);
+ }
+ smb2_deltree(tree, BASEDIR);
+ return ret;
+}
+
/*
basic testing of SMB2 ACLs
*/
@@ -2801,6 +2910,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, "MXAC-NOT-GRANTED",
+ test_mxac_not_granted);
suite->description = talloc_strdup(suite, "SMB2-ACLS tests");
--
2.17.2
From e8b1bc9b1d924f29930f02879a961eaeef36e258 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 1 Mar 2019 18:20:35 +0100
Subject: [PATCH 8/8] WIP: libcli/security: correct access check and maximum
access calculation for Owner Rights ACEs
---
libcli/security/access_check.c | 142 +++++++++++++++++----------------
selftest/knownfail.d/smb2.acls | 4 -
2 files changed, 74 insertions(+), 72 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..389879464d9 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 owner = false;
+ bool owner_rights = 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.
+ */
+ 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;
+ }
+
+ owner_rights = dom_sid_equal(&ace->trustee,
+ &global_sid_Owner_Rights);
+ if (owner_rights) {
+ break;
+ }
+ }
+ }
+
+ if (owner && !owner_rights) {
+ 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 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 (owner) {
+ owner_rights_ace = dom_sid_equal(
+ &ace->trustee, &global_sid_Owner_Rights);
}
- if (!security_token_has_sid(token, &ace->trustee)) {
+ if (!owner_rights_ace &&
+ !security_token_has_sid(token, &ace->trustee))
+ {
continue;
}
@@ -150,22 +173,13 @@ 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;
}
}
- 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 owner = false;
+ bool owner_rights = 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.
+ */
+ 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;
+ }
+
+ owner_rights = dom_sid_equal(&ace->trustee,
+ &global_sid_Owner_Rights);
+ if (owner_rights) {
+ break;
+ }
+ }
+ }
+ if (owner && !owner_rights) {
+ 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 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 (owner) {
+ owner_rights_ace = dom_sid_equal(
+ &ace->trustee, &global_sid_Owner_Rights);
}
- if (!security_token_has_sid(token, &ace->trustee)) {
+ if (!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 a01fc0097e4..00000000000
--- a/selftest/knownfail.d/smb2.acls
+++ /dev/null
@@ -1,4 +0,0 @@
-^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.17.2
More information about the samba-technical
mailing list