Jeremy Allison
2016-Aug-26 21:46 UTC
[Samba] Issue with acl_xattr:ignore system acls in 4.5rc2
On Fri, Aug 26, 2016 at 06:44:05PM +0200, Ralph Böhme wrote:> > Cheerio! > -slowStill reviewing this - but a few things that will need changing: When adding the validate_nt_acl_blob() function in [PATCH 06/12] vfs_acl_common: move the ACL blob validation to a helper function this makes some of the existing function names in debug statements incorrect. Eg. validate_nt_acl_blob() has debug statements: 688 DEBUG(10, ("get_nt_acl_internal: ACL blob revision " 689 "mismatch (%u) for file %s\n", 690 (unsigned int)hash_type, 691 smb_fname->base_name)); 692 TALLOC_FREE(psd_blob); 693 return NT_STATUS_OK; 694 } 695 696 /* determine which type of xattr we got */ 697 if (hash_type != XATTR_SD_HASH_TYPE_SHA256) { 698 DEBUG(10, ("get_nt_acl_internal: ACL blob hash type " 699 "(%u) unexpected for file %s\n", 700 (unsigned int)hash_type, 701 smb_fname->base_name)); 702 TALLOC_FREE(psd_blob); 768 if (!NT_STATUS_IS_OK(status)) { 769 DEBUG(10, ("get_nt_acl_internal: get_next_acl for file %s " 770 "returned %s\n", 771 smb_fname->base_name, 772 nt_errstr(status))); 773 goto fail; 784 if (memcmp(&hash[0], &hash_tmp[0], XATTR_SD_HASH_SIZE) == 0) { 785 /* Hash matches, return blob sd. */ 786 DEBUG(10, ("get_nt_acl_internal: blob hash " 787 "matches for file %s\n", 788 smb_fname->base_name )); 789 *ppsd = psd_blob; 794 DEBUG(10, ("get_nt_acl_internal: blob hash " 795 "does not match for file %s - returning " 796 "file system SD mapping.\n", 797 smb_fname->base_name )); 798 799 if (DEBUGLEVEL >= 10) { 800 DEBUG(10,("get_nt_acl_internal: acl for blob hash for %s is:\n", 801 smb_fname->base_name )); 802 NDR_PRINT_DEBUG(security_descriptor, psd_fs); These are gonna need fixing to remove the function names from the debugs (they are automatically added now). Still reviewing...> From dd4665a7930fad64ff649110cb087b69b08464f4 Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Wed, 24 Aug 2016 10:04:24 +0200 > Subject: [PATCH 01/12] Revert "vfs_acl_xattr: objects without NT ACL xattr" > > This reverts commit 961c4b591bb102751079d9cc92d7aa1c37f1958c. > > Subsequent commits will add the same functionality as an optional > feature. > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > source3/modules/vfs_acl_common.c | 43 ++++++++++++++++++++++++++++++++++++---- > 1 file changed, 39 insertions(+), 4 deletions(-) > > diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c > index 2fda938e..a287945 100644 > --- a/source3/modules/vfs_acl_common.c > +++ b/source3/modules/vfs_acl_common.c > @@ -379,10 +379,12 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx, > gid_to_sid(&group_sid, psbuf->st_ex_gid); > > /* > - * We provide 2 ACEs: > - * - Owner > - * - NT System > - */ > + We provide up to 4 ACEs > + - Owner > + - Group > + - Everyone > + - NT System > + */ > > if (mode & S_IRUSR) { > if (mode & S_IWUSR) { > @@ -402,6 +404,39 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx, > 0); > idx++; > > + access_mask = 0; > + if (mode & S_IRGRP) { > + access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE; > + } > + if (mode & S_IWGRP) { > + /* note that delete is not granted - this matches posix behaviour */ > + access_mask |= SEC_RIGHTS_FILE_WRITE; > + } > + if (access_mask) { > + init_sec_ace(&aces[idx], > + &group_sid, > + SEC_ACE_TYPE_ACCESS_ALLOWED, > + access_mask, > + 0); > + idx++; > + } > + > + access_mask = 0; > + if (mode & S_IROTH) { > + access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE; > + } > + if (mode & S_IWOTH) { > + access_mask |= SEC_RIGHTS_FILE_WRITE; > + } > + if (access_mask) { > + init_sec_ace(&aces[idx], > + &global_sid_World, > + SEC_ACE_TYPE_ACCESS_ALLOWED, > + access_mask, > + 0); > + idx++; > + } > + > init_sec_ace(&aces[idx], > &global_sid_System, > SEC_ACE_TYPE_ACCESS_ALLOWED, > -- > 2.7.4 > > > From ac9828d7fcc48af51f59f7b271210a6328abed0f Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Tue, 23 Aug 2016 13:08:12 +0200 > Subject: [PATCH 02/12] vfs_acl_common: rename psd to psd_blob in > get_nt_acl_internal() > > This makes it explicit where the SD is originating from. No change in > behaviour. > > This just paves the way for a later change that will simplify the whole > logic and talloc hierarchy, therefor this also strictly renames the > occurences after the out label. > > Logically, behind the out label, we're dealing with a variable that > points to what we're going to return, so the name psd_blob is > misleading, but I'm desperately trying to avoid logic changes in this > commit and therefor I'm just strictly renaming. > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > source3/modules/vfs_acl_common.c | 58 ++++++++++++++++++++-------------------- > 1 file changed, 29 insertions(+), 29 deletions(-) > > diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c > index a287945..1adc875 100644 > --- a/source3/modules/vfs_acl_common.c > +++ b/source3/modules/vfs_acl_common.c > @@ -488,7 +488,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > uint8_t sys_acl_hash[XATTR_SD_HASH_SIZE]; > uint8_t hash_tmp[XATTR_SD_HASH_SIZE]; > uint8_t sys_acl_hash_tmp[XATTR_SD_HASH_SIZE]; > - struct security_descriptor *psd = NULL; > + struct security_descriptor *psd_blob = NULL; > struct security_descriptor *pdesc_next = NULL; > const struct smb_filename *smb_fname = NULL; > bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn), > @@ -509,25 +509,25 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > if (!NT_STATUS_IS_OK(status)) { > DEBUG(10, ("get_nt_acl_internal: get_acl_blob returned %s\n", > nt_errstr(status))); > - psd = NULL; > + psd_blob = NULL; > goto out; > } else { > - status = parse_acl_blob(&blob, mem_ctx, &psd, > + status = parse_acl_blob(&blob, mem_ctx, &psd_blob, > &hash_type, &xattr_version, &hash[0], &sys_acl_hash[0]); > if (!NT_STATUS_IS_OK(status)) { > DEBUG(10, ("parse_acl_blob returned %s\n", > nt_errstr(status))); > - psd = NULL; > + psd_blob = NULL; > goto out; > } > } > > - /* Ensure we don't leak psd if we don't choose it. > + /* Ensure we don't leak psd_blob if we don't choose it. > * > * We don't allocate it onto frame as it is preferred not to > * steal from a talloc pool. > */ > - talloc_steal(frame, psd); > + talloc_steal(frame, psd_blob); > > /* determine which type of xattr we got */ > switch (xattr_version) { > @@ -550,8 +550,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > "mismatch (%u) for file %s\n", > (unsigned int)hash_type, > smb_fname->base_name)); > - TALLOC_FREE(psd); > - psd = NULL; > + TALLOC_FREE(psd_blob); > + psd_blob = NULL; > goto out; > } > > @@ -561,8 +561,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > "(%u) unexpected for file %s\n", > (unsigned int)hash_type, > smb_fname->base_name)); > - TALLOC_FREE(psd); > - psd = NULL; > + TALLOC_FREE(psd_blob); > + psd_blob = NULL; > goto out; > } > > @@ -645,8 +645,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > > status = hash_sd_sha256(pdesc_next, hash_tmp); > if (!NT_STATUS_IS_OK(status)) { > - TALLOC_FREE(psd); > - psd = pdesc_next; > + TALLOC_FREE(psd_blob); > + psd_blob = pdesc_next; > goto out; > } > > @@ -670,12 +670,12 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > NDR_PRINT_DEBUG(security_descriptor, pdesc_next); > } > > - TALLOC_FREE(psd); > - psd = pdesc_next; > + TALLOC_FREE(psd_blob); > + psd_blob = pdesc_next; > } > out: > > - if (psd == NULL) { > + if (psd_blob == NULL) { > /* Get the full underlying sd, as we failed to get the > * blob for the hash, or the revision/hash type wasn't > * known */ > @@ -708,10 +708,10 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > * steal from a talloc pool. > */ > talloc_steal(frame, pdesc_next); > - psd = pdesc_next; > + psd_blob = pdesc_next; > } > > - if (psd != pdesc_next) { > + if (psd_blob != pdesc_next) { > /* We're returning the blob, throw > * away the filesystem SD. */ > TALLOC_FREE(pdesc_next); > @@ -764,20 +764,20 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > status = make_default_filesystem_acl(mem_ctx, > smb_fname->base_name, > psbuf, > - &psd); > + &psd_blob); > if (!NT_STATUS_IS_OK(status)) { > TALLOC_FREE(frame); > return status; > } > } else { > if (is_directory && > - !sd_has_inheritable_components(psd, > + !sd_has_inheritable_components(psd_blob, > true)) { > status = add_directory_inheritable_components( > handle, > smb_fname->base_name, > psbuf, > - psd); > + psd_blob); > if (!NT_STATUS_IS_OK(status)) { > TALLOC_FREE(frame); > return status; > @@ -787,23 +787,23 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > the ~SEC_DESC_DACL_PROTECTED bit, as ACLs > can't be inherited in this way under POSIX. > Remove it for Windows-style ACLs. */ > - psd->type &= ~SEC_DESC_DACL_PROTECTED; > + psd_blob->type &= ~SEC_DESC_DACL_PROTECTED; > } > } > > if (!(security_info & SECINFO_OWNER)) { > - psd->owner_sid = NULL; > + psd_blob->owner_sid = NULL; > } > if (!(security_info & SECINFO_GROUP)) { > - psd->group_sid = NULL; > + psd_blob->group_sid = NULL; > } > if (!(security_info & SECINFO_DACL)) { > - psd->type &= ~SEC_DESC_DACL_PRESENT; > - psd->dacl = NULL; > + psd_blob->type &= ~SEC_DESC_DACL_PRESENT; > + psd_blob->dacl = NULL; > } > if (!(security_info & SECINFO_SACL)) { > - psd->type &= ~SEC_DESC_SACL_PRESENT; > - psd->sacl = NULL; > + psd_blob->type &= ~SEC_DESC_SACL_PRESENT; > + psd_blob->sacl = NULL; > } > > TALLOC_FREE(blob.data); > @@ -811,11 +811,11 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > if (DEBUGLEVEL >= 10) { > DEBUG(10,("get_nt_acl_internal: returning acl for %s is:\n", > smb_fname->base_name )); > - NDR_PRINT_DEBUG(security_descriptor, psd); > + NDR_PRINT_DEBUG(security_descriptor, psd_blob); > } > > /* The VFS API is that the ACL is expected to be on mem_ctx */ > - *ppdesc = talloc_move(mem_ctx, &psd); > + *ppdesc = talloc_move(mem_ctx, &psd_blob); > > TALLOC_FREE(frame); > return NT_STATUS_OK; > -- > 2.7.4 > > > From bf5cd7cb4aac0733c8634b62ad29c128a9b3f451 Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Tue, 23 Aug 2016 13:11:24 +0200 > Subject: [PATCH 03/12] vfs_acl_common: rename pdesc_next to psd_fs > > In most realistic cases the "next" VFS op will return the permissions > from the filesystem. This rename makes it explicit where the SD is > originating from. No change in behaviour. > > This just paves the way for a later change that will simplify the whole > logic and talloc hierarchy. > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > source3/modules/vfs_acl_common.c | 34 +++++++++++++++++----------------- > 1 file changed, 17 insertions(+), 17 deletions(-) > > diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c > index 1adc875..fb01bf4 100644 > --- a/source3/modules/vfs_acl_common.c > +++ b/source3/modules/vfs_acl_common.c > @@ -489,7 +489,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > uint8_t hash_tmp[XATTR_SD_HASH_SIZE]; > uint8_t sys_acl_hash_tmp[XATTR_SD_HASH_SIZE]; > struct security_descriptor *psd_blob = NULL; > - struct security_descriptor *pdesc_next = NULL; > + struct security_descriptor *psd_fs = NULL; > const struct smb_filename *smb_fname = NULL; > bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn), > ACL_MODULE_NAME, > @@ -618,13 +618,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > fsp, > HASH_SECURITY_INFO, > mem_ctx, > - &pdesc_next); > + &psd_fs); > } else { > status = SMB_VFS_NEXT_GET_NT_ACL(handle, > smb_fname, > HASH_SECURITY_INFO, > mem_ctx, > - &pdesc_next); > + &psd_fs); > } > > if (!NT_STATUS_IS_OK(status)) { > @@ -636,17 +636,17 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > return status; > } > > - /* Ensure we don't leak psd_next if we don't choose it. > + /* Ensure we don't leak psd_fs if we don't choose it. > * > * We don't allocate it onto frame as it is preferred not to > * steal from a talloc pool. > */ > - talloc_steal(frame, pdesc_next); > + talloc_steal(frame, psd_fs); > > - status = hash_sd_sha256(pdesc_next, hash_tmp); > + status = hash_sd_sha256(psd_fs, hash_tmp); > if (!NT_STATUS_IS_OK(status)) { > TALLOC_FREE(psd_blob); > - psd_blob = pdesc_next; > + psd_blob = psd_fs; > goto out; > } > > @@ -667,11 +667,11 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > if (DEBUGLEVEL >= 10) { > DEBUG(10,("get_nt_acl_internal: acl for blob hash for %s is:\n", > smb_fname->base_name )); > - NDR_PRINT_DEBUG(security_descriptor, pdesc_next); > + NDR_PRINT_DEBUG(security_descriptor, psd_fs); > } > > TALLOC_FREE(psd_blob); > - psd_blob = pdesc_next; > + psd_blob = psd_fs; > } > out: > > @@ -684,13 +684,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > fsp, > security_info, > mem_ctx, > - &pdesc_next); > + &psd_fs); > } else { > status = SMB_VFS_NEXT_GET_NT_ACL(handle, > smb_fname, > security_info, > mem_ctx, > - &pdesc_next); > + &psd_fs); > } > > if (!NT_STATUS_IS_OK(status)) { > @@ -702,19 +702,19 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > return status; > } > > - /* Ensure we don't leak psd_next if we don't choose it. > + /* Ensure we don't leak psd_fs if we don't choose it. > * > * We don't allocate it onto frame as it is preferred not to > * steal from a talloc pool. > */ > - talloc_steal(frame, pdesc_next); > - psd_blob = pdesc_next; > + talloc_steal(frame, psd_fs); > + psd_blob = psd_fs; > } > > - if (psd_blob != pdesc_next) { > + if (psd_blob != psd_fs) { > /* We're returning the blob, throw > * away the filesystem SD. */ > - TALLOC_FREE(pdesc_next); > + TALLOC_FREE(psd_fs); > } else { > SMB_STRUCT_STAT sbuf; > SMB_STRUCT_STAT *psbuf = &sbuf; > @@ -760,7 +760,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > is_directory = S_ISDIR(psbuf->st_ex_mode); > > if (ignore_file_system_acl) { > - TALLOC_FREE(pdesc_next); > + TALLOC_FREE(psd_fs); > status = make_default_filesystem_acl(mem_ctx, > smb_fname->base_name, > psbuf, > -- > 2.7.4 > > > From b77ee317be4b59aaffe13aaddd3262ec64ecf914 Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Tue, 23 Aug 2016 13:14:50 +0200 > Subject: [PATCH 04/12] vfs_acl_common: remove redundant NULL assignment > > The variables are already set to NULL by TALLOC_FREE. > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > source3/modules/vfs_acl_common.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c > index fb01bf4..0c40f37 100644 > --- a/source3/modules/vfs_acl_common.c > +++ b/source3/modules/vfs_acl_common.c > @@ -551,7 +551,6 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > (unsigned int)hash_type, > smb_fname->base_name)); > TALLOC_FREE(psd_blob); > - psd_blob = NULL; > goto out; > } > > @@ -562,7 +561,6 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > (unsigned int)hash_type, > smb_fname->base_name)); > TALLOC_FREE(psd_blob); > - psd_blob = NULL; > goto out; > } > > -- > 2.7.4 > > > From f199343ccafc8ff3c620be730d7a320559733018 Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Tue, 23 Aug 2016 17:07:20 +0200 > Subject: [PATCH 05/12] vfs_acl_common: simplify ACL logic, cleanup and talloc > hierarchy > > No change in behaviour (hopefully! :-). This paves the way for moving > the ACL blob validation to a helper function in the next commit. > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > source3/modules/vfs_acl_common.c | 131 ++++++++++++++++++--------------------- > 1 file changed, 61 insertions(+), 70 deletions(-) > > diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c > index 0c40f37..66c58e7 100644 > --- a/source3/modules/vfs_acl_common.c > +++ b/source3/modules/vfs_acl_common.c > @@ -488,6 +488,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > uint8_t sys_acl_hash[XATTR_SD_HASH_SIZE]; > uint8_t hash_tmp[XATTR_SD_HASH_SIZE]; > uint8_t sys_acl_hash_tmp[XATTR_SD_HASH_SIZE]; > + struct security_descriptor *psd = NULL; > struct security_descriptor *psd_blob = NULL; > struct security_descriptor *psd_fs = NULL; > const struct smb_filename *smb_fname = NULL; > @@ -495,7 +496,9 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > ACL_MODULE_NAME, > "ignore system acls", > false); > - TALLOC_CTX *frame = talloc_stackframe(); > + char *sys_acl_blob_description = NULL; > + DATA_BLOB sys_acl_blob = { 0 }; > + bool psd_is_from_fs = false; > > if (fsp && smb_fname_in == NULL) { > smb_fname = fsp->fsp_name; > @@ -505,11 +508,10 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > > DEBUG(10, ("get_nt_acl_internal: name=%s\n", smb_fname->base_name)); > > - status = get_acl_blob(frame, handle, fsp, smb_fname, &blob); > + status = get_acl_blob(mem_ctx, handle, fsp, smb_fname, &blob); > if (!NT_STATUS_IS_OK(status)) { > DEBUG(10, ("get_nt_acl_internal: get_acl_blob returned %s\n", > nt_errstr(status))); > - psd_blob = NULL; > goto out; > } else { > status = parse_acl_blob(&blob, mem_ctx, &psd_blob, > @@ -517,17 +519,12 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > if (!NT_STATUS_IS_OK(status)) { > DEBUG(10, ("parse_acl_blob returned %s\n", > nt_errstr(status))); > - psd_blob = NULL; > + TALLOC_FREE(blob.data); > goto out; > } > } > > - /* Ensure we don't leak psd_blob if we don't choose it. > - * > - * We don't allocate it onto frame as it is preferred not to > - * steal from a talloc pool. > - */ > - talloc_steal(frame, psd_blob); > + TALLOC_FREE(blob.data); > > /* determine which type of xattr we got */ > switch (xattr_version) { > @@ -537,10 +534,14 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > * require confirmation of the hash. In particular, > * the NTVFS file server uses version 1, but > * 'samba-tool ntacl' can set these as well */ > + psd = psd_blob; > + psd_blob = NULL; > goto out; > case 3: > case 4: > if (ignore_file_system_acl) { > + psd = psd_blob; > + psd_blob = NULL; > goto out; > } > > @@ -569,20 +570,18 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > case 4: > { > int ret; > - char *sys_acl_blob_description; > - DATA_BLOB sys_acl_blob; > if (fsp) { > /* Get the full underlying sd, then hash. */ > ret = SMB_VFS_NEXT_SYS_ACL_BLOB_GET_FD(handle, > fsp, > - frame, > + mem_ctx, > &sys_acl_blob_description, > &sys_acl_blob); > } else { > /* Get the full underlying sd, then hash. */ > ret = SMB_VFS_NEXT_SYS_ACL_BLOB_GET_FILE(handle, > smb_fname->base_name, > - frame, > + mem_ctx, > &sys_acl_blob_description, > &sys_acl_blob); > } > @@ -592,16 +591,20 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > if (ret == 0) { > status = hash_blob_sha256(sys_acl_blob, sys_acl_hash_tmp); > if (!NT_STATUS_IS_OK(status)) { > - TALLOC_FREE(frame); > - return status; > + goto fail; > } > > + TALLOC_FREE(sys_acl_blob_description); > + TALLOC_FREE(sys_acl_blob.data); > + > if (memcmp(&sys_acl_hash[0], &sys_acl_hash_tmp[0], > XATTR_SD_HASH_SIZE) == 0) { > /* Hash matches, return blob sd. */ > DEBUG(10, ("get_nt_acl_internal: blob hash " > "matches for file %s\n", > smb_fname->base_name )); > + psd = psd_blob; > + psd_blob = NULL; > goto out; > } > } > @@ -630,21 +633,15 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > "returned %s\n", > smb_fname->base_name, > nt_errstr(status))); > - TALLOC_FREE(frame); > - return status; > + goto fail; > } > > - /* Ensure we don't leak psd_fs if we don't choose it. > - * > - * We don't allocate it onto frame as it is preferred not to > - * steal from a talloc pool. > - */ > - talloc_steal(frame, psd_fs); > - > status = hash_sd_sha256(psd_fs, hash_tmp); > if (!NT_STATUS_IS_OK(status)) { > TALLOC_FREE(psd_blob); > - psd_blob = psd_fs; > + psd = psd_fs; > + psd_fs = NULL; > + psd_is_from_fs = true; > goto out; > } > > @@ -653,6 +650,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > DEBUG(10, ("get_nt_acl_internal: blob hash " > "matches for file %s\n", > smb_fname->base_name )); > + psd = psd_blob; > + psd_blob = NULL; > goto out; > } > > @@ -669,11 +668,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > } > > TALLOC_FREE(psd_blob); > - psd_blob = psd_fs; > + psd = psd_fs; > + psd_fs = NULL; > + psd_is_from_fs = true; > } > - out: > > - if (psd_blob == NULL) { > +out: > + if (psd == NULL) { > /* Get the full underlying sd, as we failed to get the > * blob for the hash, or the revision/hash type wasn't > * known */ > @@ -682,13 +683,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > fsp, > security_info, > mem_ctx, > - &psd_fs); > + &psd); > } else { > status = SMB_VFS_NEXT_GET_NT_ACL(handle, > smb_fname, > security_info, > mem_ctx, > - &psd_fs); > + &psd); > } > > if (!NT_STATUS_IS_OK(status)) { > @@ -696,24 +697,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > "returned %s\n", > smb_fname->base_name, > nt_errstr(status))); > - TALLOC_FREE(frame); > - return status; > + goto fail; > } > > - /* Ensure we don't leak psd_fs if we don't choose it. > - * > - * We don't allocate it onto frame as it is preferred not to > - * steal from a talloc pool. > - */ > - talloc_steal(frame, psd_fs); > - psd_blob = psd_fs; > + psd_is_from_fs = true; > } > > - if (psd_blob != psd_fs) { > - /* We're returning the blob, throw > - * away the filesystem SD. */ > - TALLOC_FREE(psd_fs); > - } else { > + if (psd_is_from_fs) { > SMB_STRUCT_STAT sbuf; > SMB_STRUCT_STAT *psbuf = &sbuf; > bool is_directory = false; > @@ -725,8 +715,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > if (fsp) { > status = vfs_stat_fsp(fsp); > if (!NT_STATUS_IS_OK(status)) { > - TALLOC_FREE(frame); > - return status; > + goto fail; > } > psbuf = &fsp->fsp_name->st; > } else { > @@ -751,72 +740,74 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > smb_fname, > &sbuf); > if (ret == -1) { > - TALLOC_FREE(frame); > - return map_nt_error_from_unix(errno); > + status = map_nt_error_from_unix(errno); > + goto fail; > } > } > is_directory = S_ISDIR(psbuf->st_ex_mode); > > if (ignore_file_system_acl) { > - TALLOC_FREE(psd_fs); > + TALLOC_FREE(psd); > status = make_default_filesystem_acl(mem_ctx, > smb_fname->base_name, > psbuf, > - &psd_blob); > + &psd); > if (!NT_STATUS_IS_OK(status)) { > - TALLOC_FREE(frame); > - return status; > + goto fail; > } > } else { > if (is_directory && > - !sd_has_inheritable_components(psd_blob, > + !sd_has_inheritable_components(psd, > true)) { > status = add_directory_inheritable_components( > handle, > smb_fname->base_name, > psbuf, > - psd_blob); > + psd); > if (!NT_STATUS_IS_OK(status)) { > - TALLOC_FREE(frame); > - return status; > + goto fail; > } > } > /* The underlying POSIX module always sets > the ~SEC_DESC_DACL_PROTECTED bit, as ACLs > can't be inherited in this way under POSIX. > Remove it for Windows-style ACLs. */ > - psd_blob->type &= ~SEC_DESC_DACL_PROTECTED; > + psd->type &= ~SEC_DESC_DACL_PROTECTED; > } > } > > if (!(security_info & SECINFO_OWNER)) { > - psd_blob->owner_sid = NULL; > + psd->owner_sid = NULL; > } > if (!(security_info & SECINFO_GROUP)) { > - psd_blob->group_sid = NULL; > + psd->group_sid = NULL; > } > if (!(security_info & SECINFO_DACL)) { > - psd_blob->type &= ~SEC_DESC_DACL_PRESENT; > - psd_blob->dacl = NULL; > + psd->type &= ~SEC_DESC_DACL_PRESENT; > + psd->dacl = NULL; > } > if (!(security_info & SECINFO_SACL)) { > - psd_blob->type &= ~SEC_DESC_SACL_PRESENT; > - psd_blob->sacl = NULL; > + psd->type &= ~SEC_DESC_SACL_PRESENT; > + psd->sacl = NULL; > } > > - TALLOC_FREE(blob.data); > - > if (DEBUGLEVEL >= 10) { > DEBUG(10,("get_nt_acl_internal: returning acl for %s is:\n", > smb_fname->base_name )); > - NDR_PRINT_DEBUG(security_descriptor, psd_blob); > + NDR_PRINT_DEBUG(security_descriptor, psd); > } > > - /* The VFS API is that the ACL is expected to be on mem_ctx */ > - *ppdesc = talloc_move(mem_ctx, &psd_blob); > + *ppdesc = psd; > > - TALLOC_FREE(frame); > return NT_STATUS_OK; > + > +fail: > + TALLOC_FREE(psd); > + TALLOC_FREE(psd_blob); > + TALLOC_FREE(psd_fs); > + TALLOC_FREE(sys_acl_blob_description); > + TALLOC_FREE(sys_acl_blob.data); > + return status; > } > > /********************************************************************* > -- > 2.7.4 > > > From a063153eeb506a018c84a34801eaddcc416eced9 Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Tue, 23 Aug 2016 22:32:57 +0200 > Subject: [PATCH 06/12] vfs_acl_common: move the ACL blob validation to a > helper function > > No change in behaviour. > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > source3/modules/vfs_acl_common.c | 177 +++++++++++++++++++++++++-------------- > 1 file changed, 112 insertions(+), 65 deletions(-) > > diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c > index 66c58e7..2cf5012 100644 > --- a/source3/modules/vfs_acl_common.c > +++ b/source3/modules/vfs_acl_common.c > @@ -467,20 +467,32 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx, > return NT_STATUS_OK; > } > > -/******************************************************************* > - Pull a DATA_BLOB from an xattr given a pathname. > - If the hash doesn't match, or doesn't exist - return the underlying > - filesystem sd. > -*******************************************************************/ > - > -static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > - files_struct *fsp, > - const struct smb_filename *smb_fname_in, > - uint32_t security_info, > - TALLOC_CTX *mem_ctx, > - struct security_descriptor **ppdesc) > +/** > + * Validate an ACL blob > + * > + * This validates an ACL blob against the underlying filesystem ACL. If this > + * function returns NT_STATUS_OK ppsd can be > + * > + * 1. the ACL from the blob (psd_from_fs=false), or > + * 2. the ACL from the fs (psd_from_fs=true), or > + * 3. NULL (!) > + * > + * If the return value is anything else then NT_STATUS_OK, ppsd is set to NULL > + * and psd_from_fs set to false. > + * > + * Returning the underlying filesystem ACL in case no. 2 is really just an > + * optimisation, because some validations have to fetch the filesytem ACL as > + * part of the validation, so we already have it available and callers might > + * need it as well. > + **/ > +static NTSTATUS validate_nt_acl_blob(TALLOC_CTX *mem_ctx, > + vfs_handle_struct *handle, > + files_struct *fsp, > + const struct smb_filename *smb_fname, > + DATA_BLOB *blob, > + struct security_descriptor **ppsd, > + bool *psd_is_from_fs) > { > - DATA_BLOB blob = data_blob_null; > NTSTATUS status; > uint16_t hash_type = XATTR_SD_HASH_TYPE_NONE; > uint16_t xattr_version = 0; > @@ -491,41 +503,28 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > struct security_descriptor *psd = NULL; > struct security_descriptor *psd_blob = NULL; > struct security_descriptor *psd_fs = NULL; > - const struct smb_filename *smb_fname = NULL; > + char *sys_acl_blob_description = NULL; > + DATA_BLOB sys_acl_blob = { 0 }; > bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn), > ACL_MODULE_NAME, > "ignore system acls", > false); > - char *sys_acl_blob_description = NULL; > - DATA_BLOB sys_acl_blob = { 0 }; > - bool psd_is_from_fs = false; > > - if (fsp && smb_fname_in == NULL) { > - smb_fname = fsp->fsp_name; > - } else { > - smb_fname = smb_fname_in; > - } > + *ppsd = NULL; > + *psd_is_from_fs = false; > > - DEBUG(10, ("get_nt_acl_internal: name=%s\n", smb_fname->base_name)); > - > - status = get_acl_blob(mem_ctx, handle, fsp, smb_fname, &blob); > + status = parse_acl_blob(blob, > + mem_ctx, > + &psd_blob, > + &hash_type, > + &xattr_version, > + &hash[0], > + &sys_acl_hash[0]); > if (!NT_STATUS_IS_OK(status)) { > - DEBUG(10, ("get_nt_acl_internal: get_acl_blob returned %s\n", > - nt_errstr(status))); > - goto out; > - } else { > - status = parse_acl_blob(&blob, mem_ctx, &psd_blob, > - &hash_type, &xattr_version, &hash[0], &sys_acl_hash[0]); > - if (!NT_STATUS_IS_OK(status)) { > - DEBUG(10, ("parse_acl_blob returned %s\n", > - nt_errstr(status))); > - TALLOC_FREE(blob.data); > - goto out; > - } > + DBG_DEBUG("parse_acl_blob returned %s\n", nt_errstr(status)); > + goto fail; > } > > - TALLOC_FREE(blob.data); > - > /* determine which type of xattr we got */ > switch (xattr_version) { > case 1: > @@ -534,15 +533,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > * require confirmation of the hash. In particular, > * the NTVFS file server uses version 1, but > * 'samba-tool ntacl' can set these as well */ > - psd = psd_blob; > - psd_blob = NULL; > - goto out; > + *ppsd = psd_blob; > + return NT_STATUS_OK; > case 3: > case 4: > if (ignore_file_system_acl) { > - psd = psd_blob; > - psd_blob = NULL; > - goto out; > + *ppsd = psd_blob; > + return NT_STATUS_OK; > } > > break; > @@ -552,7 +549,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > (unsigned int)hash_type, > smb_fname->base_name)); > TALLOC_FREE(psd_blob); > - goto out; > + return NT_STATUS_OK; > } > > /* determine which type of xattr we got */ > @@ -562,7 +559,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > (unsigned int)hash_type, > smb_fname->base_name)); > TALLOC_FREE(psd_blob); > - goto out; > + return NT_STATUS_OK; > } > > /* determine which type of xattr we got */ > @@ -603,9 +600,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > DEBUG(10, ("get_nt_acl_internal: blob hash " > "matches for file %s\n", > smb_fname->base_name )); > - psd = psd_blob; > - psd_blob = NULL; > - goto out; > + *ppsd = psd_blob; > + return NT_STATUS_OK; > } > } > > @@ -639,10 +635,9 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > status = hash_sd_sha256(psd_fs, hash_tmp); > if (!NT_STATUS_IS_OK(status)) { > TALLOC_FREE(psd_blob); > - psd = psd_fs; > - psd_fs = NULL; > - psd_is_from_fs = true; > - goto out; > + *ppsd = psd_fs; > + *psd_is_from_fs = true; > + return NT_STATUS_OK; > } > > if (memcmp(&hash[0], &hash_tmp[0], XATTR_SD_HASH_SIZE) == 0) { > @@ -650,9 +645,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > DEBUG(10, ("get_nt_acl_internal: blob hash " > "matches for file %s\n", > smb_fname->base_name )); > - psd = psd_blob; > - psd_blob = NULL; > - goto out; > + *ppsd = psd_blob; > + return NT_STATUS_OK; > } > > /* Hash doesn't match, return underlying sd. */ > @@ -668,12 +662,69 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > } > > TALLOC_FREE(psd_blob); > - psd = psd_fs; > - psd_fs = NULL; > - psd_is_from_fs = true; > + *ppsd = psd_fs; > + *psd_is_from_fs = true; > + } > + > + return NT_STATUS_OK; > + > +fail: > + TALLOC_FREE(psd); > + TALLOC_FREE(psd_blob); > + TALLOC_FREE(psd_fs); > + TALLOC_FREE(sys_acl_blob_description); > + TALLOC_FREE(sys_acl_blob.data); > + return status; > +} > + > +/******************************************************************* > + Pull a DATA_BLOB from an xattr given a pathname. > + If the hash doesn't match, or doesn't exist - return the underlying > + filesystem sd. > +*******************************************************************/ > + > +static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > + files_struct *fsp, > + const struct smb_filename *smb_fname_in, > + uint32_t security_info, > + TALLOC_CTX *mem_ctx, > + struct security_descriptor **ppdesc) > +{ > + DATA_BLOB blob = data_blob_null; > + NTSTATUS status; > + struct security_descriptor *psd = NULL; > + const struct smb_filename *smb_fname = NULL; > + bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn), > + ACL_MODULE_NAME, > + "ignore system acls", > + false); > + bool psd_is_from_fs = false; > + > + if (fsp && smb_fname_in == NULL) { > + smb_fname = fsp->fsp_name; > + } else { > + smb_fname = smb_fname_in; > + } > + > + DEBUG(10, ("get_nt_acl_internal: name=%s\n", smb_fname->base_name)); > + > + status = get_acl_blob(mem_ctx, handle, fsp, smb_fname, &blob); > + if (NT_STATUS_IS_OK(status)) { > + status = validate_nt_acl_blob(mem_ctx, > + handle, > + fsp, > + smb_fname, > + &blob, > + &psd, > + &psd_is_from_fs); > + TALLOC_FREE(blob.data); > + if (!NT_STATUS_IS_OK(status)) { > + DBG_DEBUG("ACL validation for [%s] failed\n", > + smb_fname->base_name); > + goto fail; > + } > } > > -out: > if (psd == NULL) { > /* Get the full underlying sd, as we failed to get the > * blob for the hash, or the revision/hash type wasn't > @@ -803,10 +854,6 @@ out: > > fail: > TALLOC_FREE(psd); > - TALLOC_FREE(psd_blob); > - TALLOC_FREE(psd_fs); > - TALLOC_FREE(sys_acl_blob_description); > - TALLOC_FREE(sys_acl_blob.data); > return status; > } > > -- > 2.7.4 > > > From 9a9795133dfc713dd5008eb8705d5b7f72ae1f91 Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Wed, 24 Aug 2016 10:01:17 +0200 > Subject: [PATCH 07/12] vfs_acl_tdb|xattr: use a config handle > > Better for performance and a subsequent commit will add one more option > where this will pay off. > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > source3/modules/vfs_acl_common.c | 50 ++++++++++++++++++++++++++++++++-------- > source3/modules/vfs_acl_tdb.c | 7 ++++++ > source3/modules/vfs_acl_xattr.c | 7 ++++++ > 3 files changed, 54 insertions(+), 10 deletions(-) > > diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c > index 2cf5012..fe631e3 100644 > --- a/source3/modules/vfs_acl_common.c > +++ b/source3/modules/vfs_acl_common.c > @@ -46,6 +46,34 @@ static NTSTATUS store_acl_blob_fsp(vfs_handle_struct *handle, > SECINFO_DACL | \ > SECINFO_SACL) > > +struct acl_common_config { > + bool ignore_system_acls; > +}; > + > +static bool init_acl_common_config(vfs_handle_struct *handle) > +{ > + struct acl_common_config *config = NULL; > + > + config = talloc_zero(handle->conn, struct acl_common_config); > + if (config == NULL) { > + DBG_ERR("talloc_zero() failed\n"); > + errno = ENOMEM; > + return false; > + } > + > + config->ignore_system_acls = lp_parm_bool(SNUM(handle->conn), > + ACL_MODULE_NAME, > + "ignore system acls", > + false); > + > + SMB_VFS_HANDLE_SET_DATA(handle, config, NULL, > + struct acl_common_config, > + return false); > + > + return true; > +} > + > + > /******************************************************************* > Hash a security descriptor. > *******************************************************************/ > @@ -505,14 +533,15 @@ static NTSTATUS validate_nt_acl_blob(TALLOC_CTX *mem_ctx, > struct security_descriptor *psd_fs = NULL; > char *sys_acl_blob_description = NULL; > DATA_BLOB sys_acl_blob = { 0 }; > - bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn), > - ACL_MODULE_NAME, > - "ignore system acls", > - false); > + struct acl_common_config *config = NULL; > > *ppsd = NULL; > *psd_is_from_fs = false; > > + SMB_VFS_HANDLE_GET_DATA(handle, config, > + struct acl_common_config, > + return NT_STATUS_UNSUCCESSFUL); > + > status = parse_acl_blob(blob, > mem_ctx, > &psd_blob, > @@ -537,7 +566,7 @@ static NTSTATUS validate_nt_acl_blob(TALLOC_CTX *mem_ctx, > return NT_STATUS_OK; > case 3: > case 4: > - if (ignore_file_system_acl) { > + if (config->ignore_system_acls) { > *ppsd = psd_blob; > return NT_STATUS_OK; > } > @@ -694,11 +723,12 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > NTSTATUS status; > struct security_descriptor *psd = NULL; > const struct smb_filename *smb_fname = NULL; > - bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn), > - ACL_MODULE_NAME, > - "ignore system acls", > - false); > bool psd_is_from_fs = false; > + struct acl_common_config *config = NULL; > + > + SMB_VFS_HANDLE_GET_DATA(handle, config, > + struct acl_common_config, > + return NT_STATUS_UNSUCCESSFUL); > > if (fsp && smb_fname_in == NULL) { > smb_fname = fsp->fsp_name; > @@ -797,7 +827,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > } > is_directory = S_ISDIR(psbuf->st_ex_mode); > > - if (ignore_file_system_acl) { > + if (config->ignore_system_acls) { > TALLOC_FREE(psd); > status = make_default_filesystem_acl(mem_ctx, > smb_fname->base_name, > diff --git a/source3/modules/vfs_acl_tdb.c b/source3/modules/vfs_acl_tdb.c > index e4c8462..0c92b72 100644 > --- a/source3/modules/vfs_acl_tdb.c > +++ b/source3/modules/vfs_acl_tdb.c > @@ -308,6 +308,7 @@ static int connect_acl_tdb(struct vfs_handle_struct *handle, > const char *user) > { > int ret = SMB_VFS_NEXT_CONNECT(handle, service, user); > + bool ok; > > if (ret < 0) { > return ret; > @@ -318,6 +319,12 @@ static int connect_acl_tdb(struct vfs_handle_struct *handle, > return -1; > } > > + ok = init_acl_common_config(handle); > + if (!ok) { > + DBG_ERR("init_acl_common_config failed\n"); > + return -1; > + } > + > /* Ensure we have the parameters correct if we're > * using this module. */ > DEBUG(2,("connect_acl_tdb: setting 'inherit acls = true' " > diff --git a/source3/modules/vfs_acl_xattr.c b/source3/modules/vfs_acl_xattr.c > index d311c57..307ab6a 100644 > --- a/source3/modules/vfs_acl_xattr.c > +++ b/source3/modules/vfs_acl_xattr.c > @@ -180,11 +180,18 @@ static int connect_acl_xattr(struct vfs_handle_struct *handle, > const char *user) > { > int ret = SMB_VFS_NEXT_CONNECT(handle, service, user); > + bool ok; > > if (ret < 0) { > return ret; > } > > + ok = init_acl_common_config(handle); > + if (!ok) { > + DBG_ERR("init_acl_common_config failed\n"); > + return -1; > + } > + > /* Ensure we have the parameters correct if we're > * using this module. */ > DEBUG(2,("connect_acl_xattr: setting 'inherit acls = true' " > -- > 2.7.4 > > > From 8a645460b093d8c61a3507ebb6e3337a3873b10a Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Wed, 24 Aug 2016 10:30:15 +0200 > Subject: [PATCH 08/12] vfs_acl_common: move stat stuff to a helper function > > Will be reused in the next commit when moving the > make_default_filesystem_acl() stuff to a different place. > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > source3/modules/vfs_acl_common.c | 79 ++++++++++++++++++++++++---------------- > 1 file changed, 48 insertions(+), 31 deletions(-) > > diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c > index fe631e3..d7fe258 100644 > --- a/source3/modules/vfs_acl_common.c > +++ b/source3/modules/vfs_acl_common.c > @@ -706,6 +706,48 @@ fail: > return status; > } > > +static NTSTATUS stat_fsp_or_smb_fname(vfs_handle_struct *handle, > + files_struct *fsp, > + const struct smb_filename *smb_fname, > + SMB_STRUCT_STAT *sbuf, > + SMB_STRUCT_STAT **psbuf) > +{ > + NTSTATUS status; > + int ret; > + > + if (fsp) { > + status = vfs_stat_fsp(fsp); > + if (!NT_STATUS_IS_OK(status)) { > + return status; > + } > + *psbuf = &fsp->fsp_name->st; > + } else { > + /* > + * https://bugzilla.samba.org/show_bug.cgi?id=11249 > + * > + * We are currently guaranteed that 'name' here is a > + * smb_fname->base_name, which *cannot* contain a stream name > + * (':'). vfs_stat_smb_fname() splits a name into a base name + > + * stream name, which when we get here we know we've already > + * done. So we have to call the stat or lstat VFS calls > + * directly here. Else, a base_name that contains a ':' (from a > + * demangled name) will get split again. > + * > + * FIXME. > + * This uglyness will go away once smb_fname is fully plumbed > + * through the VFS. > + */ > + ret = vfs_stat_smb_basename(handle->conn, > + smb_fname, > + sbuf); > + if (ret == -1) { > + return map_nt_error_from_unix(errno); > + } > + } > + > + return NT_STATUS_OK; > +} > + > /******************************************************************* > Pull a DATA_BLOB from an xattr given a pathname. > If the hash doesn't match, or doesn't exist - return the underlying > @@ -793,38 +835,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > * filesystem. If it's a directory, and has no > * inheritable ACE entries we have to fake them. > */ > - if (fsp) { > - status = vfs_stat_fsp(fsp); > - if (!NT_STATUS_IS_OK(status)) { > - goto fail; > - } > - psbuf = &fsp->fsp_name->st; > - } else { > - /* > - * https://bugzilla.samba.org/show_bug.cgi?id=11249 > - * > - * We are currently guaranteed that 'name' here is > - * a smb_fname->base_name, which *cannot* contain > - * a stream name (':'). vfs_stat_smb_fname() splits > - * a name into a base name + stream name, which > - * when we get here we know we've already done. > - * So we have to call the stat or lstat VFS > - * calls directly here. Else, a base_name that > - * contains a ':' (from a demangled name) will > - * get split again. > - * > - * FIXME. > - * This uglyness will go away once smb_fname > - * is fully plumbed through the VFS. > - */ > - int ret = vfs_stat_smb_basename(handle->conn, > - smb_fname, > - &sbuf); > - if (ret == -1) { > - status = map_nt_error_from_unix(errno); > - goto fail; > - } > + > + status = stat_fsp_or_smb_fname(handle, fsp, smb_fname, > + &sbuf, &psbuf); > + if (!NT_STATUS_IS_OK(status)) { > + goto fail; > } > + > is_directory = S_ISDIR(psbuf->st_ex_mode); > > if (config->ignore_system_acls) { > -- > 2.7.4 > > > From 33980bcae5a907f9b8f67c5852a365bd22afc53c Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Wed, 24 Aug 2016 10:43:47 +0200 > Subject: [PATCH 09/12] vfs_acl_common: check for ignore_system_acls before > fetching filesystem ACL > > If ignore_system_acls is set and we're synthesizing a default ACL, we > were fetching the filesystem ACL just to free it again. This change > avoids this. > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > source3/modules/vfs_acl_common.c | 99 ++++++++++++++++++++++------------------ > 1 file changed, 55 insertions(+), 44 deletions(-) > > diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c > index d7fe258..9071775 100644 > --- a/source3/modules/vfs_acl_common.c > +++ b/source3/modules/vfs_acl_common.c > @@ -801,35 +801,57 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > /* Get the full underlying sd, as we failed to get the > * blob for the hash, or the revision/hash type wasn't > * known */ > - if (fsp) { > - status = SMB_VFS_NEXT_FGET_NT_ACL(handle, > - fsp, > - security_info, > - mem_ctx, > - &psd); > + > + if (config->ignore_system_acls) { > + SMB_STRUCT_STAT sbuf; > + SMB_STRUCT_STAT *psbuf = &sbuf; > + > + status = stat_fsp_or_smb_fname(handle, fsp, smb_fname, > + &sbuf, &psbuf); > + if (!NT_STATUS_IS_OK(status)) { > + goto fail; > + } > + > + status = make_default_filesystem_acl( > + mem_ctx, > + smb_fname->base_name, > + psbuf, > + &psd); > + if (!NT_STATUS_IS_OK(status)) { > + goto fail; > + } > } else { > - status = SMB_VFS_NEXT_GET_NT_ACL(handle, > - smb_fname, > - security_info, > - mem_ctx, > - &psd); > - } > + if (fsp) { > + status = SMB_VFS_NEXT_FGET_NT_ACL(handle, > + fsp, > + security_info, > + mem_ctx, > + &psd); > + } else { > + status = SMB_VFS_NEXT_GET_NT_ACL(handle, > + smb_fname, > + security_info, > + mem_ctx, > + &psd); > + } > > - if (!NT_STATUS_IS_OK(status)) { > - DEBUG(10, ("get_nt_acl_internal: get_next_acl for file %s " > - "returned %s\n", > - smb_fname->base_name, > - nt_errstr(status))); > - goto fail; > - } > + if (!NT_STATUS_IS_OK(status)) { > + DBG_DEBUG("get_next_acl for file %s " > + "returned %s\n", > + smb_fname->base_name, > + nt_errstr(status)); > + goto fail; > + } > > - psd_is_from_fs = true; > + psd_is_from_fs = true; > + } > } > > if (psd_is_from_fs) { > SMB_STRUCT_STAT sbuf; > SMB_STRUCT_STAT *psbuf = &sbuf; > bool is_directory = false; > + > /* > * We're returning the underlying ACL from the > * filesystem. If it's a directory, and has no > @@ -844,34 +866,23 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > > is_directory = S_ISDIR(psbuf->st_ex_mode); > > - if (config->ignore_system_acls) { > - TALLOC_FREE(psd); > - status = make_default_filesystem_acl(mem_ctx, > - smb_fname->base_name, > - psbuf, > - &psd); > + if (is_directory && !sd_has_inheritable_components(psd, true)) { > + status = add_directory_inheritable_components( > + handle, > + smb_fname->base_name, > + psbuf, > + psd); > if (!NT_STATUS_IS_OK(status)) { > goto fail; > } > - } else { > - if (is_directory && > - !sd_has_inheritable_components(psd, > - true)) { > - status = add_directory_inheritable_components( > - handle, > - smb_fname->base_name, > - psbuf, > - psd); > - if (!NT_STATUS_IS_OK(status)) { > - goto fail; > - } > - } > - /* The underlying POSIX module always sets > - the ~SEC_DESC_DACL_PROTECTED bit, as ACLs > - can't be inherited in this way under POSIX. > - Remove it for Windows-style ACLs. */ > - psd->type &= ~SEC_DESC_DACL_PROTECTED; > } > + > + /* > + * The underlying POSIX module always sets the > + * ~SEC_DESC_DACL_PROTECTED bit, as ACLs can't be inherited in > + * this way under POSIX. Remove it for Windows-style ACLs. > + */ > + psd->type &= ~SEC_DESC_DACL_PROTECTED; > } > > if (!(security_info & SECINFO_OWNER)) { > -- > 2.7.4 > > > From 1dafd89f8ffa4825d8593fe27069b4d8dcb1ae80 Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Wed, 24 Aug 2016 20:31:00 +0200 > Subject: [PATCH 10/12] vfs_acl_xattr|tdb: add option to control default ACL > style > > Existing behaviour is "posix" style. Next commit will (re)add the > "windows" style. This commit doesn't change behaviour in any way. > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > docs-xml/manpages/vfs_acl_tdb.8.xml | 25 +++++++++++++++++++ > docs-xml/manpages/vfs_acl_xattr.8.xml | 25 +++++++++++++++++++ > source3/modules/vfs_acl_common.c | 45 +++++++++++++++++++++++++++++++---- > 3 files changed, 91 insertions(+), 4 deletions(-) > > diff --git a/docs-xml/manpages/vfs_acl_tdb.8.xml b/docs-xml/manpages/vfs_acl_tdb.8.xml > index 640cec0..607e344 100644 > --- a/docs-xml/manpages/vfs_acl_tdb.8.xml > +++ b/docs-xml/manpages/vfs_acl_tdb.8.xml > @@ -63,6 +63,31 @@ > </para> > </listitem> > </varlistentry> > + > + <varlistentry> > + <term>acl_tdb:default acl style = [posix|windows]</term> > + <listitem> > + <para> > + This parameter determines the type of ACL that is synthesized in > + case a file or directory lacks an > + <emphasis>security.NTACL</emphasis> xattr. > + </para> > + <para> > + When set to <emphasis>posix</emphasis>, an ACL will be > + synthesized based on the POSIX mode permissions for user, group > + and others, with an additional ACE for <emphasis>NT > + Authority\SYSTEM</emphasis> will full rights. > + </para> > + <para> > + When set to <emphasis>windows</emphasis>, an ACL is synthesized > + the same way Windows does it, only including permissions for the > + owner and <emphasis>NT Authority\SYSTEM</emphasis>. > + </para> > + <para> > + The default for this option is <emphasis>posix</emphasis>. > + </para> > + </listitem> > + </varlistentry> > </variablelist> > > </refsect1> > diff --git a/docs-xml/manpages/vfs_acl_xattr.8.xml b/docs-xml/manpages/vfs_acl_xattr.8.xml > index 60a1c2d..8da73e0 100644 > --- a/docs-xml/manpages/vfs_acl_xattr.8.xml > +++ b/docs-xml/manpages/vfs_acl_xattr.8.xml > @@ -67,6 +67,31 @@ > </para> > </listitem> > </varlistentry> > + > + <varlistentry> > + <term>acl_xattr:default acl style = [posix|windows]</term> > + <listitem> > + <para> > + This parameter determines the type of ACL that is synthesized in > + case a file or directory lacks an > + <emphasis>security.NTACL</emphasis> xattr. > + </para> > + <para> > + When set to <emphasis>posix</emphasis>, an ACL will be > + synthesized based on the POSIX mode permissions for user, group > + and others, with an additional ACE for <emphasis>NT > + Authority\SYSTEM</emphasis> will full rights. > + </para> > + <para> > + When set to <emphasis>windows</emphasis>, an ACL is synthesized > + the same way Windows does it, only including permissions for the > + owner and <emphasis>NT Authority\SYSTEM</emphasis>. > + </para> > + <para> > + The default for this option is <emphasis>posix</emphasis>. > + </para> > + </listitem> > + </varlistentry> > </variablelist> > > </refsect1> > diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c > index 9071775..919a095 100644 > --- a/source3/modules/vfs_acl_common.c > +++ b/source3/modules/vfs_acl_common.c > @@ -46,8 +46,16 @@ static NTSTATUS store_acl_blob_fsp(vfs_handle_struct *handle, > SECINFO_DACL | \ > SECINFO_SACL) > > +enum default_acl_style {DEFAULT_ACL_POSIX, DEFAULT_ACL_WINDOWS}; > + > +static const struct enum_list default_acl_style[] = { > + {DEFAULT_ACL_POSIX, "posix"}, > + {DEFAULT_ACL_WINDOWS, "windows"} > +}; > + > struct acl_common_config { > bool ignore_system_acls; > + enum default_acl_style default_acl_style; > }; > > static bool init_acl_common_config(vfs_handle_struct *handle) > @@ -65,6 +73,11 @@ static bool init_acl_common_config(vfs_handle_struct *handle) > ACL_MODULE_NAME, > "ignore system acls", > false); > + config->default_acl_style = lp_parm_enum(SNUM(handle->conn), > + ACL_MODULE_NAME, > + "default acl style", > + default_acl_style, > + DEFAULT_ACL_POSIX); > > SMB_VFS_HANDLE_SET_DATA(handle, config, NULL, > struct acl_common_config, > @@ -387,10 +400,10 @@ static NTSTATUS add_directory_inheritable_components(vfs_handle_struct *handle, > return NT_STATUS_OK; > } > > -static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx, > - const char *name, > - SMB_STRUCT_STAT *psbuf, > - struct security_descriptor **ppdesc) > +static NTSTATUS make_default_acl_posix(TALLOC_CTX *ctx, > + const char *name, > + SMB_STRUCT_STAT *psbuf, > + struct security_descriptor **ppdesc) > { > struct dom_sid owner_sid, group_sid; > size_t size = 0; > @@ -495,6 +508,29 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx, > return NT_STATUS_OK; > } > > +static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx, > + struct acl_common_config *config, > + const char *name, > + SMB_STRUCT_STAT *psbuf, > + struct security_descriptor **ppdesc) > +{ > + NTSTATUS status; > + > + switch (config->default_acl_style) { > + > + case DEFAULT_ACL_POSIX: > + status = make_default_acl_posix(ctx, name, psbuf, ppdesc); > + break; > + > + default: > + DBG_ERR("unknown acl style %d", config->default_acl_style); > + status = NT_STATUS_INTERNAL_ERROR; > + break; > + } > + > + return status; > +} > + > /** > * Validate an ACL blob > * > @@ -814,6 +850,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, > > status = make_default_filesystem_acl( > mem_ctx, > + config, > smb_fname->base_name, > psbuf, > &psd); > -- > 2.7.4 > > > From 54dbe78c2ec49dff927bd120e25db1f2a9b10e65 Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Thu, 25 Aug 2016 07:45:34 +0200 > Subject: [PATCH 11/12] vfs_acl_common: Windows style default ACL > > Reintroduce Windows style default ACL, but this time as an optional > feature, not changing default behaviour. > > Original bugreport that got reverted because it changed the default > behaviour: https://bugzilla.samba.org/show_bug.cgi?id=12028 > > Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177 > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > source3/modules/vfs_acl_common.c | 76 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 76 insertions(+) > > diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c > index 919a095..a3f1e3d 100644 > --- a/source3/modules/vfs_acl_common.c > +++ b/source3/modules/vfs_acl_common.c > @@ -508,6 +508,78 @@ static NTSTATUS make_default_acl_posix(TALLOC_CTX *ctx, > return NT_STATUS_OK; > } > > +static NTSTATUS make_default_acl_windows(TALLOC_CTX *ctx, > + const char *name, > + SMB_STRUCT_STAT *psbuf, > + struct security_descriptor **ppdesc) > +{ > + struct dom_sid owner_sid, group_sid; > + size_t size = 0; > + struct security_ace aces[4]; > + uint32_t access_mask = 0; > + mode_t mode = psbuf->st_ex_mode; > + struct security_acl *new_dacl = NULL; > + int idx = 0; > + > + DBG_DEBUG("file [%s] mode [0%o]\n", name, (int)mode); > + > + uid_to_sid(&owner_sid, psbuf->st_ex_uid); > + gid_to_sid(&group_sid, psbuf->st_ex_gid); > + > + /* > + * We provide 2 ACEs: > + * - Owner > + * - NT System > + */ > + > + if (mode & S_IRUSR) { > + if (mode & S_IWUSR) { > + access_mask |= SEC_RIGHTS_FILE_ALL; > + } else { > + access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE; > + } > + } > + if (mode & S_IWUSR) { > + access_mask |= SEC_RIGHTS_FILE_WRITE | SEC_STD_DELETE; > + } > + > + init_sec_ace(&aces[idx], > + &owner_sid, > + SEC_ACE_TYPE_ACCESS_ALLOWED, > + access_mask, > + 0); > + idx++; > + > + init_sec_ace(&aces[idx], > + &global_sid_System, > + SEC_ACE_TYPE_ACCESS_ALLOWED, > + SEC_RIGHTS_FILE_ALL, > + 0); > + idx++; > + > + new_dacl = make_sec_acl(ctx, > + NT4_ACL_REVISION, > + idx, > + aces); > + > + if (!new_dacl) { > + return NT_STATUS_NO_MEMORY; > + } > + > + *ppdesc = make_sec_desc(ctx, > + SECURITY_DESCRIPTOR_REVISION_1, > + SEC_DESC_SELF_RELATIVE|SEC_DESC_DACL_PRESENT, > + &owner_sid, > + &group_sid, > + NULL, > + new_dacl, > + &size); > + if (!*ppdesc) { > + return NT_STATUS_NO_MEMORY; > + } > + return NT_STATUS_OK; > +} > + > static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx, > struct acl_common_config *config, > const char *name, > @@ -522,6 +594,10 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx, > status = make_default_acl_posix(ctx, name, psbuf, ppdesc); > break; > > + case DEFAULT_ACL_WINDOWS: > + status = make_default_acl_windows(ctx, name, psbuf, ppdesc); > + break; > + > default: > DBG_ERR("unknown acl style %d", config->default_acl_style); > status = NT_STATUS_INTERNAL_ERROR; > -- > 2.7.4 > > > From 521fda43f24d35b8af4ed9fd503b1d42f30e3d03 Mon Sep 17 00:00:00 2001 > From: Ralph Boehme <slow at samba.org> > Date: Thu, 25 Aug 2016 16:30:24 +0200 > Subject: [PATCH 12/12] s4/torture: tests for vfs_acl_xattr default ACL styles > > Signed-off-by: Ralph Boehme <slow at samba.org> > --- > selftest/target/Samba3.pm | 8 + > source3/selftest/tests.py | 4 +- > source4/torture/vfs/acl_xattr.c | 314 ++++++++++++++++++++++++++++++++++++++++ > source4/torture/vfs/vfs.c | 1 + > source4/torture/wscript_build | 2 +- > 5 files changed, 327 insertions(+), 2 deletions(-) > create mode 100644 source4/torture/vfs/acl_xattr.c > > diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm > index 8fc3204..f68d7de 100755 > --- a/selftest/target/Samba3.pm > +++ b/selftest/target/Samba3.pm > @@ -1792,6 +1792,14 @@ sub provision($$$$$$$$) > vfs objects = acl_xattr fake_acls xattr_tdb fake_dfq > inherit owner = yes > include = $dfqconffile > +[acl_xattr_ign_sysacl_posix] > + copy = tmp > + acl_xattr:ignore system acls = yes > + acl_xattr:default acl style = posix > +[acl_xattr_ign_sysacl_windows] > + copy = tmp > + acl_xattr:ignore system acls = yes > + acl_xattr:default acl style = windows > "; > close(CONF); > > diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py > index 7d0dcc1..e4c31c6 100755 > --- a/source3/selftest/tests.py > +++ b/source3/selftest/tests.py > @@ -322,7 +322,7 @@ nbt = ["nbt.dgram" ] > > libsmbclient = ["libsmbclient"] > > -vfs = ["vfs.fruit"] > +vfs = ["vfs.fruit", "vfs.acl_xattr"] > > tests= base + raw + smb2 + rpc + unix + local + rap + nbt + libsmbclient + idmap + vfs > > @@ -418,6 +418,8 @@ for t in tests: > plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD --signing=required') > elif t == "smb2.dosmode": > plansmbtorture4testsuite(t, "simpleserver", '//$SERVER/dosmode -U$USERNAME%$PASSWORD') > + elif t == "vfs.acl_xattr": > + plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD') > else: > plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD') > plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD') > diff --git a/source4/torture/vfs/acl_xattr.c b/source4/torture/vfs/acl_xattr.c > new file mode 100644 > index 0000000..7fd10d0 > --- /dev/null > +++ b/source4/torture/vfs/acl_xattr.c > @@ -0,0 +1,314 @@ > +/* > + Unix SMB/CIFS implementation. > + > + Copyright (C) Ralph Boehme 2016 > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see <http://www.gnu.org/licenses/>. > +*/ > + > +#include "includes.h" > +#include "lib/cmdline/popt_common.h" > +#include "libcli/smb2/smb2.h" > +#include "libcli/smb2/smb2_calls.h" > +#include "libcli/smb/smbXcli_base.h" > +#include "torture/torture.h" > +#include "torture/vfs/proto.h" > +#include "libcli/resolve/resolve.h" > +#include "torture/util.h" > +#include "torture/smb2/proto.h" > +#include "libcli/security/security.h" > +#include "librpc/gen_ndr/ndr_security.h" > +#include "lib/param/param.h" > + > +#define BASEDIR "smb2-testsd" > + > +#define CHECK_SECURITY_DESCRIPTOR(_sd1, _sd2) do { \ > + if (!security_descriptor_equal(_sd1, _sd2)) { \ > + torture_warning(tctx, "security descriptors don't match!\n"); \ > + torture_warning(tctx, "got:\n"); \ > + NDR_PRINT_DEBUG(security_descriptor, _sd1); \ > + torture_warning(tctx, "expected:\n"); \ > + NDR_PRINT_DEBUG(security_descriptor, _sd2); \ > + torture_result(tctx, TORTURE_FAIL, \ > + "%s: security descriptors don't match!\n", \ > + __location__); \ > + ret = false; \ > + } \ > +} while (0) > + > +/** > + * SMB2 connect with explicit share > + **/ > +static bool torture_smb2_con_share(struct torture_context *tctx, > + const char *share, > + struct smb2_tree **tree) > +{ > + struct smbcli_options options; > + NTSTATUS status; > + const char *host = torture_setting_string(tctx, "host", NULL); > + struct cli_credentials *credentials = cmdline_credentials; > + > + lpcfg_smbcli_options(tctx->lp_ctx, &options); > + > + status = smb2_connect_ext(tctx, > + host, > + lpcfg_smb_ports(tctx->lp_ctx), > + share, > + lpcfg_resolve_context(tctx->lp_ctx), > + credentials, > + 0, > + tree, > + tctx->ev, > + &options, > + lpcfg_socket_options(tctx->lp_ctx), > + lpcfg_gensec_settings(tctx, tctx->lp_ctx) > + ); > + if (!NT_STATUS_IS_OK(status)) { > + printf("Failed to connect to SMB2 share \\\\%s\\%s - %s\n", > + host, share, nt_errstr(status)); > + return false; > + } > + return true; > +} > + > +static bool test_default_acl_posix(struct torture_context *tctx, > + struct smb2_tree *tree_unused) > +{ > + struct smb2_tree *tree = NULL; > + NTSTATUS status; > + bool ok; > + bool ret = true; > + const char *dname = BASEDIR "\\testdir"; > + const char *fname = BASEDIR "\\testdir\\testfile"; > + struct smb2_handle fhandle, dhandle; > + union smb_fileinfo q; > + union smb_setfileinfo set; > + struct security_descriptor *sd = NULL; > + struct security_descriptor *exp_sd = NULL; > + char *owner_sid = NULL; > + char *group_sid = NULL; > + > + ok = torture_smb2_con_share(tctx, "acl_xattr_ign_sysacl_posix", &tree); > + torture_assert_goto(tctx, ok == true, ret, done, > + "Unable to connect to 'acl_xattr_ign_sysacl_posix'\n"); > + > + ok = smb2_util_setup_dir(tctx, tree, BASEDIR); > + torture_assert_goto(tctx, ok == true, ret, done, "Unable to setup testdir\n"); > + > + ZERO_STRUCT(dhandle); > + status = torture_smb2_testdir(tree, dname, &dhandle); > + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "torture_smb2_testdir\n"); > + > + torture_comment(tctx, "Get the original sd\n"); > + > + ZERO_STRUCT(q); > + q.query_secdesc.level = RAW_FILEINFO_SEC_DESC; > + q.query_secdesc.in.file.handle = dhandle; > + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER | SECINFO_GROUP; > + status = smb2_getinfo_file(tree, tctx, &q); > + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_getinfo_file\n"); > + > + sd = q.query_secdesc.out.sd; > + owner_sid = dom_sid_string(tctx, sd->owner_sid); > + group_sid = dom_sid_string(tctx, sd->group_sid); > + torture_comment(tctx, "owner [%s] group [%s]\n", owner_sid, group_sid); > + > + torture_comment(tctx, "Set ACL with no inheritable ACE\n"); > + > + sd = security_descriptor_dacl_create(tctx, > + 0, NULL, NULL, > + owner_sid, > + SEC_ACE_TYPE_ACCESS_ALLOWED, > + SEC_RIGHTS_DIR_ALL, > + 0, > + NULL); > + > + ZERO_STRUCT(set); > + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC; > + set.set_secdesc.in.file.handle = dhandle; > + set.set_secdesc.in.secinfo_flags = SECINFO_DACL; > + set.set_secdesc.in.sd = sd; > + status = smb2_setinfo_file(tree, &set); > + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_setinfo_file\n"); > + > + TALLOC_FREE(sd); > + smb2_util_close(tree, dhandle); > + > + torture_comment(tctx, "Create file\n"); > + > + ZERO_STRUCT(fhandle); > + status = torture_smb2_testfile(tree, fname, &fhandle); > + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_create_complex_file\n"); > + > + torture_comment(tctx, "Query file SD\n"); > + > + ZERO_STRUCT(q); > + q.query_secdesc.level = RAW_FILEINFO_SEC_DESC; > + q.query_secdesc.in.file.handle = fhandle; > + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER | SECINFO_GROUP; > + status = smb2_getinfo_file(tree, tctx, &q); > + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_getinfo_file\n"); > + sd = q.query_secdesc.out.sd; > + > + smb2_util_close(tree, fhandle); > + ZERO_STRUCT(fhandle); > + > + torture_comment(tctx, "Checking actual file SD against expected SD\n"); > + > + exp_sd = security_descriptor_dacl_create( > + tctx, 0, owner_sid, group_sid, > + owner_sid, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, 0, > + group_sid, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE, 0, > + SID_WORLD, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE, 0, > + SID_NT_SYSTEM, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, 0, > + NULL); > + > + CHECK_SECURITY_DESCRIPTOR(sd, exp_sd); > + > +done: > + if (!smb2_util_handle_empty(fhandle)) { > + smb2_util_close(tree, fhandle); > + } > + if (!smb2_util_handle_empty(dhandle)) { > + smb2_util_close(tree, dhandle); > + } > + if (tree != NULL) { > + smb2_deltree(tree, BASEDIR); > + smb2_tdis(tree); > + } > + > + return ret; > +} > + > +static bool test_default_acl_win(struct torture_context *tctx, > + struct smb2_tree *tree_unused) > +{ > + struct smb2_tree *tree = NULL; > + NTSTATUS status; > + bool ok; > + bool ret = true; > + const char *dname = BASEDIR "\\testdir"; > + const char *fname = BASEDIR "\\testdir\\testfile"; > + struct smb2_handle fhandle, dhandle; > + union smb_fileinfo q; > + union smb_setfileinfo set; > + struct security_descriptor *sd = NULL; > + struct security_descriptor *exp_sd = NULL; > + char *owner_sid = NULL; > + char *group_sid = NULL; > + > + ok = torture_smb2_con_share(tctx, "acl_xattr_ign_sysacl_windows", &tree); > + torture_assert_goto(tctx, ok == true, ret, done, > + "Unable to connect to 'acl_xattr_ign_sysacl_windows'\n"); > + > + ok = smb2_util_setup_dir(tctx, tree, BASEDIR); > + torture_assert_goto(tctx, ok == true, ret, done, "Unable to setup testdir\n"); > + > + ZERO_STRUCT(dhandle); > + status = torture_smb2_testdir(tree, dname, &dhandle); > + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "torture_smb2_testdir\n"); > + > + torture_comment(tctx, "Get the original sd\n"); > + > + ZERO_STRUCT(q); > + q.query_secdesc.level = RAW_FILEINFO_SEC_DESC; > + q.query_secdesc.in.file.handle = dhandle; > + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER | SECINFO_GROUP; > + status = smb2_getinfo_file(tree, tctx, &q); > + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_getinfo_file\n"); > + > + sd = q.query_secdesc.out.sd; > + owner_sid = dom_sid_string(tctx, sd->owner_sid); > + group_sid = dom_sid_string(tctx, sd->group_sid); > + torture_comment(tctx, "owner [%s] group [%s]\n", owner_sid, group_sid); > + > + torture_comment(tctx, "Set ACL with no inheritable ACE\n"); > + > + sd = security_descriptor_dacl_create(tctx, > + 0, NULL, NULL, > + owner_sid, > + SEC_ACE_TYPE_ACCESS_ALLOWED, > + SEC_RIGHTS_DIR_ALL, > + 0, > + NULL); > + > + ZERO_STRUCT(set); > + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC; > + set.set_secdesc.in.file.handle = dhandle; > + set.set_secdesc.in.secinfo_flags = SECINFO_DACL; > + set.set_secdesc.in.sd = sd; > + status = smb2_setinfo_file(tree, &set); > + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_setinfo_file\n"); > + > + TALLOC_FREE(sd); > + smb2_util_close(tree, dhandle); > + > + torture_comment(tctx, "Create file\n"); > + > + ZERO_STRUCT(fhandle); > + status = torture_smb2_testfile(tree, fname, &fhandle); > + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_create_complex_file\n"); > + > + torture_comment(tctx, "Query file SD\n"); > + > + ZERO_STRUCT(q); > + q.query_secdesc.level = RAW_FILEINFO_SEC_DESC; > + q.query_secdesc.in.file.handle = fhandle; > + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER | SECINFO_GROUP; > + status = smb2_getinfo_file(tree, tctx, &q); > + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_getinfo_file\n"); > + sd = q.query_secdesc.out.sd; > + > + smb2_util_close(tree, fhandle); > + ZERO_STRUCT(fhandle); > + > + torture_comment(tctx, "Checking actual file SD against expected SD\n"); > + > + exp_sd = security_descriptor_dacl_create( > + tctx, 0, owner_sid, group_sid, > + owner_sid, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, 0, > + SID_NT_SYSTEM, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, 0, > + NULL); > + > + CHECK_SECURITY_DESCRIPTOR(sd, exp_sd); > + > +done: > + if (!smb2_util_handle_empty(fhandle)) { > + smb2_util_close(tree, fhandle); > + } > + if (!smb2_util_handle_empty(dhandle)) { > + smb2_util_close(tree, dhandle); > + } > + if (tree != NULL) { > + smb2_deltree(tree, BASEDIR); > + smb2_tdis(tree); > + } > + > + return ret; > +} > + > +/* > + basic testing of vfs_acl_xattr > +*/ > +struct torture_suite *torture_acl_xattr(void) > +{ > + struct torture_suite *suite = torture_suite_create(talloc_autofree_context(), "acl_xattr"); > + > + torture_suite_add_1smb2_test(suite, "default-acl-style-posix", test_default_acl_posix); > + torture_suite_add_1smb2_test(suite, "default-acl-style-windows", test_default_acl_win); > + > + suite->description = talloc_strdup(suite, "vfs_acl_xattr tests"); > + > + return suite; > +} > diff --git a/source4/torture/vfs/vfs.c b/source4/torture/vfs/vfs.c > index f3ce447..7f805f4 100644 > --- a/source4/torture/vfs/vfs.c > +++ b/source4/torture/vfs/vfs.c > @@ -107,6 +107,7 @@ NTSTATUS torture_vfs_init(void) > suite->description = talloc_strdup(suite, "VFS modules tests"); > > torture_suite_add_suite(suite, torture_vfs_fruit()); > + torture_suite_add_suite(suite, torture_acl_xattr()); > > torture_register_suite(suite); > > diff --git a/source4/torture/wscript_build b/source4/torture/wscript_build > index 382e2c6..ff79c3d 100755 > --- a/source4/torture/wscript_build > +++ b/source4/torture/wscript_build > @@ -271,7 +271,7 @@ bld.SAMBA_MODULE('TORTURE_NTP', > ) > > bld.SAMBA_MODULE('TORTURE_VFS', > - source='vfs/vfs.c vfs/fruit.c', > + source='vfs/vfs.c vfs/fruit.c vfs/acl_xattr.c', > subsystem='smbtorture', > deps='LIBCLI_SMB POPT_CREDENTIALS TORTURE_UTIL smbclient-raw TORTURE_RAW', > internal_module=True, > -- > 2.7.4 >
Jeremy Allison
2016-Aug-26 23:03 UTC
[Samba] Issue with acl_xattr:ignore system acls in 4.5rc2
On Fri, Aug 26, 2016 at 02:46:19PM -0700, Jeremy Allison via samba wrote:> On Fri, Aug 26, 2016 at 06:44:05PM +0200, Ralph Böhme wrote: > > > > Cheerio! > > -slow > > Still reviewing this - but a few things that will need changing: > > When adding the validate_nt_acl_blob() function in > [PATCH 06/12] vfs_acl_common: move the ACL blob validation to a helper function > > this makes some of the existing function names in debug statements > incorrect. > > Eg. validate_nt_acl_blob() has debug statements:How about adding this on top - or squashing into it ?
Ralph Böhme
2016-Aug-27 08:14 UTC
[Samba] Issue with acl_xattr:ignore system acls in 4.5rc2
On Fri, Aug 26, 2016 at 04:03:49PM -0700, Jeremy Allison wrote:> On Fri, Aug 26, 2016 at 02:46:19PM -0700, Jeremy Allison via samba wrote: > > On Fri, Aug 26, 2016 at 06:44:05PM +0200, Ralph Böhme wrote: > > > > > > Cheerio! > > > -slow > > > > Still reviewing this - but a few things that will need changing: > > > > When adding the validate_nt_acl_blob() function in > > [PATCH 06/12] vfs_acl_common: move the ACL blob validation to a helper function > > > > this makes some of the existing function names in debug statements > > incorrect.thanks for spotting this!> > > > Eg. validate_nt_acl_blob() has debug statements: > > How about adding this on top - or squashing into it ?Updated patchset attached. My patches now (hopefully) update all DEBUG calls they touch. New patch on-top that does the rest of the file, using the new DBG_LEVEL macros in all places. Cheerio! -slow -------------- next part --------------
Reasonably Related Threads
- Issue with acl_xattr:ignore system acls in 4.5rc2
- Issue with acl_xattr:ignore system acls in 4.5rc2
- Issue with acl_xattr:ignore system acls in 4.5rc2
- Security permissions issues after changing idmap backend from RID to AUTORID
- Security permissions issues after changing idmap backend from RID to AUTORID