[PATCH] ecryptfs: open lower files on kthread under SELinux

[PATCH] ecryptfs: open lower files on kthread under SELinux

[Ricky Zhou]

When SELinux is enabled, files are associated with the current task's
SID when they are opened. Before this change, the lower file would
sometimes be associated with the SID of the first process to open the
corresponding eCryptfs file. If a second process attempts to access this
file through the same lower file, the access is denied unless the
SELinux policy allows the second process to use files opened by the
first process (the FD__USE permission).

Since it may not be desirable to allow all processes this permission,
this change always opens the lower file from a thread with the kernel
SID. This way, the policy only needs to grant all processes FD__USE on
the kernel context (which in practice, is exactly the use of eCryptfs
lower files).

Signed-off-by: Ricky Zhou <rickyz@chromium.org>
---
 fs/ecryptfs/kthread.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
index 866bb18..0ce7828 100644
--- a/fs/ecryptfs/kthread.c
+++ b/fs/ecryptfs/kthread.c
@@ -25,6 +25,7 @@
 #include <linux/slab.h>
 #include <linux/wait.h>
 #include <linux/mount.h>
+#include <linux/selinux.h>
 #include "ecryptfs_kernel.h"
 
 struct ecryptfs_open_req {
@@ -141,17 +142,30 @@ int ecryptfs_privileged_open(struct file **lower_file,
 	req.path.dentry = lower_dentry;
 	req.path.mnt = lower_mnt;
 
-	/* Corresponding dput() and mntput() are done when the
-	 * lower file is fput() when all eCryptfs files for the inode are
-	 * released. */
-	flags |= IS_RDONLY(d_inode(lower_dentry)) ? O_RDONLY : O_RDWR;
-	(*lower_file) = dentry_open(&req.path, flags, cred);
-	if (!IS_ERR(*lower_file))
-		goto out;
-	if ((flags & O_ACCMODE) == O_RDONLY) {
-		rc = PTR_ERR((*lower_file));
-		goto out;
+	/*
+	 * When SELinux is enabled, force the lower file to be opened by the
+	 * kernel thread. Otherwise, the lower file will be associated with the
+	 * SELinux context of the first process that opens it, which may prevent
+	 * a different process from using it, unless that process has permission
+	 * to use fds from the first process.
+	 *
+	 * If SELinux is enabled, then any process that needs read access to an
+	 * ecryptfs filesytem must allow fd:use on the kernel context.
+	 */
+	if (!selinux_is_enabled()) {
+		/* Corresponding dput() and mntput() are done when the lower
+		 * file is fput() when all eCryptfs files for the inode are
+		 * released. */
+		flags |= IS_RDONLY(d_inode(lower_dentry)) ? O_RDONLY : O_RDWR;
+		(*lower_file) = dentry_open(&req.path, flags, cred);
+		if (!IS_ERR(*lower_file))
+			goto out;
+		if ((flags & O_ACCMODE) == O_RDONLY) {
+			rc = PTR_ERR((*lower_file));
+			goto out;
+		}
 	}
+
 	mutex_lock(&ecryptfs_kthread_ctl.mux);
 	if (ecryptfs_kthread_ctl.flags & ECRYPTFS_KTHREAD_ZOMBIE) {
 		rc = -EIO;
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH] ecryptfs: open lower files on kthread under SELinux

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 4770 bytes --]

On 2016-03-31 19:36:38, Ricky Zhou wrote:
> When SELinux is enabled, files are associated with the current task's
> SID when they are opened. Before this change, the lower file would
> sometimes be associated with the SID of the first process to open the
> corresponding eCryptfs file. If a second process attempts to access this
> file through the same lower file, the access is denied unless the
> SELinux policy allows the second process to use files opened by the
> first process (the FD__USE permission).
> 
> Since it may not be desirable to allow all processes this permission,
> this change always opens the lower file from a thread with the kernel
> SID. This way, the policy only needs to grant all processes FD__USE on
> the kernel context (which in practice, is exactly the use of eCryptfs
> lower files).

Hi Ricky - Thanks for patch!

This is interesting because it gets close to fixing a long standing bug
with eCryptfs and AppArmor. Access has to be granted to the lower
mountpoint in AppArmor profiles of processes that deal with files in
eCryptfs mounts. It is similar to the eCryptfs and SELinux bug that
you're trying to address.

Because of the AppArmor issue, I initially thought to recommend *always*
delegating lower opens to the kthread. If we're looking at doing it when
AppArmor or SELinux are enabled, I think we're talking about a good
number of Linux systems. We might as well always do it.

However, I don't think that this patch is the final solution that I'd
like to see. There are a couple reasons:

1) The kthread doesn't look like it will perform well enough to handle
   all lower opens. We could probably squeeze some extra performance out
   of it but I think the better solution is to temporarily change creds,
   do the lower open, and change back to the original cred. See callers
   of prepare_creds() for examples of this.

2) I don't think that delegating open() is sufficient. What about inode
   based operations such as chown()? I think that we'll have a very
   similar problem.

I wanted to have a little bit better solution planned out before
responding but I haven't had sufficient time. I'll give it some more
thought. Feel free to make suggestions in the meantime. :)

Tyler

> 
> Signed-off-by: Ricky Zhou <rickyz@chromium.org>
> ---
>  fs/ecryptfs/kthread.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
> index 866bb18..0ce7828 100644
> --- a/fs/ecryptfs/kthread.c
> +++ b/fs/ecryptfs/kthread.c
> @@ -25,6 +25,7 @@
>  #include <linux/slab.h>
>  #include <linux/wait.h>
>  #include <linux/mount.h>
> +#include <linux/selinux.h>
>  #include "ecryptfs_kernel.h"
>  
>  struct ecryptfs_open_req {
> @@ -141,17 +142,30 @@ int ecryptfs_privileged_open(struct file **lower_file,
>  	req.path.dentry = lower_dentry;
>  	req.path.mnt = lower_mnt;
>  
> -	/* Corresponding dput() and mntput() are done when the
> -	 * lower file is fput() when all eCryptfs files for the inode are
> -	 * released. */
> -	flags |= IS_RDONLY(d_inode(lower_dentry)) ? O_RDONLY : O_RDWR;
> -	(*lower_file) = dentry_open(&req.path, flags, cred);
> -	if (!IS_ERR(*lower_file))
> -		goto out;
> -	if ((flags & O_ACCMODE) == O_RDONLY) {
> -		rc = PTR_ERR((*lower_file));
> -		goto out;
> +	/*
> +	 * When SELinux is enabled, force the lower file to be opened by the
> +	 * kernel thread. Otherwise, the lower file will be associated with the
> +	 * SELinux context of the first process that opens it, which may prevent
> +	 * a different process from using it, unless that process has permission
> +	 * to use fds from the first process.
> +	 *
> +	 * If SELinux is enabled, then any process that needs read access to an
> +	 * ecryptfs filesytem must allow fd:use on the kernel context.
> +	 */
> +	if (!selinux_is_enabled()) {
> +		/* Corresponding dput() and mntput() are done when the lower
> +		 * file is fput() when all eCryptfs files for the inode are
> +		 * released. */
> +		flags |= IS_RDONLY(d_inode(lower_dentry)) ? O_RDONLY : O_RDWR;
> +		(*lower_file) = dentry_open(&req.path, flags, cred);
> +		if (!IS_ERR(*lower_file))
> +			goto out;
> +		if ((flags & O_ACCMODE) == O_RDONLY) {
> +			rc = PTR_ERR((*lower_file));
> +			goto out;
> +		}
>  	}
> +
>  	mutex_lock(&ecryptfs_kthread_ctl.mux);
>  	if (ecryptfs_kthread_ctl.flags & ECRYPTFS_KTHREAD_ZOMBIE) {
>  		rc = -EIO;
> -- 
> 2.8.0.rc3.226.g39d4020
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] ecryptfs: open lower files on kthread under SELinux

[Ricky Zhou]

On Fri, Apr 8, 2016 at 5:15 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> 1) The kthread doesn't look like it will perform well enough to handle
>    all lower opens. We could probably squeeze some extra performance out
>    of it but I think the better solution is to temporarily change creds,
>    do the lower open, and change back to the original cred. See callers
>    of prepare_creds() for examples of this.
Yup, that was definitely my biggest concern with this change - thanks
for the pointers, I'll take a look at what it'll take to replace the
kthread with override_creds/revert_creds instead.

> 2) I don't think that delegating open() is sufficient. What about inode
>    based operations such as chown()? I think that we'll have a very
>    similar problem.
My impression was that this wasn't likely to be an issue since for
normal filesystems, SELinux doesn't associate any contexts from the
current task with inodes (task_sid in inode_security_struct should
only be used for things like sockets or tmpfs - and the case of tmpfs,
I think the resulting behavior is actually expected and correct under
ecryptfs).

Thanks,
Ricky