[PATCH] Fix bug #10229 - No access check verification on stream files.

Jeremy Allison jra at samba.org
Wed Oct 30 11:12:16 MDT 2013


Complete patchset including regression test for smbtorture.

Tests opening streams against a file set as ATTRIBUTE_REAONLY
and also with a DENY security descriptor.

Please review !

Cheers,

	Jeremy.
-------------- next part --------------
From 0be1f07fda4aa8f43e3bf7f786ba97f39c3ee2a7 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 28 Oct 2013 16:59:20 -0700
Subject: [PATCH 1/2] Fix bug #10229 - No access check verification on stream
 files.

https://bugzilla.samba.org/show_bug.cgi?id=10229

We need to check if the requested access mask
could be used to open the underlying file (if
it existed), as we're passing in zero for the
access mask to the base filename.

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source3/smbd/open.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 4db673a..105eb09 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -302,6 +302,46 @@ static NTSTATUS check_parent_access(struct connection_struct *conn,
 }
 
 /****************************************************************************
+ Ensure when opening a base file for a stream open that we have permissions
+ to do so given the access mask on the base file.
+****************************************************************************/
+
+static NTSTATUS check_base_file_access(struct connection_struct *conn,
+				struct smb_filename *smb_fname,
+				uint32_t access_mask)
+{
+	NTSTATUS status;
+
+	status = smbd_calculate_access_mask(conn, smb_fname,
+					false,
+					access_mask,
+					&access_mask);
+	if (!NT_STATUS_IS_OK(status)) {
+		DEBUG(10, ("smbd_calculate_access_mask "
+			"on file %s returned %s\n",
+			smb_fname_str_dbg(smb_fname),
+			nt_errstr(status)));
+		return status;
+	}
+
+	if (access_mask & (FILE_WRITE_DATA|FILE_APPEND_DATA)) {
+		uint32_t dosattrs;
+		if (!CAN_WRITE(conn)) {
+			return NT_STATUS_ACCESS_DENIED;
+		}
+		dosattrs = dos_mode(conn, smb_fname);
+		if (IS_DOS_READONLY(dosattrs)) {
+			return NT_STATUS_ACCESS_DENIED;
+		}
+	}
+
+	return smbd_check_access_rights(conn,
+					smb_fname,
+					false,
+					access_mask);
+}
+
+/****************************************************************************
  fd support routines - attempt to do a dos_open.
 ****************************************************************************/
 
@@ -3795,6 +3835,25 @@ static NTSTATUS create_file_unixpath(connection_struct *conn,
 		if (SMB_VFS_STAT(conn, smb_fname_base) == -1) {
 			DEBUG(10, ("Unable to stat stream: %s\n",
 				   smb_fname_str_dbg(smb_fname_base)));
+		} else {
+			/*
+			 * https://bugzilla.samba.org/show_bug.cgi?id=10229
+			 * We need to check if the requested access mask
+			 * could be used to open the underlying file (if
+			 * it existed), as we're passing in zero for the
+			 * access mask to the base filename.
+			 */
+			status = check_base_file_access(conn,
+							smb_fname_base,
+							access_mask);
+
+			if (!NT_STATUS_IS_OK(status)) {
+				DEBUG(10, ("Permission check "
+					"for base %s failed: "
+					"%s\n", smb_fname->base_name,
+					nt_errstr(status)));
+				goto fail;
+			}
 		}
 
 		/* Open the base file. */
-- 
1.8.4.1


From 0b19d21d5ba0ee68148e5363451be038c066adf9 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 29 Oct 2013 15:57:01 -0700
Subject: [PATCH 2/2] Add regression test for bug #10229 - No access check
 verification on stream files.

Checks against a file with attribute READONLY, and
a security descriptor denying WRITE_DATA access.

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 selftest/knownfail            |   1 +
 source4/torture/raw/streams.c | 181 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 182 insertions(+)

diff --git a/selftest/knownfail b/selftest/knownfail
index ab6d45f..1d3672e 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -148,6 +148,7 @@
 ^samba4.raw.streams.*.delete
 ^samba4.raw.streams.*.createdisp
 ^samba4.raw.streams.*.sumtab
+^samba4.raw.streams.*.perms
 ^samba4.raw.acls.INHERITFLAGS
 ^samba4.raw.acls.*.create_dir
 ^samba4.raw.acls.*.create_file
diff --git a/source4/torture/raw/streams.c b/source4/torture/raw/streams.c
index cbb7dcf..c1e502f 100644
--- a/source4/torture/raw/streams.c
+++ b/source4/torture/raw/streams.c
@@ -23,6 +23,8 @@
 #include "system/locale.h"
 #include "torture/torture.h"
 #include "libcli/raw/libcliraw.h"
+#include "libcli/security/dom_sid.h"
+#include "libcli/security/security_descriptor.h"
 #include "system/filesys.h"
 #include "libcli/libcli.h"
 #include "torture/util.h"
@@ -1885,6 +1887,184 @@ static bool test_stream_summary_tab(struct torture_context *tctx,
 	return ret;
 }
 
+/* Test how streams interact with base file permissions */
+/* Regression test for bug:
+   https://bugzilla.samba.org/show_bug.cgi?id=10229
+   bug #10229 - No access check verification on stream files.
+*/
+static bool test_stream_permissions(struct torture_context *tctx,
+					   struct smbcli_state *cli)
+{
+	NTSTATUS status;
+	bool ret = true;
+	union smb_open io;
+	const char *fname = BASEDIR "\\stream_permissions.txt";
+	const char *stream = "Stream One:$DATA";
+	const char *fname_stream;
+	union smb_fileinfo finfo;
+	union smb_setfileinfo sfinfo;
+	int fnum = -1;
+	union smb_fileinfo q;
+	union smb_setfileinfo set;
+	struct security_ace ace;
+	struct security_descriptor *sd;
+
+	torture_assert(tctx, torture_setup_dir(cli, BASEDIR),
+		"Failed to setup up test directory: " BASEDIR);
+
+	torture_comment(tctx, "(%s) testing permissions on streams\n", __location__);
+
+	fname_stream = talloc_asprintf(tctx, "%s:%s", fname, stream);
+
+	/* Create a file with a stream with attribute FILE_ATTRIBUTE_ARCHIVE. */
+	ret = create_file_with_stream(tctx, cli, fname_stream);
+	if (!ret) {
+		goto done;
+	}
+
+	ZERO_STRUCT(finfo);
+	finfo.generic.level = RAW_FILEINFO_BASIC_INFO;
+	finfo.generic.in.file.path = fname;
+	status = smb_raw_pathinfo(cli->tree, tctx, &finfo);
+	CHECK_STATUS(status, NT_STATUS_OK);
+
+	torture_assert_int_equal_goto(tctx,
+		finfo.all_info.out.attrib & ~FILE_ATTRIBUTE_NONINDEXED,
+		FILE_ATTRIBUTE_ARCHIVE, ret, done, "attrib incorrect");
+
+	/* Change the attributes on the base file name. */
+	ZERO_STRUCT(sfinfo);
+	sfinfo.generic.level = RAW_SFILEINFO_SETATTR;
+	sfinfo.generic.in.file.path        = fname;
+	sfinfo.setattr.in.attrib           = FILE_ATTRIBUTE_READONLY;
+
+	status = smb_raw_setpathinfo(cli->tree, &sfinfo);
+	CHECK_STATUS(status, NT_STATUS_OK);
+
+	/* Try and open the stream name for WRITE_DATA. Should
+	   fail with ACCESS_DENIED. */
+
+	ZERO_STRUCT(io);
+	io.generic.level = RAW_OPEN_NTCREATEX;
+	io.ntcreatex.in.root_fid.fnum = 0;
+	io.ntcreatex.in.flags = 0;
+	io.ntcreatex.in.access_mask = SEC_FILE_WRITE_DATA;
+	io.ntcreatex.in.create_options = 0;
+	io.ntcreatex.in.file_attr = 0;
+	io.ntcreatex.in.share_access = 0;
+	io.ntcreatex.in.alloc_size = 0;
+	io.ntcreatex.in.open_disposition = NTCREATEX_DISP_OPEN;
+	io.ntcreatex.in.impersonation = NTCREATEX_IMPERSONATION_ANONYMOUS;
+	io.ntcreatex.in.security_flags = 0;
+	io.ntcreatex.in.fname = fname_stream;
+
+	status = smb_raw_open(cli->tree, tctx, &io);
+	CHECK_STATUS(status, NT_STATUS_ACCESS_DENIED);
+
+	/* Change the attributes on the base file back. */
+	ZERO_STRUCT(sfinfo);
+	sfinfo.generic.level = RAW_SFILEINFO_SETATTR;
+	sfinfo.generic.in.file.path        = fname;
+	sfinfo.setattr.in.attrib           = 0;
+
+	status = smb_raw_setpathinfo(cli->tree, &sfinfo);
+	CHECK_STATUS(status, NT_STATUS_OK);
+
+	/* Re-open the file name. */
+
+	ZERO_STRUCT(io);
+	io.generic.level = RAW_OPEN_NTCREATEX;
+	io.ntcreatex.in.root_fid.fnum = 0;
+	io.ntcreatex.in.flags = 0;
+	io.ntcreatex.in.access_mask = (SEC_FILE_READ_DATA|SEC_FILE_WRITE_DATA|
+		SEC_STD_READ_CONTROL|SEC_STD_WRITE_DAC|
+		SEC_FILE_WRITE_ATTRIBUTE);
+	io.ntcreatex.in.create_options = 0;
+	io.ntcreatex.in.file_attr = 0;
+	io.ntcreatex.in.share_access = 0;
+	io.ntcreatex.in.alloc_size = 0;
+	io.ntcreatex.in.open_disposition = NTCREATEX_DISP_OPEN;
+	io.ntcreatex.in.impersonation = NTCREATEX_IMPERSONATION_ANONYMOUS;
+	io.ntcreatex.in.security_flags = 0;
+	io.ntcreatex.in.fname = fname;
+
+	status = smb_raw_open(cli->tree, tctx, &io);
+	CHECK_STATUS(status, NT_STATUS_OK);
+
+	fnum = io.ntcreatex.out.file.fnum;
+
+	/* Get the existing security descriptor. */
+	ZERO_STRUCT(q);
+	q.query_secdesc.level = RAW_FILEINFO_SEC_DESC;
+	q.query_secdesc.in.file.fnum = fnum;
+	q.query_secdesc.in.secinfo_flags =
+		SECINFO_OWNER |
+		SECINFO_GROUP |
+		SECINFO_DACL;
+	status = smb_raw_fileinfo(cli->tree, tctx, &q);
+	CHECK_STATUS(status, NT_STATUS_OK);
+	sd = q.query_secdesc.out.sd;
+
+	/* Now add a DENY WRITE security descriptor for Everyone. */
+	torture_comment(tctx, "add a new ACE to the DACL\n");
+
+	ace.type = SEC_ACE_TYPE_ACCESS_DENIED;
+	ace.flags = 0;
+	ace.access_mask = SEC_FILE_WRITE_DATA;
+	ace.trustee = *dom_sid_parse_talloc(tctx, SID_WORLD);
+
+	status = security_descriptor_dacl_add(sd, &ace);
+	CHECK_STATUS(status, NT_STATUS_OK);
+
+	/* security_descriptor_dacl_add adds to the *end* of
+	   the ace array, we need it at the start. Swap.. */
+	ace = sd->dacl->aces[0];
+	sd->dacl->aces[0] = sd->dacl->aces[sd->dacl->num_aces-1];
+	sd->dacl->aces[sd->dacl->num_aces-1] = ace;
+
+	ZERO_STRUCT(set);
+	set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC;
+	set.set_secdesc.in.file.fnum = fnum;
+	set.set_secdesc.in.secinfo_flags = SECINFO_DACL;
+	set.set_secdesc.in.sd = sd;
+
+	status = smb_raw_setfileinfo(cli->tree, &set);
+	CHECK_STATUS(status, NT_STATUS_OK);
+
+	smbcli_close(cli->tree, fnum);
+	fnum = -1;
+
+	/* Try and open the stream name for WRITE_DATA. Should
+	   fail with ACCESS_DENIED. */
+
+	ZERO_STRUCT(io);
+	io.generic.level = RAW_OPEN_NTCREATEX;
+	io.ntcreatex.in.root_fid.fnum = 0;
+	io.ntcreatex.in.flags = 0;
+	io.ntcreatex.in.access_mask = SEC_FILE_WRITE_DATA;
+	io.ntcreatex.in.create_options = 0;
+	io.ntcreatex.in.file_attr = 0;
+	io.ntcreatex.in.share_access = 0;
+	io.ntcreatex.in.alloc_size = 0;
+	io.ntcreatex.in.open_disposition = NTCREATEX_DISP_OPEN;
+	io.ntcreatex.in.impersonation = NTCREATEX_IMPERSONATION_ANONYMOUS;
+	io.ntcreatex.in.security_flags = 0;
+	io.ntcreatex.in.fname = fname_stream;
+
+	status = smb_raw_open(cli->tree, tctx, &io);
+	CHECK_STATUS(status, NT_STATUS_ACCESS_DENIED);
+
+ done:
+
+	if (fnum != -1) {
+		smbcli_close(cli->tree, fnum);
+	}
+	smbcli_unlink(cli->tree, fname);
+
+	smbcli_deltree(cli->tree, BASEDIR);
+	return ret;
+}
+
 /* 
    basic testing of streams calls
 */
@@ -1905,6 +2085,7 @@ struct torture_suite *torture_raw_streams(TALLOC_CTX *tctx)
 	    test_stream_create_disposition);
 	torture_suite_add_1smb_test(suite, "attr", test_stream_attributes);
 	torture_suite_add_1smb_test(suite, "sumtab", test_stream_summary_tab);
+	torture_suite_add_1smb_test(suite, "perms", test_stream_permissions);
 
 #if 0
 	torture_suite_add_1smb_test(suite, "LARGESTREAMINFO",
-- 
1.8.4.1



More information about the samba-technical mailing list