LU-14677 sec: do not expose security.c to listxattr/getxattr

security.c xattr, which contains encryption context, should not be
exposed by the xattr-related system calls such as listxattr() and
getxattr() because of its special semantics.
Update sanity-sec test_57 to test this.

Signed-off-by: Sebastien Buisson <sbuisson@ddn.com>
Change-Id: I919f5cbafc53f5745fbfb5b9d2d7316e892d8c9f
Reviewed-on: https://review.whamcloud.com/44101
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Patrick Farrell <pfarrell@whamcloud.com>
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>

diff --git a/lustre/llite/crypto.c b/lustre/llite/crypto.c
index 89917c7..e8d87d6 100644 (file)
--- a/lustre/llite/crypto.c
+++ b/lustre/llite/crypto.c
@@ -34,10 +34,26 @@
 static int ll_get_context(struct inode *inode, void *ctx, size_t len)
 {
        struct dentry *dentry = d_find_any_alias(inode);
+       struct lu_env *env;
+       __u16 refcheck;
        int rc;
 
+       env = cl_env_get(&refcheck);
+       if (IS_ERR(env))
+               return PTR_ERR(env);
+
+       /* Set lcc_getencctx=1 to allow this thread to read
+        * LL_XATTR_NAME_ENCRYPTION_CONTEXT xattr, as requested by llcrypt.
+        */
+       ll_cl_add(inode, env, NULL, LCC_RW);
+       ll_env_info(env)->lti_io_ctx.lcc_getencctx = 1;
+
        rc = ll_vfs_getxattr(dentry, inode, LL_XATTR_NAME_ENCRYPTION_CONTEXT,
                             ctx, len);
+
+       ll_cl_remove(inode, env);
+       cl_env_put(env, &refcheck);
+
        if (dentry)
                dput(dentry);
 

diff --git a/lustre/llite/llite_internal.h b/lustre/llite/llite_internal.h
index e2b9174..c5465b3 100644 (file)
--- a/lustre/llite/llite_internal.h
+++ b/lustre/llite/llite_internal.h
@@ -1315,6 +1315,11 @@ struct ll_cl_context {
        struct cl_io            *lcc_io;
        struct cl_page          *lcc_page;
        enum lcc_type            lcc_type;
+       /**
+        * Get encryption context operation in progress,
+        * allow getxattr of LL_XATTR_NAME_ENCRYPTION_CONTEXT xattr
+        */
+       unsigned                 lcc_getencctx:1;
 };
 
 struct ll_thread_info {

diff --git a/lustre/llite/xattr.c b/lustre/llite/xattr.c
index a8628f3..779e6e0 100644 (file)
--- a/lustre/llite/xattr.c
+++ b/lustre/llite/xattr.c
@@ -379,6 +379,19 @@ int ll_xattr_list(struct inode *inode, const char *name, int type, void *buffer,
        int rc;
        ENTRY;
 
+       /* Getting LL_XATTR_NAME_ENCRYPTION_CONTEXT xattr is only allowed
+        * when it comes from ll_get_context(), ie when llcrypt needs to
+        * know the encryption context.
+        * Otherwise, any direct reading of this xattr returns -EPERM.
+        */
+       if (type == XATTR_SECURITY_T &&
+           !strcmp(name, LL_XATTR_NAME_ENCRYPTION_CONTEXT)) {
+               struct ll_cl_context *lcc = ll_cl_find(inode);
+
+               if (!lcc || !lcc->lcc_getencctx)
+                       GOTO(out_xattr, rc = -EPERM);
+       }
+
        if (sbi->ll_xattr_cache_enabled && type != XATTR_ACL_ACCESS_T &&
            (type != XATTR_SECURITY_T || strcmp(name, "security.selinux"))) {
                rc = ll_xattr_cache_get(inode, name, buffer, size, valid);
@@ -639,9 +652,24 @@ ssize_t ll_listxattr(struct dentry *dentry, char *buffer, size_t size)
        rem = rc;
 
        while (rem > 0) {
+               bool hide_xattr = false;
+
+               /* Listing xattrs should not expose
+                * LL_XATTR_NAME_ENCRYPTION_CONTEXT xattr, unless it comes
+                * from llcrypt.
+                */
+               if (get_xattr_type(xattr_name)->flags == XATTR_SECURITY_T &&
+                   !strcmp(xattr_name, LL_XATTR_NAME_ENCRYPTION_CONTEXT)) {
+                       struct ll_cl_context *lcc = ll_cl_find(inode);
+
+                       if (!lcc || !lcc->lcc_getencctx)
+                               hide_xattr = true;
+               }
+
                len = strnlen(xattr_name, rem - 1) + 1;
                rem -= len;
-               if (!xattr_type_filter(sbi, get_xattr_type(xattr_name))) {
+               if (!xattr_type_filter(sbi, hide_xattr ? NULL :
+                                      get_xattr_type(xattr_name))) {
                        /* Skip OK xattr type, leave it in buffer. */
                        xattr_name += len;
                        continue;

diff --git a/lustre/tests/sanity-sec.sh b/lustre/tests/sanity-sec.sh
index 751f4ec..fb718f0 100755 (executable)
--- a/lustre/tests/sanity-sec.sh
+++ b/lustre/tests/sanity-sec.sh
@@ -4287,11 +4287,27 @@ test_57() {
        setup_for_enc_tests
 
        mkdir $testdir
-       setfattr -n security.c -v myval $testdir &&
+       if [ $(getfattr -n security.c $testdir 2>&1 |
+              grep -ci "Operation not permitted") -eq 0 ]; then
+               error "getting xattr on $testdir should have failed"
+       fi
+       getfattr -d -m - $testdir 2>&1 | grep security\.c &&
+               error "listing xattrs on $testdir should not expose security.c"
+       if [ $(setfattr -n security.c -v myval $testdir 2>&1 |
+              grep -ci "Operation not permitted") -eq 0 ]; then
                error "setting xattr on $testdir should have failed (2)"
+       fi
        touch $testfile
-       setfattr -n security.c -v myval $testfile &&
+       if [ $(getfattr -n security.c $testfile 2>&1 |
+              grep -ci "Operation not permitted") -eq 0 ]; then
+               error "getting xattr on $testfile should have failed"
+       fi
+       getfattr -d -m - $testfile 2>&1 | grep security\.c &&
+               error "listing xattrs on $testfile should not expose security.c"
+       if [ $(setfattr -n security.c -v myval $testfile 2>&1 |
+              grep -ci "Operation not permitted") -eq 0 ]; then
                error "setting xattr on $testfile should have failed (2)"
+       fi
        return 0
 }
 run_test 57 "security.c xattr protection"