[PATCH] Follow-up patch for bug in dealing with "Owner Rights" ACEs when calculating maximum access
Jeremy Allison
jra at samba.org
Fri Mar 1 18:25:49 UTC 2019
On Fri, Mar 01, 2019 at 07:17:35PM +0100, Ralph Böhme wrote:
> On Fri, Mar 01, 2019 at 06:42:05PM +0100, Ralph Böhme wrote:
> > On Fri, Mar 01, 2019 at 05:59:20PM +0100, Ralph Böhme wrote:
> > > On Fri, Mar 01, 2019 at 08:42:19AM -0800, Jeremy Allison wrote:
> > > > I'll take a look, but if you've allowed these new tests
> > > > to pass whilst simplifying the algorithm I'm all in favour :-).
> > >
> > > yeah, but it's a really subtle change with security implications.
> > >
> > > I still have to do a larger rewrite, so I think you can ignore the
> > > patches for now. I'll update here once I have something in shape for
> > > curious eyes. :)
> >
> > ok, this ones looks much better. WIP patchset attached.
>
> yeah, this seems to be going in the right direction. We can split the main
> patch with the fix into two individual patches, fixing the OWNER-RIGHTS-DENY
> and OWNER-RIGHTS-DENY1 tests individually, better demonstrating the problem.
>
> And then I'd say OWNER-RIGHTS-DENY1 is a different bug.
Looking over these now. I'm happy with it being 2 bugs.
Let me know when you've gotten to a patchset you're happy
with for final review.
Cheers,
Jeremy.
> --
> Ralph Boehme, Samba Team https://samba.org/
> Samba Developer, SerNet GmbH https://sernet.de/en/samba/
> GPG-Fingerprint FAE2C6088A24252051C559E4AA1E9B7126399E46
> From b068155416184d335bce4f489f650f0807d96a6e Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Fri, 1 Mar 2019 09:48:25 +0100
> Subject: [PATCH 1/9] s3:libsmb: add cli_smb2_query_mxac()
>
> ---
> source3/libsmb/cli_smb2_fnum.c | 95 ++++++++++++++++++++++++++++++++++
> source3/libsmb/cli_smb2_fnum.h | 3 ++
> 2 files changed, 98 insertions(+)
>
> diff --git a/source3/libsmb/cli_smb2_fnum.c b/source3/libsmb/cli_smb2_fnum.c
> index 795dc557c45..8a1df16e514 100644
> --- a/source3/libsmb/cli_smb2_fnum.c
> +++ b/source3/libsmb/cli_smb2_fnum.c
> @@ -2781,6 +2781,101 @@ NTSTATUS cli_smb2_set_security_descriptor(struct cli_state *cli,
> return status;
> }
>
> +/***************************************************************
> + Wrapper that allows SMB2 to query a security descriptor.
> + Synchronous only.
> +***************************************************************/
> +
> +NTSTATUS cli_smb2_query_mxac(struct cli_state *cli,
> + const char *fname,
> + uint32_t *_mxac)
> +{
> + TALLOC_CTX *frame = talloc_stackframe();
> + struct smb2_create_blobs in_cblobs = {0};
> + struct smb2_create_blobs out_cblobs = {0};
> + DATA_BLOB *mxac_blob = NULL;
> + struct smb_create_returns cr;
> + uint16_t fnum;
> + int i;
> + NTSTATUS mxac_status;
> + NTSTATUS status;
> +
> + if (smbXcli_conn_has_async_calls(cli->conn)) {
> + /*
> + * Can't use sync call while an async call is in flight
> + */
> + status = NT_STATUS_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + if (smbXcli_conn_protocol(cli->conn) < PROTOCOL_SMB2_02) {
> + status = NT_STATUS_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + status = smb2_create_blob_add(frame,
> + &in_cblobs,
> + SMB2_CREATE_TAG_MXAC,
> + data_blob(NULL, 0));
> + if (!NT_STATUS_IS_OK(status)) {
> + goto out;
> + }
> +
> +
> + status = cli_smb2_create_fnum(
> + cli,
> + fname,
> + 0, /* create_flags */
> + SMB2_IMPERSONATION_IMPERSONATION,
> + FILE_READ_ATTRIBUTES, /* desired_access */
> + 0, /* file attributes */
> + FILE_SHARE_READ|FILE_SHARE_WRITE, /* share_access */
> + FILE_OPEN, /* create_disposition */
> + 0, /* create_options */
> + &in_cblobs,
> + &fnum,
> + &cr,
> + frame,
> + &out_cblobs);
> + if (!NT_STATUS_IS_OK(status)) {
> + goto out;
> + }
> +
> + status = cli_smb2_close_fnum(cli, fnum);
> + if (!NT_STATUS_IS_OK(status)) {
> + goto out;
> + }
> +
> + for (i = 0; i < out_cblobs.num_blobs; i++) {
> + if (strcmp(out_cblobs.blobs[i].tag, SMB2_CREATE_TAG_MXAC) != 0) {
> + continue;
> + }
> + mxac_blob = &out_cblobs.blobs[i].data;
> + break;
> + }
> + if (mxac_blob == NULL) {
> + status = NT_STATUS_INVALID_NETWORK_RESPONSE;
> + goto out;
> + }
> + if (mxac_blob->length != 8) {
> + status = NT_STATUS_INVALID_NETWORK_RESPONSE;
> + goto out;
> + }
> +
> + mxac_status = NT_STATUS(IVAL(mxac_blob->data, 0));
> + if (!NT_STATUS_IS_OK(mxac_status)) {
> + status = mxac_status;
> + goto out;
> + }
> +
> + *_mxac = IVAL(mxac_blob->data, 4);
> + status = NT_STATUS_OK;
> +
> +out:
> + TALLOC_FREE(frame);
> + return status;
> +}
> +
> /***************************************************************
> Wrapper that allows SMB2 to rename a file.
> Synchronous only.
> diff --git a/source3/libsmb/cli_smb2_fnum.h b/source3/libsmb/cli_smb2_fnum.h
> index 62ae093862f..fd57bec2938 100644
> --- a/source3/libsmb/cli_smb2_fnum.h
> +++ b/source3/libsmb/cli_smb2_fnum.h
> @@ -173,6 +173,9 @@ NTSTATUS cli_smb2_set_security_descriptor(struct cli_state *cli,
> uint16_t fnum,
> uint32_t sec_info,
> const struct security_descriptor *sd);
> +NTSTATUS cli_smb2_query_mxac(struct cli_state *cli,
> + const char *fname,
> + uint32_t *mxac);
> NTSTATUS cli_smb2_rename(struct cli_state *cli,
> const char *fname_src,
> const char *fname_dst,
> --
> 2.17.2
>
>
> From 7ab8d8c3c8dc22e875dfd30b4877e44dc6dc0329 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Fri, 1 Mar 2019 09:49:17 +0100
> Subject: [PATCH 2/9] s3:libsmb: add cli_query_mxac()
>
> Works only for SMB2. Is there a way to do this over SMB1?
> ---
> source3/libsmb/clisecdesc.c | 12 ++++++++++++
> source3/libsmb/proto.h | 4 ++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/source3/libsmb/clisecdesc.c b/source3/libsmb/clisecdesc.c
> index c11e4b322ef..f9a7a057ed6 100644
> --- a/source3/libsmb/clisecdesc.c
> +++ b/source3/libsmb/clisecdesc.c
> @@ -91,6 +91,18 @@ NTSTATUS cli_query_secdesc(struct cli_state *cli, uint16_t fnum,
> return cli_query_security_descriptor(cli, fnum, sec_info, mem_ctx, sd);
> }
>
> +NTSTATUS cli_query_mxac(struct cli_state *cli,
> + const char *filename,
> + uint32_t *mxac)
> +{
> + if (smbXcli_conn_protocol(cli->conn) < PROTOCOL_SMB2_02) {
> + return NT_STATUS_NOT_SUPPORTED;
> + }
> +
> + return cli_smb2_query_mxac(cli, filename, mxac);
> +}
> +
> +
> /****************************************************************************
> set the security descriptor for a open file
> ****************************************************************************/
> diff --git a/source3/libsmb/proto.h b/source3/libsmb/proto.h
> index b0cfcb5aa90..e1a54ef75f9 100644
> --- a/source3/libsmb/proto.h
> +++ b/source3/libsmb/proto.h
> @@ -927,6 +927,10 @@ NTSTATUS cli_set_security_descriptor(struct cli_state *cli,
> NTSTATUS cli_set_secdesc(struct cli_state *cli, uint16_t fnum,
> const struct security_descriptor *sd);
>
> +NTSTATUS cli_query_mxac(struct cli_state *cli,
> + const char *filename,
> + uint32_t *mxac);
> +
> /* The following definitions come from libsmb/clistr.c */
>
> size_t clistr_pull_talloc(TALLOC_CTX *ctx,
> --
> 2.17.2
>
>
> From c5e96348c9d9a4c828b2ac30ad364ef5a3b67b7a Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Wed, 27 Feb 2019 16:45:07 +0100
> Subject: [PATCH 3/9] smbcacls -x
>
> ---
> source3/utils/smbcacls.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/source3/utils/smbcacls.c b/source3/utils/smbcacls.c
> index a3a40e9eeb9..b61d11df860 100644
> --- a/source3/utils/smbcacls.c
> +++ b/source3/utils/smbcacls.c
> @@ -40,6 +40,7 @@ static int test_args;
> static int sddl;
> static int query_sec_info = -1;
> static int set_sec_info = -1;
> +static bool want_mxac;
>
> static const char *domain_sid = NULL;
>
> @@ -359,12 +360,33 @@ static bool set_secdesc(struct cli_state *cli, const char *filename,
> return result;
> }
>
> +/*****************************************************
> +get maximum access for a file
> +*******************************************************/
> +static int cacl_mxac(struct cli_state *cli, const char *filename)
> +{
> + NTSTATUS status;
> + uint32_t mxac;
> +
> + status = cli_query_mxac(cli, filename, &mxac);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("Failed to get mxac: %s\n", nt_errstr(status));
> + return EXIT_FAILED;
> + }
> +
> + printf("Maximum access: 0x%x\n", mxac);
> +
> + return EXIT_OK;
> +}
> +
> +
> /*****************************************************
> dump the acls for a file
> *******************************************************/
> static int cacl_dump(struct cli_state *cli, const char *filename, bool numeric)
> {
> struct security_descriptor *sd;
> + int ret;
>
> if (test_args) {
> return EXIT_OK;
> @@ -386,6 +408,13 @@ static int cacl_dump(struct cli_state *cli, const char *filename, bool numeric)
> sec_desc_print(cli, stdout, sd, numeric);
> }
>
> + if (want_mxac) {
> + ret = cacl_mxac(cli, filename);
> + if (ret != EXIT_OK) {
> + return ret;
> + }
> + }
> +
> return EXIT_OK;
> }
>
> @@ -910,6 +939,14 @@ int main(int argc, char *argv[])
> .descrip = "Set the max protocol level",
> .argDescrip = "LEVEL",
> },
> + {
> + .longName = "maximum-access",
> + .shortName = 'x',
> + .argInfo = POPT_ARG_NONE,
> + .arg = NULL,
> + .val = 'x',
> + .descrip = "Query maximum persmissions",
> + },
> POPT_COMMON_SAMBA
> POPT_COMMON_CONNECTION
> POPT_COMMON_CREDENTIALS
> @@ -975,6 +1012,9 @@ int main(int argc, char *argv[])
> case 'm':
> lp_set_cmdline("client max protocol", poptGetOptArg(pc));
> break;
> + case 'x':
> + want_mxac = true;
> + break;
> }
> }
>
> --
> 2.17.2
>
>
> From dcb7bfba3e48c94bc571c5c07fdcab96eaad6b46 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Thu, 28 Feb 2019 13:55:31 -0800
> Subject: [PATCH 4/9] s4:torture: Fix the test_owner_rights() test to show
> permissions are additive.
>
> Tested against Windows.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source4/torture/smb2/acls.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c
> index b02d74367e3..0425d87bc04 100644
> --- a/source4/torture/smb2/acls.c
> +++ b/source4/torture/smb2/acls.c
> @@ -2419,6 +2419,14 @@ static bool test_owner_rights(struct torture_context *tctx,
> sd_orig = gi.query_secdesc.out.sd;
> owner_sid = dom_sid_string(tctx, sd_orig->owner_sid);
>
> + /*
> + * Add a 2 element ACL
> + * SEC_RIGHTS_FILE_READ for the owner,
> + * SEC_FILE_WRITE_DATA for SID_OWNER_RIGHTS.
> + *
> + * Proves that the owner and SID_OWNER_RIGHTS
> + * ACE entries are additive.
> + */
> sd = security_descriptor_dacl_create(tctx, 0, NULL, NULL,
> owner_sid,
> SEC_ACE_TYPE_ACCESS_ALLOWED,
> @@ -2426,7 +2434,7 @@ static bool test_owner_rights(struct torture_context *tctx,
> 0,
> SID_OWNER_RIGHTS,
> SEC_ACE_TYPE_ACCESS_ALLOWED,
> - SEC_RIGHTS_FILE_READ,
> + SEC_FILE_WRITE_DATA,
> 0,
> NULL);
> torture_assert_not_null_goto(tctx, sd, ret, done,
> @@ -2470,7 +2478,8 @@ static bool test_owner_rights(struct torture_context *tctx,
> /* SEC_STD_DELETE comes from the parent directory */
> torture_assert_int_equal_goto(tctx,
> cr.out.maximal_access,
> - SEC_RIGHTS_FILE_READ|SEC_STD_DELETE,
> + SEC_RIGHTS_FILE_READ|SEC_STD_DELETE|
> + SEC_FILE_WRITE_DATA,
> ret, done,
> "Wrong maximum access\n");
>
> --
> 2.17.2
>
>
> From d6960c3c908a6373620ca4ff15d98a7bfd1312fc Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Thu, 28 Feb 2019 14:37:09 -0800
> Subject: [PATCH 5/9] s4:torture: Add test_owner_rights_deny().
>
> Shows that owner and SID_OWNER_RIGHTS ACE
> entries interact in max permissions requests.
>
> Tested against Windows.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> selftest/knownfail.d/smb2.acls | 2 +
> source4/torture/smb2/acls.c | 135 +++++++++++++++++++++++++++++++++
> 2 files changed, 137 insertions(+)
> create mode 100644 selftest/knownfail.d/smb2.acls
>
> diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls
> new file mode 100644
> index 00000000000..e1b98cec606
> --- /dev/null
> +++ b/selftest/knownfail.d/smb2.acls
> @@ -0,0 +1,2 @@
> +^samba3.smb2.acls.OWNER-RIGHTS-DENY\(ad_dc\)
> +^samba3.smb2.acls.OWNER-RIGHTS-DENY\(nt4_dc\)
> diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c
> index 0425d87bc04..60f05e1b99c 100644
> --- a/source4/torture/smb2/acls.c
> +++ b/source4/torture/smb2/acls.c
> @@ -2496,6 +2496,139 @@ static bool test_owner_rights(struct torture_context *tctx,
> return ret;
> }
>
> +/*
> + * test Owner Rights with a leading DENY ACE, S-1-3-4
> + */
> +static bool test_owner_rights_deny(struct torture_context *tctx,
> + struct smb2_tree *tree)
> +{
> + const char *fname = BASEDIR "\\owner_right_deny.txt";
> + struct smb2_create cr;
> + struct smb2_handle handle = {{0}};
> + union smb_fileinfo gi;
> + union smb_setfileinfo si;
> + struct security_descriptor *sd_orig = NULL;
> + struct security_descriptor *sd = NULL;
> + const char *owner_sid = NULL;
> + NTSTATUS mxac_status;
> + NTSTATUS status;
> + bool ret = true;
> +
> + smb2_deltree(tree, BASEDIR);
> +
> + ret = smb2_util_setup_dir(tctx, tree, BASEDIR);
> + torture_assert_goto(tctx, ret, ret, done,
> + "smb2_util_setup_dir failed\n");
> +
> + torture_comment(tctx, "TESTING OWNER RIGHTS DENY\n");
> +
> + cr = (struct smb2_create) {
> + .in.desired_access = SEC_STD_READ_CONTROL |
> + SEC_STD_WRITE_DAC |SEC_STD_WRITE_OWNER,
> + .in.file_attributes = FILE_ATTRIBUTE_NORMAL,
> + .in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
> + .in.create_disposition = NTCREATEX_DISP_OPEN_IF,
> + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS,
> + .in.fname = fname,
> + };
> +
> + status = smb2_create(tree, tctx, &cr);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_create failed\n");
> + handle = cr.out.file.handle;
> +
> + torture_comment(tctx, "get the original sd\n");
> +
> + gi = (union smb_fileinfo) {
> + .query_secdesc.level = RAW_FILEINFO_SEC_DESC,
> + .query_secdesc.in.file.handle = handle,
> + .query_secdesc.in.secinfo_flags = SECINFO_DACL|SECINFO_OWNER,
> + };
> +
> + status = smb2_getinfo_file(tree, tctx, &gi);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_getinfo_file failed\n");
> +
> + sd_orig = gi.query_secdesc.out.sd;
> + owner_sid = dom_sid_string(tctx, sd_orig->owner_sid);
> +
> + /*
> + * Add a 2 element ACL
> + * DENY SEC_FILE_DATA_READ for SID_OWNER_RIGHTS
> + * SEC_FILE_READ_DATA for the owner.
> + *
> + * Proves that the owner and SID_OWNER_RIGHTS
> + * ACE entries are additive.
> + */
> + sd = security_descriptor_dacl_create(tctx, 0, NULL, NULL,
> + SID_OWNER_RIGHTS,
> + SEC_ACE_TYPE_ACCESS_DENIED,
> + SEC_FILE_READ_DATA,
> + 0,
> + owner_sid,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + SEC_RIGHTS_FILE_READ,
> + 0,
> + NULL);
> + torture_assert_not_null_goto(tctx, sd, ret, done,
> + "SD create failed\n");
> +
> + si = (union smb_setfileinfo) {
> + .set_secdesc.level = RAW_SFILEINFO_SEC_DESC,
> + .set_secdesc.in.file.handle = handle,
> + .set_secdesc.in.secinfo_flags = SECINFO_DACL,
> + .set_secdesc.in.sd = sd,
> + };
> +
> + status = smb2_setinfo_file(tree, &si);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_setinfo_file failed\n");
> +
> + status = smb2_util_close(tree, handle);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_util_close failed\n");
> + ZERO_STRUCT(handle);
> +
> + cr = (struct smb2_create) {
> + .in.desired_access = SEC_STD_READ_CONTROL,
> + .in.file_attributes = FILE_ATTRIBUTE_NORMAL,
> + .in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
> + .in.create_disposition = NTCREATEX_DISP_OPEN_IF,
> + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS,
> + .in.query_maximal_access = true,
> + .in.fname = fname,
> + };
> +
> + status = smb2_create(tree, tctx, &cr);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_setinfo_file failed\n");
> + handle = cr.out.file.handle;
> +
> + mxac_status = NT_STATUS(cr.out.maximal_access_status);
> + torture_assert_ntstatus_ok_goto(tctx, mxac_status, ret, done,
> + "smb2_setinfo_file failed\n");
> +
> + /* SEC_STD_DELETE comes from the parent directory */
> + torture_assert_int_equal_goto(tctx,
> + cr.out.maximal_access,
> + ((SEC_RIGHTS_FILE_READ|SEC_STD_DELETE) &
> + (~SEC_FILE_READ_DATA)),
> + ret, done,
> + "Wrong maximum access\n");
> +
> + status = smb2_util_close(tree, handle);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_util_close failed\n");
> + ZERO_STRUCT(handle);
> +
> +done:
> + if (!smb2_util_handle_empty(handle)) {
> + smb2_util_close(tree, handle);
> + }
> + smb2_deltree(tree, BASEDIR);
> + return ret;
> +}
> +
> /*
> basic testing of SMB2 ACLs
> */
> @@ -2516,6 +2649,8 @@ struct torture_suite *torture_smb2_acls_init(TALLOC_CTX *ctx)
> #endif
> torture_suite_add_1smb2_test(suite, "ACCESSBASED", test_access_based);
> torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS", test_owner_rights);
> + torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS-DENY",
> + test_owner_rights_deny);
>
> suite->description = talloc_strdup(suite, "SMB2-ACLS tests");
>
> --
> 2.17.2
>
>
> From a59a39874915c6f4edef7e2489df6f1b58a59a80 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Thu, 28 Feb 2019 14:59:01 -0800
> Subject: [PATCH 6/9] s4:torture: Add test_owner_rights_deny1().
>
> Creates a 3-element ALLOW + ALLOW + DENY ACE
> showing that when calculating maximum access
> already seen allow bits are not removed, thus
> giving me the info I need to put the correct
> algorithm in access_check_max_allowed() in
> the next commit.
>
> Tested against Windows.
>
> Note this test can't currently pass against
> Samba as it creates a pathalogical ACL that
> cannot be mapped to a POSIX backend.
>
> Currently just mark as knownfail, but should
> I use if (torture_setting_bool(tctx, "samba3", false)
> to skip instead ?
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> selftest/knownfail.d/smb2.acls | 2 +
> source4/torture/smb2/acls.c | 150 +++++++++++++++++++++++++++++++++
> 2 files changed, 152 insertions(+)
>
> diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls
> index e1b98cec606..a01fc0097e4 100644
> --- a/selftest/knownfail.d/smb2.acls
> +++ b/selftest/knownfail.d/smb2.acls
> @@ -1,2 +1,4 @@
> ^samba3.smb2.acls.OWNER-RIGHTS-DENY\(ad_dc\)
> ^samba3.smb2.acls.OWNER-RIGHTS-DENY\(nt4_dc\)
> +^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(ad_dc\)
> +^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(nt4_dc\)
> diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c
> index 60f05e1b99c..00a625fce06 100644
> --- a/source4/torture/smb2/acls.c
> +++ b/source4/torture/smb2/acls.c
> @@ -2629,6 +2629,154 @@ static bool test_owner_rights_deny(struct torture_context *tctx,
> return ret;
> }
>
> +/*
> + * test Owner Rights with a trailing DENY ACE, S-1-3-4
> + * NOTE: Samba *cannot* currently pass this as it creates
> + * a pathalogical set of ACE's (ALLOW followed by DENY)
> + * that the posix ACL code cannot cope with. Passes
> + * against Windows.
> + *
> + * Right now mark this as selftest/knownfail.d/smb2.acls,
> + * but should I just skip this test using
> + * if (torture_setting_bool(tctx, "samba3", false) ?
> + */
> +static bool test_owner_rights_deny1(struct torture_context *tctx,
> + struct smb2_tree *tree)
> +{
> + const char *fname = BASEDIR "\\owner_right_deny1.txt";
> + struct smb2_create cr;
> + struct smb2_handle handle = {{0}};
> + union smb_fileinfo gi;
> + union smb_setfileinfo si;
> + struct security_descriptor *sd_orig = NULL;
> + struct security_descriptor *sd = NULL;
> + const char *owner_sid = NULL;
> + NTSTATUS mxac_status;
> + NTSTATUS status;
> + bool ret = true;
> +
> + smb2_deltree(tree, BASEDIR);
> +
> + ret = smb2_util_setup_dir(tctx, tree, BASEDIR);
> + torture_assert_goto(tctx, ret, ret, done,
> + "smb2_util_setup_dir failed\n");
> +
> + torture_comment(tctx, "TESTING OWNER RIGHTS DENY1\n");
> +
> + cr = (struct smb2_create) {
> + .in.desired_access = SEC_STD_READ_CONTROL |
> + SEC_STD_WRITE_DAC |SEC_STD_WRITE_OWNER,
> + .in.file_attributes = FILE_ATTRIBUTE_NORMAL,
> + .in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
> + .in.create_disposition = NTCREATEX_DISP_OPEN_IF,
> + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS,
> + .in.fname = fname,
> + };
> +
> + status = smb2_create(tree, tctx, &cr);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_create failed\n");
> + handle = cr.out.file.handle;
> +
> + torture_comment(tctx, "get the original sd\n");
> +
> + gi = (union smb_fileinfo) {
> + .query_secdesc.level = RAW_FILEINFO_SEC_DESC,
> + .query_secdesc.in.file.handle = handle,
> + .query_secdesc.in.secinfo_flags = SECINFO_DACL|SECINFO_OWNER,
> + };
> +
> + status = smb2_getinfo_file(tree, tctx, &gi);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_getinfo_file failed\n");
> +
> + sd_orig = gi.query_secdesc.out.sd;
> + owner_sid = dom_sid_string(tctx, sd_orig->owner_sid);
> +
> + /*
> + * Add a 3 element ACL
> + *
> + * SEC_RIGHTS_FILE_READ allow for owner.
> + * SEC_FILE_WRITE_DATA allow for SID-OWNER-RIGHTS.
> + * SEC_FILE_WRITE_DATA|SEC_FILE_READ_DATA) deny for SID-OWNER-RIGHTS.
> + *
> + * Shows on Windows that trailing DENY entries don't
> + * override granted permissions in max access calculations.
> + */
> + sd = security_descriptor_dacl_create(tctx, 0, NULL, NULL,
> + owner_sid,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + SEC_RIGHTS_FILE_READ,
> + 0,
> + SID_OWNER_RIGHTS,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + SEC_FILE_WRITE_DATA,
> + 0,
> + SID_OWNER_RIGHTS,
> + SEC_ACE_TYPE_ACCESS_DENIED,
> + (SEC_FILE_WRITE_DATA|
> + SEC_FILE_READ_DATA),
> + 0,
> + NULL);
> + torture_assert_not_null_goto(tctx, sd, ret, done,
> + "SD create failed\n");
> +
> + si = (union smb_setfileinfo) {
> + .set_secdesc.level = RAW_SFILEINFO_SEC_DESC,
> + .set_secdesc.in.file.handle = handle,
> + .set_secdesc.in.secinfo_flags = SECINFO_DACL,
> + .set_secdesc.in.sd = sd,
> + };
> +
> + status = smb2_setinfo_file(tree, &si);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_setinfo_file failed\n");
> +
> + status = smb2_util_close(tree, handle);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_util_close failed\n");
> + ZERO_STRUCT(handle);
> +
> + cr = (struct smb2_create) {
> + .in.desired_access = SEC_STD_READ_CONTROL,
> + .in.file_attributes = FILE_ATTRIBUTE_NORMAL,
> + .in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
> + .in.create_disposition = NTCREATEX_DISP_OPEN_IF,
> + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS,
> + .in.query_maximal_access = true,
> + .in.fname = fname,
> + };
> +
> + status = smb2_create(tree, tctx, &cr);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_setinfo_file failed\n");
> + handle = cr.out.file.handle;
> +
> + mxac_status = NT_STATUS(cr.out.maximal_access_status);
> + torture_assert_ntstatus_ok_goto(tctx, mxac_status, ret, done,
> + "smb2_setinfo_file failed\n");
> +
> + /* SEC_STD_DELETE comes from the parent directory */
> + torture_assert_int_equal_goto(tctx,
> + cr.out.maximal_access,
> + SEC_RIGHTS_FILE_READ|SEC_STD_DELETE|
> + SEC_FILE_WRITE_DATA,
> + ret, done,
> + "Wrong maximum access\n");
> +
> + status = smb2_util_close(tree, handle);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_util_close failed\n");
> + ZERO_STRUCT(handle);
> +
> +done:
> + if (!smb2_util_handle_empty(handle)) {
> + smb2_util_close(tree, handle);
> + }
> + smb2_deltree(tree, BASEDIR);
> + return ret;
> +}
> +
> /*
> basic testing of SMB2 ACLs
> */
> @@ -2651,6 +2799,8 @@ struct torture_suite *torture_smb2_acls_init(TALLOC_CTX *ctx)
> torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS", test_owner_rights);
> torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS-DENY",
> test_owner_rights_deny);
> + torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS-DENY1",
> + test_owner_rights_deny1);
>
> suite->description = talloc_strdup(suite, "SMB2-ACLS tests");
>
> --
> 2.17.2
>
>
> From ea14334b256e52688fc0282a2d7fd4f853537212 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 7/9] libcli/security: correct access check and maximum access
> calculation for Owner Rights ACEs
>
> ---
> libcli/security/access_check.c | 140 +++++++++++++++++----------------
> selftest/knownfail.d/smb2.acls | 2 -
> 2 files changed, 73 insertions(+), 69 deletions(-)
>
> diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c
> index 5d49b718f0c..54d890e0ca0 100644
> --- a/libcli/security/access_check.c
> +++ b/libcli/security/access_check.c
> @@ -109,10 +109,9 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd,
> const struct security_token *token)
> {
> uint32_t denied = 0, granted = 0;
> + bool owner = false;
> + bool owner_rights = false;
> unsigned i;
> - uint32_t owner_rights_allowed = 0;
> - uint32_t owner_rights_denied = 0;
> - bool owner_rights_default = true;
>
> if (sd->dacl == NULL) {
> if (security_token_has_sid(token, sd->owner_sid)) {
> @@ -121,26 +120,50 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd,
> return granted;
> }
>
> + if (security_token_has_sid(token, sd->owner_sid)) {
> + /*
> + * Check for explicit owner rights: if there are none, we remove
> + * the default owner right SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL
> + * from remaining_access. Otherwise we just process the
> + * explicitly granted rights when processing the ACEs.
> + */
> + owner = true;
> +
> + for (i=0; i < sd->dacl->num_aces; i++) {
> + struct security_ace *ace = &sd->dacl->aces[i];
> +
> + if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
> + continue;
> + }
> +
> + owner_rights = dom_sid_equal(&ace->trustee,
> + &global_sid_Owner_Rights);
> + if (owner_rights) {
> + break;
> + }
> + }
> + }
> +
> + if (owner && !owner_rights) {
> + granted |= SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL;
> + }
> +
> for (i = 0;i<sd->dacl->num_aces; i++) {
> struct security_ace *ace = &sd->dacl->aces[i];
> + bool owner_rights_ace = false;
>
> if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
> continue;
> }
>
> - if (dom_sid_equal(&ace->trustee, &global_sid_Owner_Rights)) {
> - if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED) {
> - owner_rights_allowed |= ace->access_mask;
> - owner_rights_default = false;
> - } else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) {
> - owner_rights_denied |= (owner_rights_allowed &
> - ace->access_mask);
> - owner_rights_default = false;
> - }
> - continue;
> + if (owner) {
> + owner_rights_ace = dom_sid_equal(
> + &ace->trustee, &global_sid_Owner_Rights);
> }
>
> - if (!security_token_has_sid(token, &ace->trustee)) {
> + if (!owner_rights_ace &&
> + !security_token_has_sid(token, &ace->trustee))
> + {
> continue;
> }
>
> @@ -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 owner = false;
> + bool owner_rights = false;
>
> *access_granted = access_desired;
> bits_remaining = access_desired;
> @@ -221,35 +227,50 @@ NTSTATUS se_access_check(const struct security_descriptor *sd,
> goto done;
> }
>
> + if (security_token_has_sid(token, sd->owner_sid)) {
> + /*
> + * Check for explicit owner rights: if there are none, we remove
> + * the default owner right SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL
> + * from remaining_access. Otherwise we just process the
> + * explicitly granted rights when processing the ACEs.
> + */
> + owner = true;
> +
> + for (i=0; i < sd->dacl->num_aces; i++) {
> + struct security_ace *ace = &sd->dacl->aces[i];
> +
> + if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
> + continue;
> + }
> +
> + owner_rights = dom_sid_equal(&ace->trustee,
> + &global_sid_Owner_Rights);
> + if (owner_rights) {
> + break;
> + }
> + }
> + }
> + if (owner && !owner_rights) {
> + bits_remaining &= ~(SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL);
> + }
> +
> /* check each ace in turn. */
> for (i=0; bits_remaining && i < sd->dacl->num_aces; i++) {
> struct security_ace *ace = &sd->dacl->aces[i];
> + bool owner_rights_ace = false;
>
> if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
> continue;
> }
>
> - /*
> - * We need the Owner Rights permissions to ensure we
> - * give or deny the correct permissions to the owner. Replace
> - * owner_rights with the perms here if it is present.
> - *
> - * We don't care if we are not the owner because that is taken
> - * care of below when we check if our token has the owner SID.
> - *
> - */
> - if (dom_sid_equal(&ace->trustee, &global_sid_Owner_Rights)) {
> - if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED) {
> - owner_rights_allowed |= ace->access_mask;
> - owner_rights_default = false;
> - } else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) {
> - owner_rights_denied |= (bits_remaining & ace->access_mask);
> - owner_rights_default = false;
> - }
> - continue;
> + if (owner) {
> + owner_rights_ace = dom_sid_equal(
> + &ace->trustee, &global_sid_Owner_Rights);
> }
>
> - if (!security_token_has_sid(token, &ace->trustee)) {
> + if (!owner_rights_ace &&
> + !security_token_has_sid(token, &ace->trustee))
> + {
> continue;
> }
>
> @@ -269,21 +290,6 @@ NTSTATUS se_access_check(const struct security_descriptor *sd,
> /* Explicitly denied bits always override */
> bits_remaining |= explicitly_denied_bits;
>
> - /* The owner always gets owner rights as defined above. */
> - if (security_token_has_sid(token, sd->owner_sid)) {
> - if (owner_rights_default) {
> - /*
> - * Just remove them, no need to check if they are
> - * there.
> - */
> - bits_remaining &= ~(SEC_STD_WRITE_DAC |
> - SEC_STD_READ_CONTROL);
> - } else {
> - bits_remaining &= ~owner_rights_allowed;
> - bits_remaining |= owner_rights_denied;
> - }
> - }
> -
> /*
> * We check privileges here because they override even DENY entries.
> */
> diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls
> index a01fc0097e4..f8541adc1b0 100644
> --- a/selftest/knownfail.d/smb2.acls
> +++ b/selftest/knownfail.d/smb2.acls
> @@ -1,4 +1,2 @@
> -^samba3.smb2.acls.OWNER-RIGHTS-DENY\(ad_dc\)
> -^samba3.smb2.acls.OWNER-RIGHTS-DENY\(nt4_dc\)
> ^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(ad_dc\)
> ^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(nt4_dc\)
> --
> 2.17.2
>
>
> From feafaabc45f4604545fd6b05a281e380e6b1d081 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 8/9] 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.
> ---
> libcli/security/access_check.c | 2 +-
> selftest/knownfail.d/smb2.acls | 2 --
> 2 files changed, 1 insertion(+), 3 deletions(-)
> delete mode 100644 selftest/knownfail.d/smb2.acls
>
> diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c
> index 54d890e0ca0..389879464d9 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 f8541adc1b0..00000000000
> --- a/selftest/knownfail.d/smb2.acls
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(ad_dc\)
> -^samba3.smb2.acls.OWNER-RIGHTS-DENY1\(nt4_dc\)
> --
> 2.17.2
>
>
> From 07800e2d4ec4ca8299c0da9aea1e1a7ada846a3c 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 9/9] s4:torture: add a test with additional bits in
> SEC_FLAG_MAXIMUM_ALLOWED
>
> ---
> source4/torture/smb2/acls.c | 111 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 111 insertions(+)
>
> diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c
> index 00a625fce06..c513b021fd3 100644
> --- a/source4/torture/smb2/acls.c
> +++ b/source4/torture/smb2/acls.c
> @@ -2777,6 +2777,115 @@ static bool test_owner_rights_deny1(struct torture_context *tctx,
> return ret;
> }
>
> +/*
> + * test SEC_FLAG_MAXIMUM_ALLOWED with not-granted access
> + *
> + * When access_mask contains SEC_FLAG_MAXIMUM_ALLOWED, the server must still
> + * proces other bits from access_mask. Eg if access_mask contains a right that
> + * the requester doesn't have, the function must validate that against the
> + * effective permissions.
> + */
> +static bool test_mxac_not_granted(struct torture_context *tctx,
> + struct smb2_tree *tree)
> +{
> + const char *fname = BASEDIR "\\test_mxac_not_granted.txt";
> + struct smb2_create cr;
> + struct smb2_handle handle = {{0}};
> + union smb_fileinfo gi;
> + union smb_setfileinfo si;
> + struct security_descriptor *sd_orig = NULL;
> + struct security_descriptor *sd = NULL;
> + const char *owner_sid = NULL;
> + NTSTATUS status;
> + bool ret = true;
> +
> + smb2_deltree(tree, BASEDIR);
> +
> + ret = smb2_util_setup_dir(tctx, tree, BASEDIR);
> + torture_assert_goto(tctx, ret, ret, done,
> + "smb2_util_setup_dir failed\n");
> +
> + torture_comment(tctx, "TESTING OWNER RIGHTS DENY\n");
> +
> + cr = (struct smb2_create) {
> + .in.desired_access = SEC_STD_READ_CONTROL |
> + SEC_STD_WRITE_DAC |SEC_STD_WRITE_OWNER,
> + .in.file_attributes = FILE_ATTRIBUTE_NORMAL,
> + .in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
> + .in.create_disposition = NTCREATEX_DISP_OPEN_IF,
> + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS,
> + .in.fname = fname,
> + };
> +
> + status = smb2_create(tree, tctx, &cr);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_create failed\n");
> + handle = cr.out.file.handle;
> +
> + torture_comment(tctx, "get the original sd\n");
> +
> + gi = (union smb_fileinfo) {
> + .query_secdesc.level = RAW_FILEINFO_SEC_DESC,
> + .query_secdesc.in.file.handle = handle,
> + .query_secdesc.in.secinfo_flags = SECINFO_DACL|SECINFO_OWNER,
> + };
> +
> + status = smb2_getinfo_file(tree, tctx, &gi);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_getinfo_file failed\n");
> +
> + sd_orig = gi.query_secdesc.out.sd;
> + owner_sid = dom_sid_string(tctx, sd_orig->owner_sid);
> +
> + sd = security_descriptor_dacl_create(tctx, 0, NULL, NULL,
> + owner_sid,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + SEC_FILE_READ_DATA,
> + 0,
> + NULL);
> + torture_assert_not_null_goto(tctx, sd, ret, done,
> + "SD create failed\n");
> +
> + si = (union smb_setfileinfo) {
> + .set_secdesc.level = RAW_SFILEINFO_SEC_DESC,
> + .set_secdesc.in.file.handle = handle,
> + .set_secdesc.in.secinfo_flags = SECINFO_DACL,
> + .set_secdesc.in.sd = sd,
> + };
> +
> + status = smb2_setinfo_file(tree, &si);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_setinfo_file failed\n");
> +
> + status = smb2_util_close(tree, handle);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_util_close failed\n");
> + ZERO_STRUCT(handle);
> +
> + cr = (struct smb2_create) {
> + .in.desired_access = SEC_FLAG_MAXIMUM_ALLOWED |
> + SEC_FILE_WRITE_DATA,
> + .in.file_attributes = FILE_ATTRIBUTE_NORMAL,
> + .in.share_access = NTCREATEX_SHARE_ACCESS_MASK,
> + .in.create_disposition = NTCREATEX_DISP_OPEN_IF,
> + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS,
> + .in.fname = fname,
> + };
> +
> + status = smb2_create(tree, tctx, &cr);
> + torture_assert_ntstatus_equal_goto(tctx, status,
> + NT_STATUS_ACCESS_DENIED,
> + ret, done,
> + "Wrong smb2_create result\n");
> +
> +done:
> + if (!smb2_util_handle_empty(handle)) {
> + smb2_util_close(tree, handle);
> + }
> + smb2_deltree(tree, BASEDIR);
> + return ret;
> +}
> +
> /*
> basic testing of SMB2 ACLs
> */
> @@ -2801,6 +2910,8 @@ struct torture_suite *torture_smb2_acls_init(TALLOC_CTX *ctx)
> test_owner_rights_deny);
> torture_suite_add_1smb2_test(suite, "OWNER-RIGHTS-DENY1",
> test_owner_rights_deny1);
> + torture_suite_add_1smb2_test(suite, "MXAC-NOT-GRANTED",
> + test_mxac_not_granted);
>
> suite->description = talloc_strdup(suite, "SMB2-ACLS tests");
>
> --
> 2.17.2
>
More information about the samba-technical
mailing list