linux-kernel - [PATCH 5/5] overlayfs: Use vfs_getxattr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <1467733854-6314-6-git-send-email-vgoyal@redhat.com>
Date:	Tue,  5 Jul 2016 11:50:54 -0400
From:	Vivek Goyal <vgoyal@...hat.com>
To:	miklos@...redi.hu, sds@...ho.nsa.gov, linux-kernel@...r.kernel.org,
	linux-unionfs@...r.kernel.org,
	linux-security-module@...r.kernel.org
Cc:	dwalsh@...hat.com, dhowells@...hat.com, pmoore@...hat.com,
	viro@...IV.linux.org.uk, vgoyal@...hat.com,
	linux-fsdevel@...r.kernel.org
Subject: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
if mounter does not have DAC/MAC permission to access getxattr.

Specifically this becomes a problem when selinux is trying to initialize
overlay inode and does ->getxattr(overlay_inode). A task might trigger
initialization of overlay inode and we will access real inode xattr in the
context of mounter and if mounter does not have permissions, then inode
selinux context initialization fails and inode is labeled as unlabeled_t.

One way to deal with it is to let SELinux do getxattr checks both on
overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
to make sure when selinux is trying to initialize label on inode, it does
not go through checks on lower levels and initialization is successful.
And after inode initialization, SELinux will make sure task has getatttr
permission.

One issue with this approach is that it does not work for directories as
d_real() returns the overlay dentry for directories and not the underlying
directory dentry.

Another way to deal with it to introduce another function pointer in
inode_operations, say getxattr_noperm(), which is responsible to get
xattr without any checks. SELinux initialization code will call this
first if it is available on inode. So user space code path will call
->getxattr() and that will go through checks and SELinux internal
initialization will call ->getxattr_noperm() and that will not
go through checks.

For now, I am just converting ovl_getxattr() to get xattr without
any checks on underlying inode. That means it is possible for
a task to get xattr of a file/dir on lower/upper through overlay mount
while it is not possible outside overlay mount.

If this is a major concern, I can look into implementing getxattr_noperm().

Signed-off-by: Vivek Goyal <vgoyal@...hat.com>
---
 fs/overlayfs/inode.c  |  7 +------
 fs/xattr.c            | 28 +++++++++++++++++++---------
 include/linux/xattr.h |  1 +
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 36dfd86..a5d3320 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -233,16 +233,11 @@ ssize_t ovl_getxattr(struct dentry *dentry, struct inode *inode,
 		     const char *name, void *value, size_t size)
 {
 	struct dentry *realdentry = ovl_dentry_real(dentry);
-	ssize_t sz;
-	const struct cred *old_cred;
 
 	if (ovl_is_private_xattr(name))
 		return -ENODATA;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
-	sz = vfs_getxattr(realdentry, name, value, size);
-	revert_creds(old_cred);
-	return size;
+	return vfs_getxattr_noperm(realdentry, name, value, size);
 }
 
 ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
diff --git a/fs/xattr.c b/fs/xattr.c
index 4beafc4..973e18c 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -209,19 +209,11 @@ vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value,
 }
 
 ssize_t
-vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
+vfs_getxattr_noperm(struct dentry *dentry, const char *name, void *value, size_t size)
 {
 	struct inode *inode = dentry->d_inode;
 	int error;
 
-	error = xattr_permission(inode, name, MAY_READ);
-	if (error)
-		return error;
-
-	error = security_inode_getxattr(dentry, name);
-	if (error)
-		return error;
-
 	if (!strncmp(name, XATTR_SECURITY_PREFIX,
 				XATTR_SECURITY_PREFIX_LEN)) {
 		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
@@ -242,6 +234,24 @@ nolsm:
 
 	return error;
 }
+EXPORT_SYMBOL_GPL(vfs_getxattr_noperm);
+
+ssize_t
+vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
+{
+	struct inode *inode = dentry->d_inode;
+	int error;
+
+	error = xattr_permission(inode, name, MAY_READ);
+	if (error)
+		return error;
+
+	error = security_inode_getxattr(dentry, name);
+	if (error)
+		return error;
+
+	return vfs_getxattr_noperm(dentry, name, value, size);
+}
 EXPORT_SYMBOL_GPL(vfs_getxattr);
 
 ssize_t
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index 94079ba..832a6b6 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -47,6 +47,7 @@ struct xattr {
 
 ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
 ssize_t vfs_getxattr(struct dentry *, const char *, void *, size_t);
+ssize_t vfs_getxattr_noperm(struct dentry *, const char *, void *, size_t);
 ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
 int __vfs_setxattr_noperm(struct dentry *, const char *, const void *, size_t, int);
 int vfs_setxattr(struct dentry *, const char *, const void *, size_t, int);
-- 
2.7.4