From 3d9f9fb9249a2179d792ce9042cebbffeb4088f3 Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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. Back-ported for 4.0.x. Signed-off-by: Jeremy Allison Reviewed-by: Stefan Metzmacher Reviewed-by: David Disseldorp (Based on master commit 60f922bf1bd8816eacbb32c24793ad1f97a1d9f2) --- source3/smbd/open.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index a1b4e43..0282722 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -300,6 +300,44 @@ 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, + 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, + access_mask); +} + +/**************************************************************************** fd support routines - attempt to do a dos_open. ****************************************************************************/ @@ -3749,6 +3787,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.1.2 From a6d1dffee24c80852c8f0dffd65fd2c83ab06b1a Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 Reviewed-by: Stefan Metzmacher Reviewed-by: David Disseldorp Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Mon Nov 4 23:10:10 CET 2013 on sn-devel-104 (cherry picked from commit 65882152cc7ccaba0e7903862b99ca93594ed080) --- selftest/knownfail | 1 + source4/torture/raw/streams.c | 181 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+) diff --git a/selftest/knownfail b/selftest/knownfail index d249a25..e393635 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -128,6 +128,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 1611c64..61852f5 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.1.2