[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