From efb66de719329ce4d96b40f00ad592cca1e432fd Mon Sep 17 00:00:00 2001 From: Sebastien Buisson Date: Mon, 28 Jun 2021 20:32:16 +0200 Subject: [PATCH] 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 Change-Id: I919f5cbafc53f5745fbfb5b9d2d7316e892d8c9f Reviewed-on: https://review.whamcloud.com/44101 Reviewed-by: Andreas Dilger Reviewed-by: Patrick Farrell Tested-by: jenkins Tested-by: Maloo Reviewed-by: Oleg Drokin --- lustre/llite/crypto.c | 16 ++++++++++++++++ lustre/llite/llite_internal.h | 5 +++++ lustre/llite/xattr.c | 30 +++++++++++++++++++++++++++++- lustre/tests/sanity-sec.sh | 20 ++++++++++++++++++-- 4 files changed, 68 insertions(+), 3 deletions(-) diff --git a/lustre/llite/crypto.c b/lustre/llite/crypto.c index 89917c7..e8d87d6 100644 --- 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 --- 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 --- 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 --- 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" -- 1.8.3.1