Attachment 12758 Details for Bug 12466 – Ralph's correct fix !

[patch] Ralph's correct fix !

master-12466-slow.patch (text/plain), 13.34 KB, created by Jeremy Allison on 2016-12-09 17:14:20 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2016-12-09 17:14:20 UTC

Size: 13.34 KB

patch

obsolete

>From e6c83634c5193a52e0e7e97963f5e771c65ec1d6 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@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@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
>
>[2] 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@samba.org>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Ralph Boehme <slow@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@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@samba.org>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Ralph Boehme <slow@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
>

Actions: View

Attachments on bug 12466: 12752 | 12755 | 12758 | 12763 | 12764