[PATCH] Follow-up patch for bug in dealing with "Owner Rights" ACEs when calculating maximum access
Jeremy Allison
jra at samba.org
Mon Mar 4 18:08:14 UTC 2019
On Mon, Mar 04, 2019 at 07:12:15AM +0100, Ralph Böhme via samba-technical wrote:
> On Fri, Mar 01, 2019 at 02:45:20PM -0800, Jeremy Allison wrote:
> > Let me know when you're done and I'll review with
> > a view to pushing to master !
>
> final patchset with a few changes attached:
>
> - futher refined variable names in libcli/security: fix handling of deny
> type ACEs in access_check_max_allowed()
>
> - fixed test to remove SEC_STD_DELETE from expected maximum access bits, as
> Windows 2016 wasn't return that bit. I didn't want to open this can of
> worms, so I simply remove the bit from the exepcted mask.
>
> - additional test that does the same ALLOW+DENY ACE check as
> test_owner_rights_deny1(), just not with Owner Rights but with a user SID
>
> - additional test that verifies that the server honors all bit in a
> requested access mask that contains SEC_FLAG_MAXIMUM_ALLOWED
>
> - patchset also contains my smbcalcs extension (for master only), including
> async versions of cli_smb2_query_mxac() as requested by Volker, a simple
> test and a manpage update
>
> CI: https://gitlab.com/samba-team/devel/samba/pipelines/50089003
>
> All tests pass against WIndows 2016.
>
> Please review&push if happy. Thanks!
Great work Ralph, thanks ! LGTM, RB+ and pushed (hotel
network permitting :-).
Jeremy.
> --
> Ralph Boehme, Samba Team https://samba.org/
> Samba Developer, SerNet GmbH https://sernet.de/en/samba/
> GPG-Fingerprint FAE2C6088A24252051C559E4AA1E9B7126399E46
> From fd0d90958f1ed689a0734838fd44464e784777a2 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 01/11] 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>
> Reviewed-by: Ralph Boehme <slow at samba.org>
> ---
> 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 eb33947a388f69b869b3ec16343c05958d4a6ea9 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 02/11] 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>
> Reviewed-by: Ralph Boehme <slow at samba.org>
> ---
> 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 a41a4f53a37ce4eb0d3203d099515252139b1e99 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 03/11] libcli/security: correct access check and maximum
> access calculation for Owner Rights ACEs
>
> We basically must process the Owner Rights ACEs as any other ACE wrt to the
> order of adding granted permissions and checking denied permissions. According
> to MS-DTYP 2.5.3.2 Owner Rights ACEs must be evaluated in the main loop over
> the ACEs in an ACL and the corresponding access_mask must be directly applied
> to bits_remaining. We currently defer this to after the loop over the ACEs in
> ACL, this is wrong.
>
> We just have to do some initial magic to determine if an ACL contains and
> Owner Rights ACEs, and in case it doesn't we grant SEC_STD_WRITE_DAC |
> SEC_STD_READ_CONTROL at the *beginning*. MS-DTYP:
>
> -- the owner of an object is always granted READ_CONTROL and WRITE_DAC.
> CALL SidInToken(Token, SecurityDescriptor.Owner, PrincipalSelfSubst)
> IF SidInToken returns True THEN
> IF DACL does not contain ACEs from object owner THEN
> Remove READ_CONTROL and WRITE_DAC from RemainingAccess
> Set GrantedAccess to GrantedAccess or READ_CONTROL or WRITE_OWNER
> END IF
> END IF
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> libcli/security/access_check.c | 140 +++++++++++++++++----------------
> selftest/knownfail.d/smb2.acls | 2 -
> 2 files changed, 73 insertions(+), 69 deletions(-)
> delete mode 100644 selftest/knownfail.d/smb2.acls
>
> diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c
> index 5d49b718f0c..d1d57eecef2 100644
> --- a/libcli/security/access_check.c
> +++ b/libcli/security/access_check.c
> @@ -109,10 +109,9 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd,
> const struct security_token *token)
> {
> uint32_t denied = 0, granted = 0;
> + bool am_owner = false;
> + bool have_owner_rights_ace = false;
> unsigned i;
> - uint32_t owner_rights_allowed = 0;
> - uint32_t owner_rights_denied = 0;
> - bool owner_rights_default = true;
>
> if (sd->dacl == NULL) {
> if (security_token_has_sid(token, sd->owner_sid)) {
> @@ -121,26 +120,50 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd,
> return granted;
> }
>
> + if (security_token_has_sid(token, sd->owner_sid)) {
> + /*
> + * Check for explicit owner rights: if there are none, we remove
> + * the default owner right SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL
> + * from remaining_access. Otherwise we just process the
> + * explicitly granted rights when processing the ACEs.
> + */
> + am_owner = true;
> +
> + for (i=0; i < sd->dacl->num_aces; i++) {
> + struct security_ace *ace = &sd->dacl->aces[i];
> +
> + if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
> + continue;
> + }
> +
> + have_owner_rights_ace = dom_sid_equal(
> + &ace->trustee, &global_sid_Owner_Rights);
> + if (have_owner_rights_ace) {
> + break;
> + }
> + }
> + }
> +
> + if (am_owner && !have_owner_rights_ace) {
> + granted |= SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL;
> + }
> +
> for (i = 0;i<sd->dacl->num_aces; i++) {
> struct security_ace *ace = &sd->dacl->aces[i];
> + bool is_owner_rights_ace = false;
>
> if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
> continue;
> }
>
> - if (dom_sid_equal(&ace->trustee, &global_sid_Owner_Rights)) {
> - if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED) {
> - owner_rights_allowed |= ace->access_mask;
> - owner_rights_default = false;
> - } else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) {
> - owner_rights_denied |= (owner_rights_allowed &
> - ace->access_mask);
> - owner_rights_default = false;
> - }
> - continue;
> + if (am_owner) {
> + is_owner_rights_ace = dom_sid_equal(
> + &ace->trustee, &global_sid_Owner_Rights);
> }
>
> - if (!security_token_has_sid(token, &ace->trustee)) {
> + if (!is_owner_rights_ace &&
> + !security_token_has_sid(token, &ace->trustee))
> + {
> continue;
> }
>
> @@ -157,15 +180,6 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd,
> }
> }
>
> - if (security_token_has_sid(token, sd->owner_sid)) {
> - if (owner_rights_default) {
> - granted |= SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL;
> - } else {
> - granted |= owner_rights_allowed;
> - granted &= ~owner_rights_denied;
> - }
> - }
> -
> return granted & ~denied;
> }
>
> @@ -182,16 +196,8 @@ NTSTATUS se_access_check(const struct security_descriptor *sd,
> uint32_t i;
> uint32_t bits_remaining;
> uint32_t explicitly_denied_bits = 0;
> - /*
> - * Up until Windows Server 2008, owner always had these rights. Now
> - * we have to use Owner Rights perms if they are on the file.
> - *
> - * In addition we have to accumulate these bits and apply them
> - * correctly. See bug #8795
> - */
> - uint32_t owner_rights_allowed = 0;
> - uint32_t owner_rights_denied = 0;
> - bool owner_rights_default = true;
> + bool am_owner = false;
> + bool have_owner_rights_ace = false;
>
> *access_granted = access_desired;
> bits_remaining = access_desired;
> @@ -221,35 +227,50 @@ NTSTATUS se_access_check(const struct security_descriptor *sd,
> goto done;
> }
>
> + if (security_token_has_sid(token, sd->owner_sid)) {
> + /*
> + * Check for explicit owner rights: if there are none, we remove
> + * the default owner right SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL
> + * from remaining_access. Otherwise we just process the
> + * explicitly granted rights when processing the ACEs.
> + */
> + am_owner = true;
> +
> + for (i=0; i < sd->dacl->num_aces; i++) {
> + struct security_ace *ace = &sd->dacl->aces[i];
> +
> + if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
> + continue;
> + }
> +
> + have_owner_rights_ace = dom_sid_equal(
> + &ace->trustee, &global_sid_Owner_Rights);
> + if (have_owner_rights_ace) {
> + break;
> + }
> + }
> + }
> + if (am_owner && !have_owner_rights_ace) {
> + bits_remaining &= ~(SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL);
> + }
> +
> /* check each ace in turn. */
> for (i=0; bits_remaining && i < sd->dacl->num_aces; i++) {
> struct security_ace *ace = &sd->dacl->aces[i];
> + bool is_owner_rights_ace = false;
>
> if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
> continue;
> }
>
> - /*
> - * We need the Owner Rights permissions to ensure we
> - * give or deny the correct permissions to the owner. Replace
> - * owner_rights with the perms here if it is present.
> - *
> - * We don't care if we are not the owner because that is taken
> - * care of below when we check if our token has the owner SID.
> - *
> - */
> - if (dom_sid_equal(&ace->trustee, &global_sid_Owner_Rights)) {
> - if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED) {
> - owner_rights_allowed |= ace->access_mask;
> - owner_rights_default = false;
> - } else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) {
> - owner_rights_denied |= (bits_remaining & ace->access_mask);
> - owner_rights_default = false;
> - }
> - continue;
> + if (am_owner) {
> + is_owner_rights_ace = dom_sid_equal(
> + &ace->trustee, &global_sid_Owner_Rights);
> }
>
> - if (!security_token_has_sid(token, &ace->trustee)) {
> + if (!is_owner_rights_ace &&
> + !security_token_has_sid(token, &ace->trustee))
> + {
> continue;
> }
>
> @@ -269,21 +290,6 @@ NTSTATUS se_access_check(const struct security_descriptor *sd,
> /* Explicitly denied bits always override */
> bits_remaining |= explicitly_denied_bits;
>
> - /* The owner always gets owner rights as defined above. */
> - if (security_token_has_sid(token, sd->owner_sid)) {
> - if (owner_rights_default) {
> - /*
> - * Just remove them, no need to check if they are
> - * there.
> - */
> - bits_remaining &= ~(SEC_STD_WRITE_DAC |
> - SEC_STD_READ_CONTROL);
> - } else {
> - bits_remaining &= ~owner_rights_allowed;
> - bits_remaining |= owner_rights_denied;
> - }
> - }
> -
> /*
> * We check privileges here because they override even DENY entries.
> */
> diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls
> deleted file mode 100644
> index e1b98cec606..00000000000
> --- a/selftest/knownfail.d/smb2.acls
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -^samba3.smb2.acls.OWNER-RIGHTS-DENY\(ad_dc\)
> -^samba3.smb2.acls.OWNER-RIGHTS-DENY\(nt4_dc\)
> --
> 2.17.2
>
>
> From d761d5ffcf9741a34daa95d1d9afd3a7dbf232c6 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 04/11] s4:torture: Add test_owner_rights_deny1().
>
> Creates a 3-element ALLOW + ALLOW + DENY ACE showing that when
> calculating maximum access already seen allow bits are not removed.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> Reviewed-by: Ralph Boehme <slow at samba.org>
> ---
> 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 ba17ba3b1d5a51f857fce917cfbc61401ebb2cd2 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Sun, 3 Mar 2019 08:33:51 +0100
> Subject: [PATCH 05/11] s4:torture: Add test_deny1().
>
> Creates a 2-element ALLOW + DENY ACE showing that when calculating
> effective permissions and maximum access already seen allow bits are not
> removed.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> 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 d9c38c42a81d06ab6c4c4fe29c02e9caadd4dc26 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Fri, 1 Mar 2019 18:57:23 +0100
> Subject: [PATCH 06/11] libcli/security: fix handling of deny type ACEs in
> access_check_max_allowed()
>
> Deny ACEs must always be evaluated against explicitly granted rights
> from previous ACEs.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> 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
>
>
> From 8a78e191a7172c0378d79d61127b5e2d68cb996f 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 07/11] s4:torture: add a test with additional bits in
> SEC_FLAG_MAXIMUM_ALLOWED
>
> 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.
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> 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 7bccce803f0..4f4538b15e3 100644
> --- a/source4/torture/smb2/acls.c
> +++ b/source4/torture/smb2/acls.c
> @@ -2914,6 +2914,115 @@ static bool test_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
> */
> @@ -2940,6 +3049,8 @@ struct torture_suite *torture_smb2_acls_init(TALLOC_CTX *ctx)
> test_owner_rights_deny1);
> torture_suite_add_1smb2_test(suite, "DENY1",
> test_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 729035bb18574f7141424035c2ab0b7ee56b2a15 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 08/11] s3:libsmb: add cli_smb2_query_mxac()
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/libsmb/cli_smb2_fnum.c | 190 +++++++++++++++++++++++++++++++++
> source3/libsmb/cli_smb2_fnum.h | 9 ++
> 2 files changed, 199 insertions(+)
>
> diff --git a/source3/libsmb/cli_smb2_fnum.c b/source3/libsmb/cli_smb2_fnum.c
> index 795dc557c45..ef36ba88ee1 100644
> --- a/source3/libsmb/cli_smb2_fnum.c
> +++ b/source3/libsmb/cli_smb2_fnum.c
> @@ -2781,6 +2781,196 @@ NTSTATUS cli_smb2_set_security_descriptor(struct cli_state *cli,
> return status;
> }
>
> +/***************************************************************
> + Wrapper that allows SMB2 to query a security descriptor.
> + Synchronous only.
> +
> +***************************************************************/
> +
> +struct cli_smb2_mxac_state {
> + struct tevent_context *ev;
> + struct cli_state *cli;
> + const char *fname;
> + struct smb2_create_blobs in_cblobs;
> + uint16_t fnum;
> + NTSTATUS status;
> + uint32_t mxac;
> +};
> +
> +static void cli_smb2_mxac_opened(struct tevent_req *subreq);
> +static void cli_smb2_mxac_closed(struct tevent_req *subreq);
> +
> +struct tevent_req *cli_smb2_query_mxac_send(TALLOC_CTX *mem_ctx,
> + struct tevent_context *ev,
> + struct cli_state *cli,
> + const char *fname)
> +{
> + struct tevent_req *req = NULL, *subreq = NULL;
> + struct cli_smb2_mxac_state *state = NULL;
> + NTSTATUS status;
> +
> + req = tevent_req_create(mem_ctx, &state, struct cli_smb2_mxac_state);
> + if (req == NULL) {
> + return NULL;
> + }
> + *state = (struct cli_smb2_mxac_state) {
> + state->ev = ev,
> + state->cli = cli,
> + state->fname = fname,
> + };
> +
> + if (smbXcli_conn_protocol(cli->conn) < PROTOCOL_SMB2_02) {
> + tevent_req_nterror(req, NT_STATUS_INVALID_PARAMETER);
> + return tevent_req_post(req, ev);
> + }
> +
> + status = smb2_create_blob_add(state,
> + &state->in_cblobs,
> + SMB2_CREATE_TAG_MXAC,
> + data_blob(NULL, 0));
> + if (tevent_req_nterror(req, status)) {
> + return tevent_req_post(req, ev);
> + }
> +
> + subreq = cli_smb2_create_fnum_send(
> + state,
> + state->ev,
> + state->cli,
> + state->fname,
> + 0, /* create_flags */
> + SMB2_IMPERSONATION_IMPERSONATION,
> + FILE_READ_ATTRIBUTES,
> + 0, /* file attributes */
> + FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE,
> + FILE_OPEN,
> + 0, /* create_options */
> + &state->in_cblobs);
> + if (tevent_req_nomem(subreq, req)) {
> + return tevent_req_post(req, ev);
> + }
> + tevent_req_set_callback(subreq, cli_smb2_mxac_opened, req);
> + return req;
> +}
> +
> +static void cli_smb2_mxac_opened(struct tevent_req *subreq)
> +{
> + struct tevent_req *req = tevent_req_callback_data(
> + subreq, struct tevent_req);
> + struct cli_smb2_mxac_state *state = tevent_req_data(
> + req, struct cli_smb2_mxac_state);
> + struct smb2_create_blobs out_cblobs = {0};
> + DATA_BLOB *mxac_blob = NULL;
> + NTSTATUS status;
> + int i;
> +
> + status = cli_smb2_create_fnum_recv(
> + subreq, &state->fnum, NULL, state, &out_cblobs);
> + TALLOC_FREE(subreq);
> +
> + if (tevent_req_nterror(req, status)) {
> + return;
> + }
> +
> + 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) {
> + state->status = NT_STATUS_INVALID_NETWORK_RESPONSE;
> + goto close;
> + }
> + if (mxac_blob->length != 8) {
> + state->status = NT_STATUS_INVALID_NETWORK_RESPONSE;
> + goto close;
> + }
> +
> + state->status = NT_STATUS(IVAL(mxac_blob->data, 0));
> + state->mxac = IVAL(mxac_blob->data, 4);
> +
> +close:
> + subreq = cli_smb2_close_fnum_send(
> + state, state->ev, state->cli, state->fnum);
> + if (tevent_req_nomem(subreq, req)) {
> + return;
> + }
> + tevent_req_set_callback(subreq, cli_smb2_mxac_closed, req);
> +
> + return;
> +}
> +
> +static void cli_smb2_mxac_closed(struct tevent_req *subreq)
> +{
> + struct tevent_req *req = tevent_req_callback_data(
> + subreq, struct tevent_req);
> + NTSTATUS status;
> +
> + status = cli_smb2_close_fnum_recv(subreq);
> + if (tevent_req_nterror(req, status)) {
> + return;
> + }
> +
> + tevent_req_done(req);
> +}
> +
> +NTSTATUS cli_smb2_query_mxac_recv(struct tevent_req *req, uint32_t *mxac)
> +{
> + struct cli_smb2_mxac_state *state = tevent_req_data(
> + req, struct cli_smb2_mxac_state);
> + NTSTATUS status;
> +
> + if (tevent_req_is_nterror(req, &status)) {
> + return status;
> + }
> +
> + if (!NT_STATUS_IS_OK(state->status)) {
> + return state->status;
> + }
> +
> + *mxac = state->mxac;
> + return NT_STATUS_OK;
> +}
> +
> +NTSTATUS cli_smb2_query_mxac(struct cli_state *cli,
> + const char *fname,
> + uint32_t *_mxac)
> +{
> + TALLOC_CTX *frame = talloc_stackframe();
> + struct tevent_context *ev = NULL;
> + struct tevent_req *req = NULL;
> + NTSTATUS status = NT_STATUS_INTERNAL_ERROR;
> + bool ok;
> +
> + 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 fail;
> + }
> +
> + ev = samba_tevent_context_init(frame);
> + if (ev == NULL) {
> + goto fail;
> + }
> + req = cli_smb2_query_mxac_send(frame, ev, cli, fname);
> + if (req == NULL) {
> + goto fail;
> + }
> + ok = tevent_req_poll_ntstatus(req, ev, &status);
> + if (!ok) {
> + goto fail;
> + }
> + status = cli_smb2_query_mxac_recv(req, _mxac);
> +
> +fail:
> + cli->raw_status = status;
> + 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..c4ca069ee96 100644
> --- a/source3/libsmb/cli_smb2_fnum.h
> +++ b/source3/libsmb/cli_smb2_fnum.h
> @@ -173,6 +173,15 @@ NTSTATUS cli_smb2_set_security_descriptor(struct cli_state *cli,
> uint16_t fnum,
> uint32_t sec_info,
> const struct security_descriptor *sd);
> +struct tevent_req *cli_smb2_query_mxac_send(TALLOC_CTX *mem_ctx,
> + struct tevent_context *ev,
> + struct cli_state *cli,
> + const char *fname);
> +NTSTATUS cli_smb2_query_mxac_recv(struct tevent_req *req,
> + uint32_t *mxac);
> +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 009309b0765ca6d4d062ddaa408894e1ca2fa7fa 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 09/11] s3:libsmb: add cli_query_mxac()
>
> Works only for SMB2.
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> 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 63e7df6c76ce443b36398d8794192f7624809d8b 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 10/11] smbcacls: add -x argument, prints maximum access
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> docs-xml/manpages/smbcacls.1.xml | 9 +++++++
> source3/utils/smbcacls.c | 40 ++++++++++++++++++++++++++++++++
> 2 files changed, 49 insertions(+)
>
> diff --git a/docs-xml/manpages/smbcacls.1.xml b/docs-xml/manpages/smbcacls.1.xml
> index 6071047682d..7f87da80329 100644
> --- a/docs-xml/manpages/smbcacls.1.xml
> +++ b/docs-xml/manpages/smbcacls.1.xml
> @@ -38,6 +38,7 @@
> <arg choice="opt">--set-security-info FLAGS</arg>
> <arg choice="opt">--sddl</arg>
> <arg choice="opt">--domain-sid SID</arg>
> + <arg choice="opt">-x|--maximum-access</arg>
> </cmdsynopsis>
> </refsynopsisdiv>
>
> @@ -189,6 +190,14 @@
> </para></listitem>
> </varlistentry>
>
> + <varlistentry>
> + <term>-x|--maximum-access</term>
> + <listitem><para>When displaying an ACL additionally query
> + the server for effective maximum permissions. Note that this
> + is only supported with SMB protocol version 2 or higher.
> + </para></listitem>
> + </varlistentry>
> +
> &stdarg.server.debug;
> &popt.common.samba;
> &popt.common.credentials;
> 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 396ef9b47087ead591d893c398f764775b464981 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Sat, 2 Mar 2019 15:37:38 +0100
> Subject: [PATCH 11/11] tests: add a simple test for smbcacls -x
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/script/tests/test_acl_xattr.sh | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/source3/script/tests/test_acl_xattr.sh b/source3/script/tests/test_acl_xattr.sh
> index cba79122045..f134ff79c91 100755
> --- a/source3/script/tests/test_acl_xattr.sh
> +++ b/source3/script/tests/test_acl_xattr.sh
> @@ -33,6 +33,22 @@ setup_remote_file() {
> $SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "lcd $PREFIX; put $fname" || exit 1
> }
>
> +smbcacls_x() {
> + local share=$1
> + local fname="$share.$$"
> +
> + # skip with SMB1
> + echo "$ADDARGS" | grep mNT1 && exit 0
> +
> + $SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD "$fname" -x || exit 1
> + mxac=$($SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD "$fname" -x | awk '/Maximum access/ {print $3}')
> +
> + echo "mxac: $mxac"
> + if test "$mxac" != "0x1f01ff" ; then
> + exit 1
> + fi
> +}
> +
> nt_affects_posix() {
> local share=$1
> local expected=$2
> @@ -122,6 +138,7 @@ nt_affects_chgrp() {
>
> testit "setup remote file tmp" setup_remote_file tmp
> testit "setup remote file ign_sysacls" setup_remote_file ign_sysacls
> +testit "smbcacls -x" smbcacls_x tmp
> testit "nt_affects_posix tmp" nt_affects_posix tmp "true"
> testit "nt_affects_posix ign_sysacls" nt_affects_posix ign_sysacls "false"
> testit "setup remote file tmp" setup_remote_file tmp
> --
> 2.17.2
>
More information about the samba-technical
mailing list