[PATCH] Rework patch for bug 6133

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Rework patch for bug 6133

Samba - samba-technical mailing list
Hi!

Attached is a patch that moves ACE4_ADD_FILE -> ACE4_DELETE_CHILD mapping from
the NFSv4 framework to vfs_zfsacl.

This was added in e6a5f11865a55e9644292ae92e4a4b5ec0662ccd to adopt the NFSv4
framework to follow ZFS permission rules. But this is the wrong place, other
filesystems like GPFS do not allow deletion when the user has SEC_DIR_ADD_FILE,
so setting ACE4_DELETE_CHILD when the access_mask has ACE4_ADD_FILE is wrong:

# su -s /bin/bash FOO\\aduser1
bash-4.2$ mmgetacl .
#NFSv4 ACL
#owner:FOO\aduser1
#group:FOO\domain users
special:owner@:rwxc:allow:FileInherit:DirInherit:InheritOnly
 (X)READ/LIST (X)WRITE/CREATE (X)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL  (X)READ_ATTR  (X)READ_NAMED
 (X)DELETE    (X)DELETE_CHILD (X)CHOWN        (X)EXEC/SEARCH (X)WRITE_ACL (X)WRITE_ATTR (X)WRITE_NAMED

user:FOO\aduser1:rwx-:allow:FileInherit:DirInherit
 (X)READ/LIST (X)WRITE/CREATE (X)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL  (X)READ_ATTR  (X)READ_NAMED
 (-)DELETE    (-)DELETE_CHILD (-)CHOWN        (X)EXEC/SEARCH (-)WRITE_ACL (-)WRITE_ATTR (-)WRITE_NAMED

The ACL has an explicit ACE for the user that grants CREATE, but not
DELETE_CHILD, so the user can't delete the file:

bash-4.2$ rm file
rm: cannot remove ‘file’: Permission denied
bash-4.2$

Adding DELETE_CHILD allows the user to delete the file:
bash-4.2$ rm file
bash-4.2$

This patch therefor moves the change from the NFS4 framework into the ZFS
module.

Please review & push if ok. Thanks!

Cheerio!
-slow

bug6133.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Rework patch for bug 6133

Samba - samba-technical mailing list
On Thu, Sep 07, 2017 at 12:27:53PM +0200, Ralph Böhme wrote:

> Hi!
>
> Attached is a patch that moves ACE4_ADD_FILE -> ACE4_DELETE_CHILD mapping from
> the NFSv4 framework to vfs_zfsacl.
>
> This was added in e6a5f11865a55e9644292ae92e4a4b5ec0662ccd to adopt the NFSv4
> framework to follow ZFS permission rules. But this is the wrong place, other
> filesystems like GPFS do not allow deletion when the user has SEC_DIR_ADD_FILE,
> so setting ACE4_DELETE_CHILD when the access_mask has ACE4_ADD_FILE is wrong:
>
> # su -s /bin/bash FOO\\aduser1
> bash-4.2$ mmgetacl .
> #NFSv4 ACL
> #owner:FOO\aduser1
> #group:FOO\domain users
> special:owner@:rwxc:allow:FileInherit:DirInherit:InheritOnly
>  (X)READ/LIST (X)WRITE/CREATE (X)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL  (X)READ_ATTR  (X)READ_NAMED
>  (X)DELETE    (X)DELETE_CHILD (X)CHOWN        (X)EXEC/SEARCH (X)WRITE_ACL (X)WRITE_ATTR (X)WRITE_NAMED
>
> user:FOO\aduser1:rwx-:allow:FileInherit:DirInherit
>  (X)READ/LIST (X)WRITE/CREATE (X)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL  (X)READ_ATTR  (X)READ_NAMED
>  (-)DELETE    (-)DELETE_CHILD (-)CHOWN        (X)EXEC/SEARCH (-)WRITE_ACL (-)WRITE_ATTR (-)WRITE_NAMED
>
> The ACL has an explicit ACE for the user that grants CREATE, but not
> DELETE_CHILD, so the user can't delete the file:
>
> bash-4.2$ rm file
> rm: cannot remove ‘file’: Permission denied
> bash-4.2$
>
> Adding DELETE_CHILD allows the user to delete the file:
> bash-4.2$ rm file
> bash-4.2$
>
> This patch therefor moves the change from the NFS4 framework into the ZFS
> module.
>
> Please review & push if ok. Thanks!

LGTM. Pushed !

> From 9aebdfcc39b48d06e122570186554c5c302006be Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Wed, 6 Sep 2017 16:44:12 +0200
> Subject: [PATCH 1/3] vfs_zfsacl: pass smb_fname to zfs_get_nt_acl_common
>
> This is in preperation of moving SMB_ACE4_ADD_FILE /
> SMB_ACE4_DELETE_CHILD mapping from the common NFSv4 framework into this
> module excusively.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=6133
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/modules/vfs_zfsacl.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/source3/modules/vfs_zfsacl.c b/source3/modules/vfs_zfsacl.c
> index 4cb1b98f01b..97fe2028307 100644
> --- a/source3/modules/vfs_zfsacl.c
> +++ b/source3/modules/vfs_zfsacl.c
> @@ -41,7 +41,7 @@
>   * using the NFSv4 format conversion
>   */
>  static NTSTATUS zfs_get_nt_acl_common(TALLOC_CTX *mem_ctx,
> -      const char *name,
> +      const struct smb_filename *smb_fname,
>        struct SMB4ACL_T **ppacl)
>  {
>   int naces, i;
> @@ -49,13 +49,13 @@ static NTSTATUS zfs_get_nt_acl_common(TALLOC_CTX *mem_ctx,
>   struct SMB4ACL_T *pacl;
>  
>   /* read the number of file aces */
> - if((naces = acl(name, ACE_GETACLCNT, 0, NULL)) == -1) {
> + if((naces = acl(smb_fname->base_name, ACE_GETACLCNT, 0, NULL)) == -1) {
>   if(errno == ENOSYS) {
>   DEBUG(9, ("acl(ACE_GETACLCNT, %s): Operation is not "
>    "supported on the filesystem where the file "
> -  "reside\n", name));
> +  "reside\n", smb_fname->base_name));
>   } else {
> - DEBUG(9, ("acl(ACE_GETACLCNT, %s): %s ", name,
> + DEBUG(9, ("acl(ACE_GETACLCNT, %s): %s ", smb_fname->base_name,
>   strerror(errno)));
>   }
>   return map_nt_error_from_unix(errno);
> @@ -67,8 +67,8 @@ static NTSTATUS zfs_get_nt_acl_common(TALLOC_CTX *mem_ctx,
>   return NT_STATUS_NO_MEMORY;
>   }
>   /* read the aces into the field */
> - if(acl(name, ACE_GETACL, naces, acebuf) < 0) {
> - DEBUG(9, ("acl(ACE_GETACL, %s): %s ", name,
> + if(acl(smb_fname->base_name, ACE_GETACL, naces, acebuf) < 0) {
> + DEBUG(9, ("acl(ACE_GETACL, %s): %s ", smb_fname->base_name,
>   strerror(errno)));
>   return map_nt_error_from_unix(errno);
>   }
> @@ -210,9 +210,7 @@ static NTSTATUS zfsacl_fget_nt_acl(struct vfs_handle_struct *handle,
>   NTSTATUS status;
>   TALLOC_CTX *frame = talloc_stackframe();
>  
> - status = zfs_get_nt_acl_common(frame,
> -       fsp->fsp_name->base_name,
> -       &pacl);
> + status = zfs_get_nt_acl_common(frame, fsp->fsp_name, &pacl);
>   if (!NT_STATUS_IS_OK(status)) {
>   TALLOC_FREE(frame);
>   return status;
> @@ -234,9 +232,7 @@ static NTSTATUS zfsacl_get_nt_acl(struct vfs_handle_struct *handle,
>   NTSTATUS status;
>   TALLOC_CTX *frame = talloc_stackframe();
>  
> - status = zfs_get_nt_acl_common(frame,
> - smb_fname->base_name,
> - &pacl);
> + status = zfs_get_nt_acl_common(frame, smb_fname, &pacl);
>   if (!NT_STATUS_IS_OK(status)) {
>   TALLOC_FREE(frame);
>   return status;
> --
> 2.13.5
>
>
> From 8d6be0aa8ff81ed7645839ce7a37fd74201abdd0 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Wed, 6 Sep 2017 16:53:23 +0200
> Subject: [PATCH 2/3] vfs_zfsacl: ensure zfs_get_nt_acl_common() has access to
>  stat info
>
> We'll need this in the next commit.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=6133
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/modules/vfs_zfsacl.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/source3/modules/vfs_zfsacl.c b/source3/modules/vfs_zfsacl.c
> index 97fe2028307..da13c4c4908 100644
> --- a/source3/modules/vfs_zfsacl.c
> +++ b/source3/modules/vfs_zfsacl.c
> @@ -40,13 +40,31 @@
>   * read the local file's acls and return it in NT form
>   * using the NFSv4 format conversion
>   */
> -static NTSTATUS zfs_get_nt_acl_common(TALLOC_CTX *mem_ctx,
> +static NTSTATUS zfs_get_nt_acl_common(struct connection_struct *conn,
> +      TALLOC_CTX *mem_ctx,
>        const struct smb_filename *smb_fname,
>        struct SMB4ACL_T **ppacl)
>  {
>   int naces, i;
>   ace_t *acebuf;
>   struct SMB4ACL_T *pacl;
> + SMB_STRUCT_STAT sbuf;
> + const SMB_STRUCT_STAT *psbuf = NULL;
> + int ret;
> +
> + if (VALID_STAT(smb_fname->st)) {
> + psbuf = &smb_fname->st;
> + }
> +
> + if (psbuf == NULL) {
> + ret = vfs_stat_smb_basename(conn, smb_fname, &sbuf);
> + if (ret != 0) {
> + DBG_INFO("stat [%s]failed: %s\n",
> + smb_fname_str_dbg(smb_fname), strerror(errno));
> + return map_nt_error_from_unix(errno);
> + }
> + psbuf = &sbuf;
> + }
>  
>   /* read the number of file aces */
>   if((naces = acl(smb_fname->base_name, ACE_GETACLCNT, 0, NULL)) == -1) {
> @@ -210,7 +228,8 @@ static NTSTATUS zfsacl_fget_nt_acl(struct vfs_handle_struct *handle,
>   NTSTATUS status;
>   TALLOC_CTX *frame = talloc_stackframe();
>  
> - status = zfs_get_nt_acl_common(frame, fsp->fsp_name, &pacl);
> + status = zfs_get_nt_acl_common(handle->conn, frame,
> +       fsp->fsp_name, &pacl);
>   if (!NT_STATUS_IS_OK(status)) {
>   TALLOC_FREE(frame);
>   return status;
> @@ -232,7 +251,7 @@ static NTSTATUS zfsacl_get_nt_acl(struct vfs_handle_struct *handle,
>   NTSTATUS status;
>   TALLOC_CTX *frame = talloc_stackframe();
>  
> - status = zfs_get_nt_acl_common(frame, smb_fname, &pacl);
> + status = zfs_get_nt_acl_common(handle->conn, frame, smb_fname, &pacl);
>   if (!NT_STATUS_IS_OK(status)) {
>   TALLOC_FREE(frame);
>   return status;
> --
> 2.13.5
>
>
> From 216abbcf5ea028a5091722d5d9024fe6728f5794 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Wed, 6 Sep 2017 16:56:47 +0200
> Subject: [PATCH 3/3] s3/vfs: move ACE4_ADD_FILE/ACE4_DELETE_CHILD mapping from
>  NFSv4 framework to vfs_zfsacl
>
> This was added in e6a5f11865a55e9644292ae92e4a4b5ec0662ccd to adopt the
> NFSv4 framework to follow ZFS permission rules. But this is the wrong
> place, other filesystems like GPFS do not allow deletion when the user
> has SEC_DIR_ADD_FILE.
>
> This patch therefor moves the change from the NFS4 framework into the
> ZFS module.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=6133
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/modules/nfs4_acls.c  | 4 ----
>  source3/modules/vfs_zfsacl.c | 4 ++++
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/source3/modules/nfs4_acls.c b/source3/modules/nfs4_acls.c
> index c715b83390d..19f0fefdb98 100644
> --- a/source3/modules/nfs4_acls.c
> +++ b/source3/modules/nfs4_acls.c
> @@ -351,10 +351,6 @@ static bool smbacl4_nfs42win(TALLOC_CTX *mem_ctx,
>   DEBUG(10, ("mapped %d to %s\n", ace->who.id,
>     sid_string_dbg(&sid)));
>  
> - if (is_directory && (ace->aceMask & SMB_ACE4_ADD_FILE)) {
> - ace->aceMask |= SMB_ACE4_DELETE_CHILD;
> - }
> -
>   if (!is_directory && params->map_full_control) {
>   /*
>   * Do we have all access except DELETE_CHILD
> diff --git a/source3/modules/vfs_zfsacl.c b/source3/modules/vfs_zfsacl.c
> index da13c4c4908..dd0f343b8c6 100644
> --- a/source3/modules/vfs_zfsacl.c
> +++ b/source3/modules/vfs_zfsacl.c
> @@ -66,6 +66,10 @@ static NTSTATUS zfs_get_nt_acl_common(struct connection_struct *conn,
>   psbuf = &sbuf;
>   }
>  
> + if (S_ISDIR(psbuf->st_ex_mode) && (ace->aceMask & SMB_ACE4_ADD_FILE)) {
> + ace->aceMask |= SMB_ACE4_DELETE_CHILD;
> + }
> +
>   /* read the number of file aces */
>   if((naces = acl(smb_fname->base_name, ACE_GETACLCNT, 0, NULL)) == -1) {
>   if(errno == ENOSYS) {
> --
> 2.13.5
>