[PATCH] Bug in dealing with "Owner Rights" ACEs when calculating maximum access
Ralph Böhme
slow at samba.org
Thu Feb 28 17:49:51 UTC 2019
Hi!
Just came across this one:
https://bugzilla.samba.org/show_bug.cgi?id=13812
When an SMB2 client queries maximum permission on a file or directory that has
an explicit "Owner Rights" ACE, hell breaks loose. The bugreport has all the
nasty details.
A customer ran across this with macOS clients, as in vfs_fruit I'm also calling
se_access_check() with SEC_FLAG_MAXIMUM_ALLOWED.
Please review & push if happy. Thanks!
-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG-Fingerprint FAE2C6088A24252051C559E4AA1E9B7126399E46
-------------- next part --------------
From 7c696fcf649ab11df71e53aed742626094798910 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 28 Feb 2019 14:47:18 +0100
Subject: [PATCH 1/3] s4:libcli: remember return code from maximum access
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13812
Signed-off-by: Ralph Boehme <slow at samba.org>
---
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 2d8d7edaa22f3d565e1f0d1d57a5d6b59251c795 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 28 Feb 2019 14:48:02 +0100
Subject: [PATCH 2/3] 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 at samba.org>
---
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 bc63030462ff0d3ab364c4cb6e9ccbbc739fd5ad Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 27 Feb 2019 18:07:03 +0100
Subject: [PATCH 3/3] 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 at samba.org>
---
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
More information about the samba-technical
mailing list