lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zu3K34MHFUYNaRfu@tissot.1015granger.net>
Date: Fri, 20 Sep 2024 15:19:59 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Pali Rohár <pali@...nel.org>
Cc: Jeff Layton <jlayton@...nel.org>, Neil Brown <neilb@...e.de>,
        Olga Kornievskaia <okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>,
        Tom Talpey <tom@...pey.com>, linux-nfs@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nfsd: Add support for mapping sticky bit into NFS4 ACL

On Tue, Sep 17, 2024 at 06:10:50PM +0200, Pali Rohár wrote:
> On Tuesday 17 September 2024 11:39:58 Chuck Lever wrote:
> > On Fri, Sep 13, 2024 at 12:15:23AM +0200, Pali Rohár wrote:
> > > Currently the directory restricted deletion flag (sticky bit, SVTX) is not
> > > mapped into NFS4 ACL by knfsd. Which means that the NFS4 ACL client is not
> > > aware of the fact that it cannot delete some files if the sticky bit is set
> > > on the server on parent directory. If the client copies sticky bit
> > > directory recursively to some other storage which implements the NFS4-like
> > > ACL (e.g. to NTFS filesystem or to SMB server) then the directory's
> > > restricted deletion flag is completely lost.
> > > 
> > > This change aims to improve interoperability of the POSIX directory
> > > restricted deletion flag (sticky bit, SVTX) and NFS4 ACL structure in
> > > knfsd. It transparently maps POSIX directory's sticky bit into NFS4 ACL
> > > and vice-versa similarly like it already maps POSIX ACL into NFS4 ACL.
> > > It covers NFS4 GETATTR ACL, NFS4 SETATTR ACL, NFS4 CREATE+OPEN operations.
> > > When creating a new object over NFS4, it is possible to optionally specify
> > > NFS4 ACL, so this is covered too.
> > > 
> > > For client SETATTR ACL, CREATE with ACL and OPEN-create with ACL
> > > operations, detection pattern for restricted deletion flag is ACE entry
> > > INHERIT_ONLY NO_PROPAGATE ALLOW OWNER@ DELETE together with check that
> > > nobody except the OWNER@ has DELETE_CHILD permission. Note that the OWNER@
> > > does not have to have DELETE_CHILD permission in case it does not have
> > > write access to directory.
> > > 
> > > For client GETATTR ACL operation, when restricted deletion flag is present
> > > in inode, following ACE entries are prepended before any other ACEs:
> > > 
> > >   ALLOW OWNER@ DELETE_CHILD                                 (1)
> > >   DENY EVERYONE@ DELETE_CHILD
> > >   INHERIT_ONLY NO_PROPAGATE DENY user_owner_of_dir DELETE   (3)
> > >   INHERIT_ONLY NO_PROPAGATE DENY OWNER@ DELETE              (4)
> > >   INHERIT_ONLY NO_PROPAGATE DENY user1 DELETE               (ACL user1)
> > >   ...
> > >   INHERIT_ONLY NO_PROPAGATE DENY userN DELETE               (ACL userN)
> > >   INHERIT_ONLY NO_PROPAGATE ALLOW OWNER@ DELETE
> > > 
> > > ACE entry (1) is present only when user OWNER@ has write access (can modify
> > > directory, including removing child entries), because it is possible to
> > > have sticky bit set also on read-only directory.
> > > 
> > > ACE entry (3) is present only when user OWNER@ does not have write access
> > > (cannot modify directory) and it is required to override effect of the last
> > > ACE entry which allows child entry OWNER@ to remove entry itself. This is
> > > needed for example for POSIX mode 1577.
> > > 
> > > ACE entry (4) is present only when anybody else except the directory owner
> > > has no write access to the directory. This is the case when sticky bit is
> > > set but nobody can use it because of missing directory write access. So
> > > this explicit DENY covers this edge case.
> > > 
> > > ACE entries (ACL user1...userN) are for POSIX users which do not have write
> > > access to directory and therefore they cannot remove directory entries
> > > which they own. This is again needed for overriding the effect of the last
> > > ACE entry.
> > > 
> > > When restricted deletion flag is not present then nothing is added.
> > > 
> > > This is probably the best approximation of the directory restricted
> > > deletion flag. It covers directory's OWNER@ grant access, child OWNER@
> > > grant access and also restrictions for all other users. It also covers the
> > > situation when OWNER@ or some POSIX user does not have write access to the
> > > directory, and also covers situation when nobody except directory owner has
> > > write access to the directory.
> > > 
> > > What this does not cover is the restriction when some POSIX group does not
> > > have directory write access, and another POSIX group has directory write
> > > access. This is probably not possible to express in NFS4 ACL language
> > > without ability to evaluate if user is member of some group or not. NFS4
> > > ACL in this case says for the owner of the file that the delete operation
> > > is allowed.
> > > 
> > > The whole change is only about the mapping of the sticky bit into NFS4 ACL
> > > and only for NFS4 GET and SET ACL operations. It does not change any access
> > > permission checks.
> > > 
> > > Signed-off-by: Pali Rohár <pali@...nel.org>
> > > ---
> > >  fs/nfsd/acl.h      |   2 +-
> > >  fs/nfsd/nfs4acl.c  | 242 ++++++++++++++++++++++++++++++++++++++++++---
> > >  fs/nfsd/nfs4proc.c |  31 +++++-
> > >  3 files changed, 255 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/acl.h b/fs/nfsd/acl.h
> > > index 4b7324458a94..e7e7909bf03a 100644
> > > --- a/fs/nfsd/acl.h
> > > +++ b/fs/nfsd/acl.h
> > > @@ -48,6 +48,6 @@ __be32 nfs4_acl_write_who(struct xdr_stream *xdr, int who);
> > >  int nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry,
> > >  		struct nfs4_acl **acl);
> > >  __be32 nfsd4_acl_to_attr(enum nfs_ftype4 type, struct nfs4_acl *acl,
> > > -			 struct nfsd_attrs *attr);
> > > +			 struct nfsd_attrs *attr, int *dir_sticky);
> > >  
> > >  #endif /* LINUX_NFS4_ACL_H */
> > > diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> > > index 96e786b5e544..6a53772c2a13 100644
> > > --- a/fs/nfsd/nfs4acl.c
> > > +++ b/fs/nfsd/nfs4acl.c
> > > @@ -46,6 +46,7 @@
> > >  #define NFS4_ACL_TYPE_DEFAULT	0x01
> > >  #define NFS4_ACL_DIR		0x02
> > >  #define NFS4_ACL_OWNER		0x04
> > > +#define NFS4_ACL_DIR_STICKY	0x08
> > >  
> > >  /* mode bit translations: */
> > >  #define NFS4_READ_MODE (NFS4_ACE_READ_DATA)
> > > @@ -73,7 +74,7 @@ mask_from_posix(unsigned short perm, unsigned int flags)
> > >  		mask |= NFS4_READ_MODE;
> > >  	if (perm & ACL_WRITE)
> > >  		mask |= NFS4_WRITE_MODE;
> > > -	if ((perm & ACL_WRITE) && (flags & NFS4_ACL_DIR))
> > > +	if ((perm & ACL_WRITE) && (flags & NFS4_ACL_DIR) && !(flags & NFS4_ACL_DIR_STICKY))
> > >  		mask |= NFS4_ACE_DELETE_CHILD;
> > >  	if (perm & ACL_EXECUTE)
> > >  		mask |= NFS4_EXECUTE_MODE;
> > > @@ -89,7 +90,7 @@ deny_mask_from_posix(unsigned short perm, u32 flags)
> > >  		mask |= NFS4_READ_MODE;
> > >  	if (perm & ACL_WRITE)
> > >  		mask |= NFS4_WRITE_MODE;
> > > -	if ((perm & ACL_WRITE) && (flags & NFS4_ACL_DIR))
> > > +	if ((perm & ACL_WRITE) && (flags & NFS4_ACL_DIR) && !(flags & NFS4_ACL_DIR_STICKY))
> > >  		mask |= NFS4_ACE_DELETE_CHILD;
> > >  	if (perm & ACL_EXECUTE)
> > >  		mask |= NFS4_EXECUTE_MODE;
> > > @@ -110,7 +111,7 @@ low_mode_from_nfs4(u32 perm, unsigned short *mode, unsigned int flags)
> > >  {
> > >  	u32 write_mode = NFS4_WRITE_MODE;
> > >  
> > > -	if (flags & NFS4_ACL_DIR)
> > > +	if ((flags & NFS4_ACL_DIR) && !(flags | NFS4_ACL_DIR_STICKY))
> > >  		write_mode |= NFS4_ACE_DELETE_CHILD;
> > >  	*mode = 0;
> > >  	if ((perm & NFS4_READ_MODE) == NFS4_READ_MODE)
> > > @@ -123,7 +124,7 @@ low_mode_from_nfs4(u32 perm, unsigned short *mode, unsigned int flags)
> > >  
> > >  static short ace2type(struct nfs4_ace *);
> > >  static void _posix_to_nfsv4_one(struct posix_acl *, struct nfs4_acl *,
> > > -				unsigned int);
> > > +				unsigned int, kuid_t);
> > >  
> > >  int
> > >  nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry,
> > > @@ -142,8 +143,11 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry,
> > >  	if (IS_ERR(pacl))
> > >  		return PTR_ERR(pacl);
> > >  
> > > -	/* allocate for worst case: one (deny, allow) pair each: */
> > > -	size += 2 * pacl->a_count;
> > > +	/*
> > > +	 * allocate for worst case: one (deny, allow) pair for each
> > > +	 * plus for restricted deletion flag (sticky bit): 4 + one for each
> > > +	 */
> > > +	size += 2 * pacl->a_count + (4 + pacl->a_count);
> > >  
> > >  	if (S_ISDIR(inode->i_mode)) {
> > >  		flags = NFS4_ACL_DIR;
> > > @@ -155,6 +159,10 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry,
> > >  
> > >  		if (dpacl)
> > >  			size += 2 * dpacl->a_count;
> > > +
> > > +		/* propagate restricted deletion flag (sticky bit) */
> > > +		if (inode->i_mode & S_ISVTX)
> > > +			flags |= NFS4_ACL_DIR_STICKY;
> > >  	}
> > >  
> > >  	*acl = kmalloc(nfs4_acl_bytes(size), GFP_KERNEL);
> > > @@ -164,10 +172,10 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry,
> > >  	}
> > >  	(*acl)->naces = 0;
> > >  
> > > -	_posix_to_nfsv4_one(pacl, *acl, flags & ~NFS4_ACL_TYPE_DEFAULT);
> > > +	_posix_to_nfsv4_one(pacl, *acl, flags & ~NFS4_ACL_TYPE_DEFAULT, inode->i_uid);
> > >  
> > >  	if (dpacl)
> > > -		_posix_to_nfsv4_one(dpacl, *acl, flags | NFS4_ACL_TYPE_DEFAULT);
> > > +		_posix_to_nfsv4_one(dpacl, *acl, (flags | NFS4_ACL_TYPE_DEFAULT) & ~NFS4_ACL_DIR_STICKY, inode->i_uid);
> > >  
> > >  out:
> > >  	posix_acl_release(dpacl);
> > > @@ -231,7 +239,7 @@ summarize_posix_acl(struct posix_acl *acl, struct posix_acl_summary *pas)
> > >  /* We assume the acl has been verified with posix_acl_valid. */
> > >  static void
> > >  _posix_to_nfsv4_one(struct posix_acl *pacl, struct nfs4_acl *acl,
> > > -						unsigned int flags)
> > > +		    unsigned int flags, kuid_t owner_uid)
> > >  {
> > >  	struct posix_acl_entry *pa, *group_owner_entry;
> > >  	struct nfs4_ace *ace;
> > > @@ -243,9 +251,149 @@ _posix_to_nfsv4_one(struct posix_acl *pacl, struct nfs4_acl *acl,
> > >  	BUG_ON(pacl->a_count < 3);
> > >  	summarize_posix_acl(pacl, &pas);
> > >  
> > > -	pa = pacl->a_entries;
> > >  	ace = acl->aces + acl->naces;
> > >  
> > > +	/*
> > > +	 * Translate restricted deletion flag (sticky bit, SVTX) set on the
> > > +	 * directory to following NFS4 ACEs prepended before any other ACEs:
> > > +	 *
> > > +	 *   ALLOW OWNER@ DELETE_CHILD                                 (1)
> > > +	 *   DENY EVERYONE@ DELETE_CHILD
> > > +	 *   INHERIT_ONLY NO_PROPAGATE DENY user_owner_of_dir DELETE   (3)
> > > +	 *   INHERIT_ONLY NO_PROPAGATE DENY OWNER@ DELETE              (4)
> > > +	 *   INHERIT_ONLY NO_PROPAGATE DENY user1 DELETE               (ACL user1)
> > > +	 *   ...
> > > +	 *   INHERIT_ONLY NO_PROPAGATE DENY userN DELETE               (ACL userN)
> > > +	 *   INHERIT_ONLY NO_PROPAGATE ALLOW OWNER@ DELETE
> > > +	 *
> > > +	 *   (1) - only if user-owner has write access
> > > +	 *   (3) - only if user-owner does not have write access
> > > +	 *   (4) - only if non-user-owner does not have write access
> > > +	 *   (ACL user1) - only if user1 does not have write access
> > > +	 *   (ACL userN) - only if userN does not have write access
> > > +	 *
> > > +	 * These ACEs describe behavior of set restricted deletion flag (sticky
> > > +	 * bit) on directory as they allow only owner of individual child entries
> > > +	 * and owner of the directory to delete individual child entries.
> > > +	 * Everyone else is denied to remove child entries in this directory.
> > > +	 *
> > > +	 * For deleting entry in NFS4 ACL model it is needed either DELETE_CHILD
> > > +	 * permission (access mask) from the parent directory or DELETE permission
> > > +	 * (access mask) on the child. Just one of these two permissions is enough.
> > > +	 * So if there is explicit DENY DELETE_CHILD on the parent together with
> > > +	 * explicit ALLOW DELETE on the child, it means that deleting is allowed
> > > +	 * (evaluation of permissions is independent in NFS4 ACL model). OWNER@
> > > +	 * for inherited ACEs means owner of the child entry and not the owner
> > > +	 * of the parent from which was ACE inherited.
> > > +	 *
> > > +	 * This translation is imperfect just for a case when some group
> > > +	 * (including group-owner and others-group) does not have write access
> > > +	 * to directory. Handled is only the edge case (via rule 4) when
> > > +	 * everyone else except owner has no write access to the directory.
> > > +	 * This information is not present in NFS4 ACL because it looks like that
> > > +	 * this is not possible to express in this ACL model. So for ACL consumer
> > > +	 * it could look like that owner of the file can delete its own file even
> > > +	 * when group or other mode bits of the directory do not allow it.
> > > +	 */
> > > +	if (flags & NFS4_ACL_DIR_STICKY) {
> > > +		/*
> > > +		 * Explicitly allow directory owner to delete child entries
> > > +		 * if directory owner has write access
> > > +		 */
> > > +		if (pas.owner & ACL_WRITE) {
> > > +			ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
> > > +			ace->flag = 0;
> > > +			ace->access_mask = NFS4_ACE_DELETE_CHILD;
> > > +			ace->whotype = NFS4_ACL_WHO_OWNER;
> > > +			ace++;
> > > +			acl->naces++;
> > > +		}
> > > +
> > > +		/*
> > > +		 * And then deny deleting child entries for all other users
> > > +		 * which do not have explicit DELETE permission granted by
> > > +		 * last rule in this section.
> > > +		 */
> > > +		ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
> > > +		ace->flag = 0;
> > > +		ace->access_mask = NFS4_ACE_DELETE_CHILD;
> > > +		ace->whotype = NFS4_ACL_WHO_EVERYONE;
> > > +		ace++;
> > > +		acl->naces++;
> > > +
> > > +		/*
> > > +		 * Do not grant directory owner to delete child entries (by the
> > > +		 * last rule in this section) if it does not have write access.
> > > +		 */
> > > +		if (!(pas.owner & ACL_WRITE)) {
> > > +			ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
> > > +			ace->flag = NFS4_INHERITANCE_FLAGS |
> > > +				    NFS4_ACE_INHERIT_ONLY_ACE |
> > > +				    NFS4_ACE_NO_PROPAGATE_INHERIT_ACE;
> > > +			ace->access_mask = NFS4_ACE_DELETE;
> > > +			ace->whotype = NFS4_ACL_WHO_NAMED;
> > > +			ace->who_uid = owner_uid;
> > > +			ace++;
> > > +			acl->naces++;
> > > +		}
> > > +
> > > +		if (!(pas.users & ACL_WRITE) && !(pas.group & ACL_WRITE) &&
> > > +		    !(pas.groups & ACL_WRITE) && !(pas.other & ACL_WRITE)) {
> > > +			/*
> > > +			 * Do not grant child owner who is not directory owner
> > > +			 * (handled by the first rule in this section) to
> > > +			 * delete own child entries if there is no possible
> > > +			 * directory write permission (checked for named users,
> > > +			 * group-owner, named groups and others-groups).
> > > +			 * This handles special edge case when only directory
> > > +			 * owner has write access to directory.
> > > +			 */
> > > +			ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
> > > +			ace->flag = NFS4_INHERITANCE_FLAGS |
> > > +				    NFS4_ACE_INHERIT_ONLY_ACE |
> > > +				    NFS4_ACE_NO_PROPAGATE_INHERIT_ACE;
> > > +			ace->access_mask = NFS4_ACE_DELETE;
> > > +			ace->whotype = NFS4_ACL_WHO_OWNER;
> > > +			ace++;
> > > +			acl->naces++;
> > > +		} else {
> > > +			/*
> > > +			 * Do not grant individual named users to delete child
> > > +			 * entries (by the last rule in this section) if user
> > > +			 * does not have write access to directory.
> > > +			 */
> > > +			for (pa = pacl->a_entries + 1; pa->e_tag == ACL_USER; pa++) {
> > > +				if (pa->e_perm & pas.mask & ACL_WRITE)
> > > +					continue;
> > > +				ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
> > > +				ace->flag = NFS4_INHERITANCE_FLAGS |
> > > +					    NFS4_ACE_INHERIT_ONLY_ACE |
> > > +					    NFS4_ACE_NO_PROPAGATE_INHERIT_ACE;
> > > +				ace->access_mask = NFS4_ACE_DELETE;
> > > +				ace->whotype = NFS4_ACL_WHO_NAMED;
> > > +				ace->who_uid = pa->e_uid;
> > > +				ace++;
> > > +				acl->naces++;
> > > +			}
> > > +		}
> > > +
> > > +		/*
> > > +		 * Above rules filtered out users which do not have write access
> > > +		 * to the directory. Now allow child-owner to delete its own
> > > +		 * directory entries.
> > > +		 */
> > > +		ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
> > > +		ace->flag = NFS4_INHERITANCE_FLAGS |
> > > +			    NFS4_ACE_INHERIT_ONLY_ACE |
> > > +			    NFS4_ACE_NO_PROPAGATE_INHERIT_ACE;
> > > +		ace->access_mask = NFS4_ACE_DELETE;
> > > +		ace->whotype = NFS4_ACL_WHO_OWNER;
> > > +		ace++;
> > > +		acl->naces++;
> > > +	}
> > > +
> > > +	pa = pacl->a_entries;
> > > +
> > >  	/* We could deny everything not granted by the owner: */
> > >  	deny = ~pas.owner;
> > >  	/*
> > > @@ -517,7 +665,8 @@ posix_state_to_acl(struct posix_acl_state *state, unsigned int flags)
> > >  
> > >  	pace = pacl->a_entries;
> > >  	pace->e_tag = ACL_USER_OBJ;
> > > -	low_mode_from_nfs4(state->owner.allow, &pace->e_perm, flags);
> > > +	/* owner is never affected by restricted deletion flag, so clear NFS4_ACL_DIR_STICKY */
> > > +	low_mode_from_nfs4(state->owner.allow, &pace->e_perm, flags & ~NFS4_ACL_DIR_STICKY);
> > >  
> > >  	for (i=0; i < state->users->n; i++) {
> > >  		pace++;
> > > @@ -691,9 +840,14 @@ static void process_one_v4_ace(struct posix_acl_state *state,
> > >  
> > >  static int nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl,
> > >  		struct posix_acl **pacl, struct posix_acl **dpacl,
> > > +		int *dir_sticky,
> > >  		unsigned int flags)
> > >  {
> > >  	struct posix_acl_state effective_acl_state, default_acl_state;
> > > +	bool dir_allow_nonowner_delete_child = false;
> > > +	bool dir_deny_everyone_delete_child = false;
> > > +	bool dir_allow_child_owner_delete = false;
> > > +	unsigned int eflags = 0;
> > >  	struct nfs4_ace *ace;
> > >  	int ret;
> > >  
> > > @@ -705,6 +859,28 @@ static int nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl,
> > >  		goto out_estate;
> > >  	ret = -EINVAL;
> > >  	for (ace = acl->aces; ace < acl->aces + acl->naces; ace++) {
> > > +		/*
> > > +		 * Process and parse ACE entry INHERIT_ONLY NO_PROPAGATE DELETE
> > > +		 * for detecting restricted deletion flag (sticky bit). Allow
> > > +		 * SYNCHRONIZE access mask to be present as NFS4 clients can
> > > +		 * include this access mask together with any other one.
> > > +		 *
> > > +		 * It needs to be done before validation as NFS4_SUPPORTED_FLAGS
> > > +		 * does not contain NFS4_ACE_NO_PROPAGATE_INHERIT_ACE and this
> > > +		 * ACE must not throw error.
> > > +		 */
> > > +		if ((flags & NFS4_ACL_DIR) &&
> > > +		    !(ace->flag & ~(NFS4_SUPPORTED_FLAGS|NFS4_ACE_NO_PROPAGATE_INHERIT_ACE)) &&
> > > +		    (ace->flag & NFS4_INHERITANCE_FLAGS) &&
> > > +		    (ace->flag & NFS4_ACE_INHERIT_ONLY_ACE) &&
> > > +		    (ace->flag & NFS4_ACE_NO_PROPAGATE_INHERIT_ACE) &&
> > > +		    (ace->access_mask & ~NFS4_ACE_SYNCHRONIZE) == NFS4_ACE_DELETE) {
> > > +			if (ace->type == NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE &&
> > > +			    ace->whotype == NFS4_ACL_WHO_OWNER)
> > > +				dir_allow_child_owner_delete = true;
> > > +			continue;
> > > +		}
> > > +
> > >  		if (ace->type != NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE &&
> > >  		    ace->type != NFS4_ACE_ACCESS_DENIED_ACE_TYPE)
> > >  			goto out_dstate;
> > > @@ -725,6 +901,38 @@ static int nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl,
> > >  
> > >  		if (!(ace->flag & NFS4_ACE_INHERIT_ONLY_ACE))
> > >  			process_one_v4_ace(&effective_acl_state, ace);
> > > +
> > > +		/*
> > > +		 * Process and parse ACE entry with DELETE_CHILD access mask
> > > +		 * for detecting restricted deletion flag (sticky bit).
> > > +		 */
> > > +		if ((flags & NFS4_ACL_DIR) &&
> > > +		    !(ace->flag & NFS4_ACE_INHERIT_ONLY_ACE) &&
> > > +		    (ace->access_mask & NFS4_ACE_DELETE_CHILD)) {
> > > +			if (ace->type == NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE &&
> > > +			    !dir_deny_everyone_delete_child &&
> > > +			    ace->whotype != NFS4_ACL_WHO_OWNER)
> > > +				dir_allow_nonowner_delete_child = true;
> > > +			else if (ace->type == NFS4_ACE_ACCESS_DENIED_ACE_TYPE &&
> > > +				 ace->whotype == NFS4_ACL_WHO_EVERYONE)
> > > +				dir_deny_everyone_delete_child = true;
> > > +		}
> > > +	}
> > > +
> > > +	/*
> > > +	 * Recognize restricted deletion flag (sticky bit) from directory ACL
> > > +	 * if ACEs on directory allow only owner of directory child entry to
> > > +	 * delete entry itself.
> > > +	 *
> > > +	 * This is relaxed check for rules generated by _posix_to_nfsv4_one().
> > > +	 * Relaxed check of restricted deletion flag is for security reasons
> > > +	 * and means that permissions would be more stricter, to prevent
> > > +	 * granting more access than what was specified in NFS4 ACL packet.
> > > +	 */
> > > +	if (flags & NFS4_ACL_DIR) {
> > > +		*dir_sticky = !dir_allow_nonowner_delete_child && dir_allow_child_owner_delete;
> > > +		if (*dir_sticky)
> > > +			eflags |= NFS4_ACL_DIR_STICKY;
> > >  	}
> > >  
> > >  	/*
> > > @@ -750,7 +958,7 @@ static int nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl,
> > >  			default_acl_state.other = effective_acl_state.other;
> > >  	}
> > >  
> > > -	*pacl = posix_state_to_acl(&effective_acl_state, flags);
> > > +	*pacl = posix_state_to_acl(&effective_acl_state, flags | eflags);
> > >  	if (IS_ERR(*pacl)) {
> > >  		ret = PTR_ERR(*pacl);
> > >  		*pacl = NULL;
> > > @@ -776,19 +984,23 @@ static int nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl,
> > >  }
> > >  
> > >  __be32 nfsd4_acl_to_attr(enum nfs_ftype4 type, struct nfs4_acl *acl,
> > > -			 struct nfsd_attrs *attr)
> > > +			 struct nfsd_attrs *attr, int *dir_sticky)
> > >  {
> > >  	int host_error;
> > >  	unsigned int flags = 0;
> > >  
> > > -	if (!acl)
> > > +	if (!acl) {
> > > +		if (type == NF4DIR)
> > > +			*dir_sticky = -1;
> > >  		return nfs_ok;
> > > +	}
> > >  
> > >  	if (type == NF4DIR)
> > >  		flags = NFS4_ACL_DIR;
> > >  
> > >  	host_error = nfs4_acl_nfsv4_to_posix(acl, &attr->na_pacl,
> > > -					     &attr->na_dpacl, flags);
> > > +					     &attr->na_dpacl, dir_sticky,
> > > +					     flags);
> > >  	if (host_error == -EINVAL)
> > >  		return nfserr_attrnotsupp;
> > >  	else
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index 0f67f4a7b8b2..56aeb745d108 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -259,7 +259,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > >  		return nfserrno(host_err);
> > >  
> > >  	if (is_create_with_attrs(open))
> > > -		nfsd4_acl_to_attr(NF4REG, open->op_acl, &attrs);
> > > +		nfsd4_acl_to_attr(NF4REG, open->op_acl, &attrs, NULL);
> > >  
> > >  	inode_lock_nested(inode, I_MUTEX_PARENT);
> > >  
> > > @@ -791,6 +791,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >  	};
> > >  	struct svc_fh resfh;
> > >  	__be32 status;
> > > +	int dir_sticky;
> > >  	dev_t rdev;
> > >  
> > >  	fh_init(&resfh, NFS4_FHSIZE);
> > > @@ -804,7 +805,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >  	if (status)
> > >  		return status;
> > >  
> > > -	status = nfsd4_acl_to_attr(create->cr_type, create->cr_acl, &attrs);
> > > +	status = nfsd4_acl_to_attr(create->cr_type, create->cr_acl, &attrs, &dir_sticky);
> > >  	current->fs->umask = create->cr_umask;
> > >  	switch (create->cr_type) {
> > >  	case NF4LNK:
> > > @@ -848,6 +849,11 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >  		break;
> > >  
> > >  	case NF4DIR:
> > > +		if (dir_sticky == 1) {
> > > +			/* Set directory sticky bit deduced from the ACL attr. */
> > > +			create->cr_iattr.ia_valid |= ATTR_MODE;
> > > +			create->cr_iattr.ia_mode |= S_ISVTX;
> > > +		}
> > >  		create->cr_iattr.ia_valid &= ~ATTR_SIZE;
> > >  		status = nfsd_create(rqstp, &cstate->current_fh,
> > >  				     create->cr_name, create->cr_namelen,
> > > @@ -1144,6 +1150,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >  	struct inode *inode;
> > >  	__be32 status = nfs_ok;
> > >  	bool save_no_wcc;
> > > +	int dir_sticky;
> > >  	int err;
> > >  
> > >  	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> > > @@ -1165,10 +1172,26 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >  
> > >  	inode = cstate->current_fh.fh_dentry->d_inode;
> > >  	status = nfsd4_acl_to_attr(S_ISDIR(inode->i_mode) ? NF4DIR : NF4REG,
> > > -				   setattr->sa_acl, &attrs);
> > > -
> > > +				   setattr->sa_acl, &attrs, &dir_sticky);
> > >  	if (status)
> > >  		goto out;
> > > +
> > > +	if (S_ISDIR(inode->i_mode) && dir_sticky != -1 && !!(inode->i_mode & S_ISVTX) != dir_sticky) {
> > > +		/*
> > > +		 * Set directory sticky bit deduced from the ACL attr.
> > > +		 * Do not clear sticky bit if it was explicitly set in MODE attr
> > > +		 * but was not deduced from ACL attr because clients can send
> > > +		 * both MODE and ACL attrs where sticky bit is only in MODE attr.
> > > +		 */
> > > +		if (!(attrs.na_iattr->ia_valid & ATTR_MODE))
> > > +			attrs.na_iattr->ia_mode = inode->i_mode;
> > > +		if (dir_sticky)
> > > +			attrs.na_iattr->ia_mode |= S_ISVTX;
> > > +		else if (!(attrs.na_iattr->ia_valid & ATTR_MODE))
> > > +			attrs.na_iattr->ia_mode &= ~S_ISVTX;
> > > +		attrs.na_iattr->ia_valid |= ATTR_MODE;
> > > +	}
> > > +
> > >  	save_no_wcc = cstate->current_fh.fh_no_wcc;
> > >  	cstate->current_fh.fh_no_wcc = true;
> > >  	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs, NULL);
> > > -- 
> > > 2.20.1
> > > 
> > 
> > Hi Pali -
> > 
> > Apologies for the delayed response.
> > 
> > Being somewhat un-expert in things ACL, I'm not sure if this is the
> > correct approach, or if it's right for the POSIX ACL-only
> > implementation we have in Linux. I'm going to research this a bit
> > and get back to you.
> > 
> > -- 
> > Chuck Lever
> 
> Hello Chuck, thank you reply.
> 
> Just to note that this does not affect POSIX ACL-only storage on Linux
> server. Everything is same as before (it is POSIX ACL-only and
> everything is evaluated via POSIX ACLs).
> 
> This change is just what Linux NFS4 server provides to NFS4 clients,
> i.e. not for POSIX clients. As Linux NFS4 server already maps POSIX-ACL
> into NFS4-ACL format, I think that it makes sense to also map
> POSIX-sticky bit into NFS4-ACL format. It allows better interop for NFS4
> clients which use NFS4 ACLs.
> 
> What is this change trying to achieve is to allow Linux server to serve
> POSIX things (like sticky bit) for NFS4-ACL clients which are not POSIX
> aware, and serve this sticky bit in NFS4-ACL language. And same for
> opposite direction, to translate NFS4 ACL rules which describe sticky
> bit into the POSIX sticky bit in mode.

The fundamental claim from your patch description is that:

> > > the NFS4 ACL client is not
> > > aware of the fact that it cannot delete some files if the
> > > sticky bit is set on the server on parent directory.

POSIX-based clients are in fact aware of this additional constraint
because they can see the set of mode bits returned by GETATTR.

So can non-POSIX clients for that matter; although they might not
natively understand what that bit means, their NFS client can impart
that meaning.

I can find no spec mandate or guidance that requires this mapping,
nor can I find any other NFS server implementations that add it.
If this is indeed a valuable innovation, a standard that recommends
or requires implementation of this feature would be the place to
begin.

What RFC 8881 does say is on point:

> 6.3.1.1. Server Considerations

> The server uses the algorithm described in Section 6.2.1 to
> determine whether an ACL allows access to an object. However, the
> ACL might not be the sole determiner of access.

A list of examples follows. The spirit of this text seems to be that
a file object's ACL need not reflect every possible security policy
that a server might use to determine whether an operation may
proceed.

-- 
Chuck Lever

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ