[PATCH] Fix se_access_check() to correctly processes owner rights (S-1-3-4) DENY ace entries.
Jeremy Allison
jra at samba.org
Fri Dec 9 17:09:47 UTC 2016
On Fri, Dec 09, 2016 at 11:54:35AM +0100, Ralph Böhme wrote:
> > I think I found that this doesn't cover all cases, as it doesn't take
> > into account rights that where allowed in a previous ACE (given a
> > non-canonical ACL).
> >
> > Not sure if this is a valid ACL, but with this one your patch will get
> > an access denied when read+write data would be requested:
> >
> > [0] SID: User SID 1-5-21-something
> > TYPE: ALLOW
> > MASK: READ_DATA
> >
> > [1] SID: S-1-3-4
> > TYPE: DENY
> > MASK: READ_DATA
> >
> > [0] SID: User SID 1-5-21-something
> > TYPE: ALLOW
> > MASK: WRITE_DATA
> >
> > I'll post an alternative patch that should cover all cases later on.
>
> attached.
Really nice work Ralph, thanks ! I really appreciate you catching
and testing the additional corner case.
Pushing.
This kind of thing is what makes Samba so great ! :-) :-).
Cheers,
Jeremy.
> From e6c83634c5193a52e0e7e97963f5e771c65ec1d6 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Thu, 8 Dec 2016 10:40:18 -0800
> Subject: [PATCH 1/2] lib: security: se_access_check() incorrectly processes
> owner rights (S-1-3-4) DENY ace entries
>
> Reported and proposed fix by Shilpa K <shilpa.krishnareddy at gmail.com>.
>
> When processing DENY ACE entries for owner rights SIDs (S-1-3-4) the
> code OR's in the deny access mask bits without taking into account if
> they were being requested in the requested access mask.
>
> E.g. The current logic has:
>
> An ACL containining:
>
> [0] SID: S-1-3-4
> TYPE: DENY
> MASK: WRITE_DATA
> [1] SID: S-1-3-4
> TYPE: ALLOW
> MASK: ALLOW_ALL
>
> prohibits an open request by the owner for READ_DATA - even though this
> is explicitly allowed.
>
> Furthermore a non-canonical ACL containing:
>
> [[0] SID: User SID 1-5-21-something
> TYPE: ALLOW
> MASK: READ_DATA
>
> [1] SID: S-1-3-4
> TYPE: DENY
> MASK: READ_DATA
>
> [0] SID: User SID 1-5-21-something
> TYPE: ALLOW
> MASK: WRITE_DATA
>
> prohibits an open request by the owner for READ_DATA|WRITE_DATA - even
> though READ_DATA is explicitly allowed in ACE no 0 and is thus already
> filtered out of the "access-still-needed" mask when the deny ACE no 1 is
> evaluated.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12466
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> Reviewed-by: Ralph Boehme <slow at samba.org>
> ---
> libcli/security/access_check.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c
> index 2be5928..b4c850b 100644
> --- a/libcli/security/access_check.c
> +++ b/libcli/security/access_check.c
> @@ -220,7 +220,7 @@ NTSTATUS se_access_check(const struct security_descriptor *sd,
> owner_rights_allowed |= ace->access_mask;
> owner_rights_default = false;
> } else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) {
> - owner_rights_denied |= ace->access_mask;
> + owner_rights_denied |= (bits_remaining & ace->access_mask);
> owner_rights_default = false;
> }
> continue;
> --
> 2.7.4
>
>
> From 6c95be112b5e7f6bb524af4a4216b1fcab4b7dfc Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Thu, 8 Dec 2016 10:40:27 -0800
> Subject: [PATCH 2/2] s3: torture: Adds regression test case for
> se_access_check() owner rights issue.
>
> This test passes against Win2K12 but fails against smbd
> without the previous commit.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12466
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> Reviewed-by: Ralph Boehme <slow at samba.org>
> ---
> selftest/skip | 1 +
> source3/selftest/tests.py | 2 +-
> source3/torture/torture.c | 386 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 388 insertions(+), 1 deletion(-)
>
> diff --git a/selftest/skip b/selftest/skip
> index 0893962..1e6d311 100644
> --- a/selftest/skip
> +++ b/selftest/skip
> @@ -49,6 +49,7 @@
> ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).POSIX-OFD-LOCK # Fails against the s4 ntvfs server
> ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).POSIX-STREAM-DELETE # Fails against the s4 ntvfs server
> ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).RENAME-ACCESS # Fails against the s4 ntvfs server
> +^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).OWNER-RIGHTS # Don't test against the s4 ntvfs server anymore
> ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).PIDHIGH # Fails against the s4 ntvfs server
> ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).NTTRANS-FSCTL # Fails against the s4 ntvfs server
> ^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).SMB2-NEGPROT # Fails against the s4 ntvfs server
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index a678c77..d9d32cc 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -49,7 +49,7 @@ tests = ["FDPASS", "LOCK1", "LOCK2", "LOCK3", "LOCK4", "LOCK5", "LOCK6", "LOCK7"
> "OPLOCK1", "OPLOCK2", "OPLOCK4", "STREAMERROR",
> "DIR", "DIR1", "DIR-CREATETIME", "TCON", "TCONDEV", "RW1", "RW2", "RW3", "LARGE_READX", "RW-SIGNING",
> "OPEN", "XCOPY", "RENAME", "DELETE", "DELETE-LN", "WILDDELETE", "PROPERTIES", "W2K",
> - "TCON2", "IOCTL", "CHKPATH", "FDSESS", "CHAIN1", "CHAIN2",
> + "TCON2", "IOCTL", "CHKPATH", "FDSESS", "CHAIN1", "CHAIN2", "OWNER-RIGHTS",
> "CHAIN3", "PIDHIGH",
> "GETADDRINFO", "UID-REGRESSION-TEST", "SHORTNAME-TEST",
> "CASE-INSENSITIVE-CREATE", "SMB2-BASIC", "NTTRANS-FSCTL", "SMB2-NEGPROT",
> diff --git a/source3/torture/torture.c b/source3/torture/torture.c
> index ba50462..b3cd8e9 100644
> --- a/source3/torture/torture.c
> +++ b/source3/torture/torture.c
> @@ -5086,6 +5086,391 @@ static bool run_rename_access(int dummy)
> return false;
> }
>
> +/*
> + Test owner rights ACE.
> + */
> +static bool run_owner_rights(int dummy)
> +{
> + static struct cli_state *cli = NULL;
> + const char *fname = "owner_rights.txt";
> + uint16_t fnum = (uint16_t)-1;
> + struct security_descriptor *sd = NULL;
> + struct security_descriptor *newsd = NULL;
> + NTSTATUS status;
> + TALLOC_CTX *frame = NULL;
> +
> + frame = talloc_stackframe();
> + printf("starting owner rights test\n");
> +
> + /* Windows connection. */
> + if (!torture_open_connection(&cli, 0)) {
> + goto fail;
> + }
> +
> + smbXcli_conn_set_sockopt(cli->conn, sockops);
> +
> + /* Start with a clean slate. */
> + cli_unlink(cli, fname, FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
> +
> + /* Create the test file. */
> + /* Now try and open for read and write-dac. */
> + status = cli_ntcreate(cli,
> + fname,
> + 0,
> + GENERIC_ALL_ACCESS,
> + FILE_ATTRIBUTE_NORMAL,
> + FILE_SHARE_READ|FILE_SHARE_WRITE|
> + FILE_SHARE_DELETE,
> + FILE_CREATE,
> + 0,
> + 0,
> + &fnum,
> + NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("Create of %s - %s\n", fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + /* Get the original SD. */
> + status = cli_query_secdesc(cli,
> + fnum,
> + frame,
> + &sd);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("cli_query_secdesc failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + /*
> + * Add an "owner-rights" ACE denying WRITE_DATA,
> + * and an "owner-rights" ACE allowing READ_DATA.
> + */
> +
> + newsd = security_descriptor_dacl_create(frame,
> + 0,
> + NULL,
> + NULL,
> + SID_OWNER_RIGHTS,
> + SEC_ACE_TYPE_ACCESS_DENIED,
> + FILE_WRITE_DATA,
> + 0,
> + SID_OWNER_RIGHTS,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + FILE_READ_DATA,
> + 0,
> + NULL);
> + if (newsd == NULL) {
> + goto fail;
> + }
> + sd->dacl = security_acl_concatenate(frame,
> + newsd->dacl,
> + sd->dacl);
> + if (sd->dacl == NULL) {
> + goto fail;
> + }
> + status = cli_set_secdesc(cli, fnum, sd);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("cli_set_secdesc failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> + status = cli_close(cli, fnum);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("close failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> + fnum = (uint16_t)-1;
> +
> + /* Try and open for FILE_WRITE_DATA */
> + status = cli_ntcreate(cli,
> + fname,
> + 0,
> + FILE_WRITE_DATA,
> + FILE_ATTRIBUTE_NORMAL,
> + FILE_SHARE_READ|FILE_SHARE_WRITE|
> + FILE_SHARE_DELETE,
> + FILE_OPEN,
> + 0,
> + 0,
> + &fnum,
> + NULL);
> + if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
> + printf("Open of %s - %s\n", fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + /* Now try and open for FILE_READ_DATA */
> + status = cli_ntcreate(cli,
> + fname,
> + 0,
> + FILE_READ_DATA,
> + FILE_ATTRIBUTE_NORMAL,
> + FILE_SHARE_READ|FILE_SHARE_WRITE|
> + FILE_SHARE_DELETE,
> + FILE_OPEN,
> + 0,
> + 0,
> + &fnum,
> + NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("Open of %s - %s\n", fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + status = cli_close(cli, fnum);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("close failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + /* Restore clean slate. */
> + TALLOC_FREE(sd);
> + cli_unlink(cli, fname, FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
> +
> + /* Create the test file. */
> + status = cli_ntcreate(cli,
> + fname,
> + 0,
> + GENERIC_ALL_ACCESS,
> + FILE_ATTRIBUTE_NORMAL,
> + FILE_SHARE_READ|FILE_SHARE_WRITE|
> + FILE_SHARE_DELETE,
> + FILE_CREATE,
> + 0,
> + 0,
> + &fnum,
> + NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("Create of %s - %s\n", fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + /* Get the original SD. */
> + status = cli_query_secdesc(cli,
> + fnum,
> + frame,
> + &sd);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("cli_query_secdesc failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + /*
> + * Add an "owner-rights ACE denying WRITE_DATA,
> + * and an "owner-rights ACE allowing READ_DATA|WRITE_DATA.
> + */
> +
> + newsd = security_descriptor_dacl_create(frame,
> + 0,
> + NULL,
> + NULL,
> + SID_OWNER_RIGHTS,
> + SEC_ACE_TYPE_ACCESS_DENIED,
> + FILE_WRITE_DATA,
> + 0,
> + SID_OWNER_RIGHTS,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + FILE_READ_DATA|FILE_WRITE_DATA,
> + 0,
> + NULL);
> + if (newsd == NULL) {
> + goto fail;
> + }
> + sd->dacl = security_acl_concatenate(frame,
> + newsd->dacl,
> + sd->dacl);
> + if (sd->dacl == NULL) {
> + goto fail;
> + }
> + status = cli_set_secdesc(cli, fnum, sd);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("cli_set_secdesc failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> + status = cli_close(cli, fnum);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("close failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> + fnum = (uint16_t)-1;
> +
> + /* Try and open for FILE_WRITE_DATA */
> + status = cli_ntcreate(cli,
> + fname,
> + 0,
> + FILE_WRITE_DATA,
> + FILE_ATTRIBUTE_NORMAL,
> + FILE_SHARE_READ|FILE_SHARE_WRITE|
> + FILE_SHARE_DELETE,
> + FILE_OPEN,
> + 0,
> + 0,
> + &fnum,
> + NULL);
> + if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
> + printf("Open of %s - %s\n", fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + /* Now try and open for FILE_READ_DATA */
> + status = cli_ntcreate(cli,
> + fname,
> + 0,
> + FILE_READ_DATA,
> + FILE_ATTRIBUTE_NORMAL,
> + FILE_SHARE_READ|FILE_SHARE_WRITE|
> + FILE_SHARE_DELETE,
> + FILE_OPEN,
> + 0,
> + 0,
> + &fnum,
> + NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("Open of %s - %s\n", fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + status = cli_close(cli, fnum);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("close failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + /* Restore clean slate. */
> + TALLOC_FREE(sd);
> + cli_unlink(cli, fname, FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
> +
> +
> + /* Create the test file. */
> + status = cli_ntcreate(cli,
> + fname,
> + 0,
> + GENERIC_ALL_ACCESS,
> + FILE_ATTRIBUTE_NORMAL,
> + FILE_SHARE_READ|FILE_SHARE_WRITE|
> + FILE_SHARE_DELETE,
> + FILE_CREATE,
> + 0,
> + 0,
> + &fnum,
> + NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("Create of %s - %s\n", fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + /* Get the original SD. */
> + status = cli_query_secdesc(cli,
> + fnum,
> + frame,
> + &sd);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("cli_query_secdesc failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + /*
> + * Add an "authenticated users" ACE allowing READ_DATA,
> + * add an "owner-rights" denying READ_DATA,
> + * and an "authenticated users" ACE allowing WRITE_DATA.
> + */
> +
> + newsd = security_descriptor_dacl_create(frame,
> + 0,
> + NULL,
> + NULL,
> + SID_NT_AUTHENTICATED_USERS,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + FILE_READ_DATA,
> + 0,
> + SID_OWNER_RIGHTS,
> + SEC_ACE_TYPE_ACCESS_DENIED,
> + FILE_READ_DATA,
> + 0,
> + SID_NT_AUTHENTICATED_USERS,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + FILE_WRITE_DATA,
> + 0,
> + NULL);
> + if (newsd == NULL) {
> + printf("newsd == NULL\n");
> + goto fail;
> + }
> + sd->dacl = security_acl_concatenate(frame,
> + newsd->dacl,
> + sd->dacl);
> + if (sd->dacl == NULL) {
> + printf("sd->dacl == NULL\n");
> + goto fail;
> + }
> + status = cli_set_secdesc(cli, fnum, sd);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("cli_set_secdesc failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> + status = cli_close(cli, fnum);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("close failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> + fnum = (uint16_t)-1;
> +
> + /* Now try and open for FILE_READ_DATA|FILE_WRITE_DATA */
> + status = cli_ntcreate(cli,
> + fname,
> + 0,
> + FILE_READ_DATA|FILE_WRITE_DATA,
> + FILE_ATTRIBUTE_NORMAL,
> + FILE_SHARE_READ|FILE_SHARE_WRITE|
> + FILE_SHARE_DELETE,
> + FILE_OPEN,
> + 0,
> + 0,
> + &fnum,
> + NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("Open of %s - %s\n", fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + status = cli_close(cli, fnum);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("close failed for %s (%s)\n",
> + fname, nt_errstr(status));
> + goto fail;
> + }
> +
> + cli_unlink(cli, fname,
> + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
> +
> + TALLOC_FREE(frame);
> + return true;
> +
> + fail:
> +
> + if (cli) {
> + if (fnum != -1) {
> + cli_close(cli, fnum);
> + }
> + cli_unlink(cli, fname,
> + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
> + torture_close_connection(cli);
> + }
> +
> + TALLOC_FREE(frame);
> + return false;
> +}
> +
> static bool run_pipe_number(int dummy)
> {
> struct cli_state *cli1;
> @@ -10645,6 +11030,7 @@ static struct {
> {"XCOPY", run_xcopy, 0},
> {"RENAME", run_rename, 0},
> {"RENAME-ACCESS", run_rename_access, 0},
> + {"OWNER-RIGHTS", run_owner_rights, 0},
> {"DELETE", run_deletetest, 0},
> {"WILDDELETE", run_wild_deletetest, 0},
> {"DELETE-LN", run_deletetest_ln, 0},
> --
> 2.7.4
>
More information about the samba-technical
mailing list