On Thu, Aug 16, 2018 at 10:39:35AM +1000, NeilBrown wrote:
> On Tue, Aug 14 2018, Bruce Fields wrote:
> > Honestly I'm not completely sure I understand the proposal.
>
> Ok, here is a concrete RFC proposal which should make it easier to
> understand.
> I've tested that this fixes the specific problem in that a user with a
> uid that doesn't match the file, but which the server will give
> ownership rights to, can now setacl a file.
Thanks, this makes sense to me.
I might try to split this change into a couple steps, but I'm not sure
exactly how.
Minor nits:
> From 34f8b23b224e575d5f1fa30834b247e82a854546 Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@xxxxxxxx>
> Date: Thu, 16 Aug 2018 10:37:21 +1000
> Subject: [PATCH] VFS: introduce MAY_ACT_AS_OWNER
>
> A few places in VFS, particularly set_posix_acl(), use
> inode_owner_or_capable() to check if the caller has "owner"
> access to the inode.
> This assumes that it is valid to test inode->i_uid, which is not
> always the case. Particularly in the case of NFS it is not valid to
> us i_uid (or i_mode) for permission tests - the server needs to make
> the decision.
>
> As a result if the server is remaping uids
remapping
> (e.g. all-squash,anon_uid=1000),
> then all users should have ownership access, but most users will not
> be able to set acls.
>
> This patch moves the ownership test to inode_permission and
> i_op->permission.
> A new flag for this functions, MAY_ACT_AS_OWNER is introduced.
these functions?
> generic_permission() now handles this correctly and many
> i_op->permission functions call this function() and don't need any
> changes. A few are changed to handle MAY_ACT_AS_OWNER exactly
> as generic_permission() does, using inode_owner_or_capable().
> For these filesystems, no behavioural change should be noticed.
>
> For NFS, nfs_permission is changed to always return 0 (success) if
> MAY_ACT_AS_OWNER. For NFS, and operations which use this flag should
any operations
> @@ -2038,12 +2038,13 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
> * We must trust the client to do permission checking - using "ACCESS"
> * with NFSv3.
> */
> - if ((acc & NFSD_MAY_OWNER_OVERRIDE) &&
> - uid_eq(inode->i_uid, current_fsuid()))
> - return 0;
>
> /* This assumes NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
Can we do the same for NFSD_MAY_OWNER_OVERRIDE and drop the extra "if"
statement?
> - err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
> + if (acc & NFSD_MAY_OWNER_OVERRIDE)
> + err = inode_permission(inode, ((acc & (MAY_READ|MAY_WRITE|MAY_EXEC))
> + | MAY_ACT_AS_OWNER));
> + else
> + err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
>
> /* Allow read access to binaries even when mode 111 */
> if (err == -EACCES && S_ISREG(inode->i_mode) &&
--b.
