From 93e459f4d0604cccb31e5cc0a1677499d48fff0b Mon Sep 17 00:00:00 2001 From: Dmitry Eremin Date: Mon, 25 Jul 2016 17:04:12 +0300 Subject: [PATCH] LU-1482 mdd: Setting xattr are properly checked with and without ACLs Setting extended attributes permissions are properly checked with and without ACLs. In user.* namespace, only regular files and directories can have extended attributes. For sticky directories, only the owner and privileged user can write attributes. Intel-bug-id: LDEV-40 Intel-change: http://review.whamcloud.com/15848 Change-Id: Ibd79dcc15e61839d878f4847f7836f29d823be61 Signed-off-by: Dmitry Eremin Reviewed-on: http://review.whamcloud.com/21496 Reviewed-by: John L. Hammond Tested-by: Jenkins Tested-by: Maloo Reviewed-by: Alex Zhuravlev Reviewed-by: Oleg Drokin --- lustre/llite/xattr.c | 23 +++-- lustre/mdd/mdd_object.c | 22 +++-- lustre/tests/Makefile.am | 1 + lustre/tests/acl/permissions_xattr.test | 143 ++++++++++++++++++++++++++++++++ lustre/tests/sanity.sh | 9 ++ 5 files changed, 185 insertions(+), 13 deletions(-) create mode 100644 lustre/tests/acl/permissions_xattr.test diff --git a/lustre/llite/xattr.c b/lustre/llite/xattr.c index c4af9c1..2d992ae 100644 --- a/lustre/llite/xattr.c +++ b/lustre/llite/xattr.c @@ -151,15 +151,22 @@ int ll_setxattr_common(struct inode *inode, const char *name, strcmp(name, "lustre.lov") == 0)) RETURN(0); - /* b15587: ignore security.capability xattr for now */ - if ((xattr_type == XATTR_SECURITY_T && - strcmp(name, "security.capability") == 0)) - RETURN(0); + /* b15587: ignore security.capability xattr for now */ + if ((xattr_type == XATTR_SECURITY_T && + strcmp(name, "security.capability") == 0)) + RETURN(0); - /* LU-549: Disable security.selinux when selinux is disabled */ - if (xattr_type == XATTR_SECURITY_T && !selinux_is_enabled() && - strcmp(name, "security.selinux") == 0) - RETURN(-EOPNOTSUPP); + /* LU-549: Disable security.selinux when selinux is disabled */ + if (xattr_type == XATTR_SECURITY_T && !selinux_is_enabled() && + strcmp(name, "security.selinux") == 0) + RETURN(-EOPNOTSUPP); + + /* In user.* namespace, only regular files and directories can have + * extended attributes. */ + if (xattr_type == XATTR_USER_T) { + if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode)) + RETURN(-EPERM); + } rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode), valid, name, pv, size, 0, flags, ll_i2suppgid(inode), &req); diff --git a/lustre/mdd/mdd_object.c b/lustre/mdd/mdd_object.c index 8743d96..7774803 100644 --- a/lustre/mdd/mdd_object.c +++ b/lustre/mdd/mdd_object.c @@ -907,7 +907,8 @@ stop: static int mdd_xattr_sanity_check(const struct lu_env *env, struct mdd_object *obj, - const struct lu_attr *attr) + const struct lu_attr *attr, + const char *name) { struct lu_ucred *uc = lu_ucred_assert(env); ENTRY; @@ -915,8 +916,19 @@ static int mdd_xattr_sanity_check(const struct lu_env *env, if (attr->la_flags & (LUSTRE_IMMUTABLE_FL | LUSTRE_APPEND_FL)) RETURN(-EPERM); - if ((uc->uc_fsuid != attr->la_uid) && !md_capable(uc, CFS_CAP_FOWNER)) - RETURN(-EPERM); + if (strncmp(XATTR_USER_PREFIX, name, + sizeof(XATTR_USER_PREFIX) - 1) == 0) { + /* For sticky directories, only the owner and privileged user + * can write attributes. */ + if (S_ISDIR(attr->la_mode) && (attr->la_mode & S_ISVTX) && + (uc->uc_fsuid != attr->la_uid) && + !md_capable(uc, CFS_CAP_FOWNER)) + RETURN(-EPERM); + } else { + if ((uc->uc_fsuid != attr->la_uid) && + !md_capable(uc, CFS_CAP_FOWNER)) + RETURN(-EPERM); + } RETURN(0); } @@ -1038,7 +1050,7 @@ static int mdd_xattr_set(const struct lu_env *env, struct md_object *obj, if (rc) RETURN(rc); - rc = mdd_xattr_sanity_check(env, mdd_obj, attr); + rc = mdd_xattr_sanity_check(env, mdd_obj, attr, name); if (rc) RETURN(rc); @@ -1151,7 +1163,7 @@ static int mdd_xattr_del(const struct lu_env *env, struct md_object *obj, if (rc) RETURN(rc); - rc = mdd_xattr_sanity_check(env, mdd_obj, attr); + rc = mdd_xattr_sanity_check(env, mdd_obj, attr, name); if (rc) RETURN(rc); diff --git a/lustre/tests/Makefile.am b/lustre/tests/Makefile.am index 469c14a..0e3d416 100644 --- a/lustre/tests/Makefile.am +++ b/lustre/tests/Makefile.am @@ -50,6 +50,7 @@ nobase_noinst_SCRIPTS += racer/file_mknod.sh racer/file_setxattr.sh racer/file_t nobase_noinst_SCRIPTS += posix/posix.cfg nobase_noinst_DATA = acl/cp.test acl/getfacl-noacl.test acl/inheritance.test nobase_noinst_DATA += acl/misc.test acl/permissions.test acl/setfacl.test +nobase_noinst_DATA += acl/permissions_xattr.test nobase_noinst_DATA += acl/974.test acl/974_remote.test nobase_noinst_DATA += acl/2561.test acl/2561_zfs.test acl/4924.test nobase_noinst_DATA += clientapi/simple_test.c diff --git a/lustre/tests/acl/permissions_xattr.test b/lustre/tests/acl/permissions_xattr.test new file mode 100644 index 0000000..d4ca17b --- /dev/null +++ b/lustre/tests/acl/permissions_xattr.test @@ -0,0 +1,143 @@ +This script tests if extended attributes permissions are properly checked +with and without ACLs. The script must be run as root to allow switching +users. The following users are required. + + bin + nobody + + +Cry immediately if we are not running as root. + + $ id -u + > 0 + + +First, set up a temporary directory and create a regular file with +defined permissions. + + $ mkdir d + $ cd d + $ umask 027 + $ touch f + $ chown nobody:nobody f + $ ls -l f | awk -- '{ print $1, $3, $4 }' + > -rw-r----- nobody nobody + $ su nobody + $ echo nobody > f + + +Verify that the user bin don't have enough permission to set +extended attributes in user.* namespace. + + $ su bin + $ setfattr -n user.test.xattr -v 123456 f + > setfattr: f: Permission denied + + +Now, add an ACL entry for user bin that grants him rw- access. File +owners and users capable of CAP_FOWNER are allowed to change ACLs. + + $ su nobody + $ setfacl -m g:bin:rw f + $ getfacl --omit-header f + > user::rw- + > group::r-- + > group:bin:rw- + > mask::rw- + > other::--- + > + + +Verify that the additional ACL entry grants user bin permission +to set extended attributes in user.* namespace for files. + + $ su bin + $ setfattr -n user.test.xattr -v 123456 f + $ getfattr -d f + > # file: f + > user.test.xattr="123456" + > + + +Test if symlinks are properly followed. + + $ su + $ ln -s f l + $ ls -l l | awk -- '{ print $1, $3, $4 }' + > lrwxrwxrwx root root + $ su bin + $ getfattr -d l + > # file: l + > user.test.xattr="123456" + > + + +Test the sticky directories. Only the owner and privileged user can +write attributes. + + $ su + $ mkdir t + $ chown nobody:nobody t + $ chmod 1750 t + $ ls -dl t | awk -- '{ print $1, $3, $4 }' + > drwxr-x--T nobody nobody + $ su nobody + $ setfacl -m g:bin:rwx t + $ getfacl --omit-header t + > user::rwx + > group::r-x + > group:bin:rwx + > mask::rwx + > other::--- + > + $ su bin + $ setfattr -n user.test.xattr -v 654321 t + > setfattr: t: Operation not permitted + + +Verify that the additional ACL entry grants user bin permission +to set extended attributes in user.* namespace for directories. + + $ su + $ mkdir d + $ chown nobody:nobody d + $ chmod 750 d + $ ls -dl d | awk -- '{ print $1, $3, $4 }' + > drwxr-x--- nobody nobody + $ su nobody + $ setfacl -m g:bin:rwx d + $ getfacl --omit-header d + > user::rwx + > group::r-x + > group:bin:rwx + > mask::rwx + > other::--- + > + $ su bin + $ setfattr -n user.test.xattr -v 654321 d + $ getfattr -d d + > # file: d + > user.test.xattr="654321" + > + + +Test that in user.* namespace, only regular files and directories can have +extended attributes. + + $ su + $ mknod -m 0660 hdt b 91 64 # /dev/hdt + $ mknod -m 0660 null c 1 3 # /dev/null + $ mkfifo -m 0660 fifo + $ setfattr -n user.test.xattr -v 123456 hdt + > setfattr: hdt: Operation not permitted + $ setfattr -n user.test.xattr -v 123456 null + > setfattr: null: Operation not permitted + $ setfattr -n user.test.xattr -v 123456 fifo + > setfattr: fifo: Operation not permitted + + +Clean up. + + $ su + $ cd .. + $ rm -rf d diff --git a/lustre/tests/sanity.sh b/lustre/tests/sanity.sh index 8516e09..37ce91c 100755 --- a/lustre/tests/sanity.sh +++ b/lustre/tests/sanity.sh @@ -7303,6 +7303,15 @@ test_103a() { run_acl_subtest misc || error "misc test failed" echo "performing permissions..." run_acl_subtest permissions || error "permissions failed" + # LU-1482 mdd: Setting xattr are properly checked with and without ACLs + if [ $(lustre_version_code $SINGLEMDS) -gt $(version_code 2.8.55) -o \ + \( $(lustre_version_code $SINGLEMDS) -lt $(version_code 2.6) -a \ + $(lustre_version_code $SINGLEMDS) -ge $(version_code 2.5.29) \) ] + then + echo "performing permissions xattr..." + run_acl_subtest permissions_xattr || + error "permissions_xattr failed" + fi echo "performing setfacl..." run_acl_subtest setfacl || error "setfacl test failed" -- 1.8.3.1