[PATCH] ovl: redirect_dir=nofollow should not follow redirect for opaque lower

[PATCH] ovl: redirect_dir=nofollow should not follow redirect for opaque lower

[Vivek Goyal]

redirect_dir=nofollow should not follow a redirect. But in a specific
configuration it can still follow it.  For example try this.

$ mkdir -p lower0 lower1/foo upper work merged
$ touch lower1/foo/lower-file.txt
$ setfattr -n "trusted.overlay.opaque" -v "y" lower1/foo
$ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=on none merged
$ cd merged
$ mv foo foo-renamed
$ umount merged

# mount again. This time with redirect_dir=nofollow
$ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=nofollow none merged
$ ls merged/foo-renamed/
# This lists lower-file.txt, while it should not have.

Basically, we are doing redirect check after we check for d.stop. And
if this is not last lower, and we find an opaque lower, d.stop will be
set.

ovl_lookup_single()
        if (!d->last && ovl_is_opaquedir(this)) {
                d->stop = d->opaque = true;
                goto out;
        }

To fix this, first check redirect is allowed. And after that check if
d.stop has been set or not.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index beb945e1963c..ef3e7ea76296 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -678,9 +678,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		stack[ctr].layer = lower.layer;
 		ctr++;
 
-		if (d.stop)
-			break;
-
 		/*
 		 * Following redirects can have security consequences: it's like
 		 * a symlink into the lower layer without the permission checks.
@@ -697,6 +694,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			goto out_put;
 		}
 
+		if (d.stop)
+			break;
+
 		if (d.redirect && d.redirect[0] == '/' && poe != roe) {
 			poe = roe;
 
-- 
2.13.6

Re: [PATCH] ovl: redirect_dir=nofollow should not follow redirect for opaque lower

[Amir Goldstein]

On Fri, Feb 2, 2018 at 5:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> redirect_dir=nofollow should not follow a redirect. But in a specific
> configuration it can still follow it.  For example try this.
>
> $ mkdir -p lower0 lower1/foo upper work merged
> $ touch lower1/foo/lower-file.txt
> $ setfattr -n "trusted.overlay.opaque" -v "y" lower1/foo
> $ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=on none merged
> $ cd merged
> $ mv foo foo-renamed
> $ umount merged
>
> # mount again. This time with redirect_dir=nofollow
> $ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=nofollow none merged
> $ ls merged/foo-renamed/
> # This lists lower-file.txt, while it should not have.
>
> Basically, we are doing redirect check after we check for d.stop. And
> if this is not last lower, and we find an opaque lower, d.stop will be
> set.
>
> ovl_lookup_single()
>         if (!d->last && ovl_is_opaquedir(this)) {
>                 d->stop = d->opaque = true;
>                 goto out;
>         }
>
> To fix this, first check redirect is allowed. And after that check if
> d.stop has been set or not.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Looks good.
Probably should have Fixes and Cc: stable

> ---
>  fs/overlayfs/namei.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index beb945e1963c..ef3e7ea76296 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -678,9 +678,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 stack[ctr].layer = lower.layer;
>                 ctr++;
>
> -               if (d.stop)
> -                       break;
> -
>                 /*
>                  * Following redirects can have security consequences: it's like
>                  * a symlink into the lower layer without the permission checks.
> @@ -697,6 +694,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         goto out_put;
>                 }
>
> +               if (d.stop)
> +                       break;
> +
>                 if (d.redirect && d.redirect[0] == '/' && poe != roe) {
>                         poe = roe;
>
> --
> 2.13.6
>

Re: [PATCH] ovl: redirect_dir=nofollow should not follow redirect for opaque lower

[Miklos Szeredi]

On Fri, Feb 2, 2018 at 4:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> redirect_dir=nofollow should not follow a redirect. But in a specific
> configuration it can still follow it.  For example try this.
>
> $ mkdir -p lower0 lower1/foo upper work merged
> $ touch lower1/foo/lower-file.txt
> $ setfattr -n "trusted.overlay.opaque" -v "y" lower1/foo
> $ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=on none merged
> $ cd merged
> $ mv foo foo-renamed
> $ umount merged
>
> # mount again. This time with redirect_dir=nofollow
> $ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=nofollow none merged
> $ ls merged/foo-renamed/
> # This lists lower-file.txt, while it should not have.
>
> Basically, we are doing redirect check after we check for d.stop. And
> if this is not last lower, and we find an opaque lower, d.stop will be
> set.
>
> ovl_lookup_single()
>         if (!d->last && ovl_is_opaquedir(this)) {
>                 d->stop = d->opaque = true;
>                 goto out;
>         }
>
> To fix this, first check redirect is allowed. And after that check if
> d.stop has been set or not.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Thanks, applied.

Miklos

Re: [PATCH] ovl: redirect_dir=nofollow should not follow redirect for opaque lower

[Amir Goldstein]

On Fri, Feb 2, 2018 at 5:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> redirect_dir=nofollow should not follow a redirect. But in a specific
> configuration it can still follow it.  For example try this.
>
> $ mkdir -p lower0 lower1/foo upper work merged
> $ touch lower1/foo/lower-file.txt
> $ setfattr -n "trusted.overlay.opaque" -v "y" lower1/foo
> $ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=on none merged
> $ cd merged
> $ mv foo foo-renamed
> $ umount merged
>
> # mount again. This time with redirect_dir=nofollow
> $ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=nofollow none merged
> $ ls merged/foo-renamed/
> # This lists lower-file.txt, while it should not have.
>
> Basically, we are doing redirect check after we check for d.stop. And
> if this is not last lower, and we find an opaque lower, d.stop will be
> set.
>
> ovl_lookup_single()
>         if (!d->last && ovl_is_opaquedir(this)) {
>                 d->stop = d->opaque = true;
>                 goto out;
>         }
>
> To fix this, first check redirect is allowed. And after that check if
> d.stop has been set or not.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

This is already in overlayfs-next, which is the branch you should be
rebasing on..

Thanks,
Amir.

Re: [PATCH] ovl: redirect_dir=nofollow should not follow redirect for opaque lower

[Amir Goldstein]

On Wed, Mar 7, 2018 at 9:25 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Feb 2, 2018 at 5:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> redirect_dir=nofollow should not follow a redirect. But in a specific
>> configuration it can still follow it.  For example try this.
>>
>> $ mkdir -p lower0 lower1/foo upper work merged
>> $ touch lower1/foo/lower-file.txt
>> $ setfattr -n "trusted.overlay.opaque" -v "y" lower1/foo
>> $ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=on none merged
>> $ cd merged
>> $ mv foo foo-renamed
>> $ umount merged
>>
>> # mount again. This time with redirect_dir=nofollow
>> $ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=nofollow none merged
>> $ ls merged/foo-renamed/
>> # This lists lower-file.txt, while it should not have.
>>
>> Basically, we are doing redirect check after we check for d.stop. And
>> if this is not last lower, and we find an opaque lower, d.stop will be
>> set.
>>
>> ovl_lookup_single()
>>         if (!d->last && ovl_is_opaquedir(this)) {
>>                 d->stop = d->opaque = true;
>>                 goto out;
>>         }
>>
>> To fix this, first check redirect is allowed. And after that check if
>> d.stop has been set or not.
>>
>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>
> This is already in overlayfs-next, which is the branch you should be
> rebasing on..
>


Sorry, that was supposed to be a reply on first patch of metacopy series

Re: [PATCH] ovl: redirect_dir=nofollow should not follow redirect for opaque lower

[Vivek Goyal]

On Wed, Mar 07, 2018 at 09:25:27AM +0200, Amir Goldstein wrote:
> On Fri, Feb 2, 2018 at 5:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > redirect_dir=nofollow should not follow a redirect. But in a specific
> > configuration it can still follow it.  For example try this.
> >
> > $ mkdir -p lower0 lower1/foo upper work merged
> > $ touch lower1/foo/lower-file.txt
> > $ setfattr -n "trusted.overlay.opaque" -v "y" lower1/foo
> > $ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=on none merged
> > $ cd merged
> > $ mv foo foo-renamed
> > $ umount merged
> >
> > # mount again. This time with redirect_dir=nofollow
> > $ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=nofollow none merged
> > $ ls merged/foo-renamed/
> > # This lists lower-file.txt, while it should not have.
> >
> > Basically, we are doing redirect check after we check for d.stop. And
> > if this is not last lower, and we find an opaque lower, d.stop will be
> > set.
> >
> > ovl_lookup_single()
> >         if (!d->last && ovl_is_opaquedir(this)) {
> >                 d->stop = d->opaque = true;
> >                 goto out;
> >         }
> >
> > To fix this, first check redirect is allowed. And after that check if
> > d.stop has been set or not.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> This is already in overlayfs-next, which is the branch you should be
> rebasing on..

Hmm.., I generally work with linus's tree. I knew that this patch is
in overlayfs-next. Assumed that it will be dropped while merging.

Next version I can rebase on top of overlayfs-next and post.

Vivek