[LU-4703] setxattr(2) will succeed by a non root user, against a file the user doesn't own.

• Status: Resolved
• Project: Lustre
• Affects Version/s: Lustre 2.5.0, Lustre 2.6.0, Lustre 2.4.2
• Fix Version/s: Lustre 2.6.0, Lustre 2.5.1, Lustre 2.4.3
• Type: Bug
• Priority: Blocker
• Reporter: Li Dongyang (Inactive)
• Assignee: Nathaniel Clark
• Resolution: Fixed
• Labels: patch

Environment

CentOS 6.4

• Severity: 3

Description

[root@localhost ~]# mount -t lustre 192.168.122.225@tcp:/testfs /mnt/
[root@localhost ~]# ll /mnt/
total 8
drwxr-xr-x 2 dyl900 users 4096 Mar 4 16:08 dyl900
drwxr-xr-x 2 mxa900 users 4096 Mar 4 16:08 mxa900
[root@localhost ~]# su - dyl900
[dyl900@localhost ~]$ cd /mnt/
[dyl900@localhost mnt]$ getfacl ./mxa900

1. file: mxa900/
2. owner: mxa900
3. group: users
   user::rwx
   group::r-x
   other::r-x

[dyl900@localhost mnt]$ setfacl -m u:dyl900:rwx ./mxa900
[dyl900@localhost mnt]$ getfacl ./mxa900

1. file: mxa900/
2. owner: mxa900
3. group: users
   user::rwx
   user:dyl900:rwx
   group::r-x
   mask::rwx
   other::r-x

On our production system, this allows a user access other users' files...

Issue Links

• is duplicated by: LU-4704 Permission checking is missing when s...
• is related to: LU-4704 Permission checking is missing when s...

Comments

[Li Dongyang (Inactive)]

for master: http://review.whamcloud.com/#/c/9469/

(This is the one for mdd_xattr_set().)

[John Hammond]

Looking at this, I think mdd_xattr_sanity_check() could be improved somewhat. Currently it looks like:

static int mdd_xattr_sanity_check(const struct lu_env *env,
                                  struct mdd_object *obj,
                                  const struct lu_attr *attr)
{
        struct lu_ucred *uc     = lu_ucred_assert(env);
        ENTRY;

        if (mdd_is_immutable(obj) || mdd_is_append(obj))
                RETURN(-EPERM);

        if ((uc->uc_fsuid != attr->la_uid) && !md_capable(uc, CFS_CAP_FOWNER))
                RETURN(-EPERM);

        RETURN(0);
}

1. This might benefit from the explicit xattr classification done in ll_setxattr_common(). That is we could check prefix and reject anything we don't recognize/want to support.
2. Even though the client masks some xattrs like security.capability it would be worthwhile to handle them explicitly here. Setting security.capability should require CAP_SETFCAP. Setting anything other security.* should require CAP_SYS_ADMIN.
3. There are some other rules like for sticky directories that should be considered. Probably mdd_xattr_set_sanity_check() should duplicate most of the logic in xattr_permission().
4. There is also some access policy in mdt_reint_setxattr() that should be looked at and maybe moved.

[Andreas Dilger]

Patch from LU-4704 http://review.whamcloud.com/9473

[Andreas Dilger]

I just verified that this bug does not exist in any release earlier than 2.4.0. It was added in commit 7b3bfb09, which moved the ACL handling out of the OSD and into the MDD so that ZFS does not have to handle ACL checking itself.

[Bob Glossman (Inactive)]

for b2_5: http://review.whamcloud.com/9513

[Nathaniel Clark]

Test for issue http://review.whamcloud.com/9508

[Bob Glossman (Inactive)]

for b2_4: http://review.whamcloud.com/9558

[Andreas Dilger]

Note that it should be possible to disable ACL support on ldiskfs MDSes either temporarily or permanently to provide a short-term workaround for this bug. There is no mechanism to disable ACL support on ZFS filesystems at this time.

The MDS can be mounted with the "-o noacl" option to disable ACL support temporarily, or by unmounting the MDS, running "tune2fs -o ^acl /dev/<mdsdev>" to turn off ACL support in the ldiskfs superblock, and then mount the MDS again.

Disabling the ACL can be verified on the client by checking "lctl get_param mdc.*.import | grep connect_flags" and checking whether the "acl" feature is listed (it will normally be second).

[Nathaniel Clark]

sanity/102p for b2_5 http://review.whamcloud.com/9604

[Peter Jones]

Landed for 2.4.3, 2.5.1 and 2.6