Re: No extended attributes for sticky directories? (and samba)

--Boundary-00=_2+3SFa9EO3iyJfX
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

Hello,

On Friday 03 November 2006 07:27, Gerard Neil wrote:
> Andreas,
>
> On 03/11/06, Andreas Gruenbacher <[email protected]> wrote:
> > Indeed, yes. The xfs code has it right, and the patch is good. I'll fix
> > the two typos in the comment above in the same go, and push the resulting
> > patch (attached) upstream. Thanks for your help!
>
> That's quite exciting for me; I've taken my first open source step,
> however small. Thanks to both you and Dave.
>
> But... I'm not sure that the patch and xfs can/should both be right at
> the same time.
>
> Without the patch, the linux behaviour is the same for all
> filesystems. There's no access at all to extended attributes on sticky
> directories, for any user including root. Because the vfs behaviour is
> more restictive than that of xfs this applies to xfs too.
>
> The patch does two things: firstly it aligns vfs behaviour with the
> documentation and xfs, by allowing access for the owner and priveleged
> users. Then, it goes further, and allows read access for all users. My
> grounds for this latter change were that it felt intuitively right to
> me, and that this behaviour appeared to have been in the kernel in
> earlier versions (although I was going only from snippets, there
> appeared to have been a return in the function for the read-only case
> before stickyness was tested, the sort of thing that's easy to
> overlook when making changes).
>
> Anyway, the latter change makes the vfs behaviour _less_ restrictive
> than xfs, so the net effect is:
>
> vfs (therefore jfs/ext2/ext3 and most other filesystems):
>   Read: allowed for all users
>   Write: owner and CAP_FOWNER only
>
> xfs:
>   Read: owner and CAP_FOWNER only
>   Write: owner and CAP_FOWNER only

Almost, except that the vfs also checks if the user is granted the respective 
access by the file permissions (mode or ACLs).

The original intention of the user.* namespace was to use the same mechanism 
that controls access to the file data for extended attributes. The exceptions 
for special files and sticky directories are necessary additional 
restrictions for making the mechanism secure, and preventing users from 
storing xattr information where nobody would expect it.

> My thinking was that either the patch should be aligned with xfs (by
> leaving out the "allow read access part") or vice versa (by changing
> xfs to allow read access). My intent was to query which path to take.
> I apologise for not making this clear, I didn't quite believe I'd
> found a problem or it's solution, and the point got a bit lost in my
> uncertainty. It simply didn't occur to me that my patch would be
> accepted as is. I'll be more careful in future: beware when posting a
> patch... someone might just apply it!
>
> I don't believe the unmodified patch should be used because it would
> introduce a slight difference in behaviour between xfs and all other
> filesystems. That's fine if there's a good reason for such a
> difference, but otherwise it should be avoided on general principles:
> it can lead to unexpected, hard to track consequences. Case in point:
> samba. The consequence would be that hidden files suddenly appear
> unhidden if a sticky directory appears anywhere on a share with an
> underlying xfs filesystem. Sure, the samba code should be improved,
> but we're already in the margins with the sticky directory thing to
> begin with; it's worse if it's just for xfs and for no good reason.
>
> I guess my "preference" was to modify xfs to allow read access... but
> I didn't actually go ahead include the change in the patch because of
> the following: according to my reading, the code has a notion of
> attribute "capability" which is relative to process credentials; if an
> i-node is attribute-capable, then it is allowed to take attributes,
> otherwise not. My gut feeling is that the word "capable" precludes
> making it relative to the type of access, otherwise a word like
> "permission" would have been used; but it's impossible to tell for
> sure from the code, that's just the way I relate to English. It's easy
> enough to make the change (the type of access is passed to
> attr_generic_get() via xflags, but that would involve either changing
> the meaning of "capable" according to my reading or a slight refactor.
> In any case, I felt the whole thing needed discussion first.

I don't know what you are talking about in the above paragraph.

Tim, I guess the attached patch would make sense for xfs -- what do you think?

> Anyway, the last thing I want is to introduce problems because I
> didn't make myself clear enough, so I'd appreciate it Andreas if you
> could reassure me / modify the patch / consider the change to xfs.
> Once again, thanks for the replies and encouragement.

Thanks as well,
Andreas

--Boundary-00=_2+3SFa9EO3iyJfX
Content-Type: text/x-diff;
  charset="iso-8859-1";
  name="remove-duplicate-xfs-user-xattr-check.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename="remove-duplicate-xfs-user-xattr-check.diff"

Index: linux-2.6.19-rc4/fs/xfs/xfs_attr.c
===================================================================
--- linux-2.6.19-rc4.orig/fs/xfs/xfs_attr.c
+++ linux-2.6.19-rc4/fs/xfs/xfs_attr.c
@@ -2602,29 +2602,6 @@ attr_lookup_namespace(
 	return NULL;
 }
 
-/*
- * Some checks to prevent people abusing EAs to get over quota:
- * - Don't allow modifying user EAs on devices/symlinks;
- * - Don't allow modifying user EAs if sticky bit set;
- */
-STATIC int
-attr_user_capable(
-	bhv_vnode_t	*vp,
-	cred_t		*cred)
-{
-	struct inode	*inode = vn_to_inode(vp);
-
-	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
-		return -EPERM;
-	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode) &&
-	    !capable(CAP_SYS_ADMIN))
-		return -EPERM;
-	if (S_ISDIR(inode->i_mode) && (inode->i_mode & S_ISVTX) &&
-	    (current_fsuid(cred) != inode->i_uid) && !capable(CAP_FOWNER))
-		return -EPERM;
-	return 0;
-}
-
 STATIC int
 attr_trusted_capable(
 	bhv_vnode_t	*vp,
@@ -2726,7 +2703,7 @@ struct attrnames attr_user = {
 	.attr_get	= attr_generic_get,
 	.attr_set	= attr_generic_set,
 	.attr_remove	= attr_generic_remove,
-	.attr_capable	= attr_user_capable,
+	.attr_capable	= (attrcapable_t)fs_noerr,
 };
 
 struct attrnames *attr_namespaces[] =

--Boundary-00=_2+3SFa9EO3iyJfX
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
acl-devel mailing list
[email protected]
http://acl.bestbits.at/mailman/listinfo/acl-devel

--Boundary-00=_2+3SFa9EO3iyJfX--