On Tue, Jul 14, 2020 at 09:42:53PM +0300, Amir Goldstein wrote:
> On Tue, Jul 14, 2020 at 9:07 PM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> >
> > On Mon, Jul 13, 2020 at 05:19:42PM +0300, Amir Goldstein wrote:
> > > Miklos, Vivek,
> > >
> > > Following discussion on following an unsafe non-dir origin [1]
> > > and in a addition to a fix for the reported null uuid case [2] and to
> > > Vivek's doc clarification [3], I am proposing to piggy back existing
> > > config redirect_dir=nofollow to also not follow non-dir origin.
> > >
> > > Like in the case of non-dir origin, following redirects behavior was
> > > added with no opt-out option in kernel v4.10. Later security concerns
> > > about following malformed redirects resulted in the redirect_dir=nofollow
> > > config option.
> >
> > So what's the security issue you are seeing with malformed origin? If
>
> TBH I never really understood the thread that led to redirect_dir=nofollow.
> I don't think anyone has presented a proper use case that can be discussed,
IIUC, idea was that automated mounting can mount a handcrafted upper on
usb hence allow access to directories on host which are otherwise
inaccessible.
> so I just treat this config option as "paranoia" or "don't give me anything that
> very old overlay did not give me".
> Therefore I suggested piggybacking on it.
Even if it is paranoia, put more unrelated checks under this option does
not make much sense to me. It will make things just more confusing.
Anyway, redirect_dir=nofollow is a thing of past. Now if you want to
not follow origin, then we first need to have a genuine explanation
of why to do that (and not be driven by just paranoia).
> Of course if we do, we will need to document that.
redirect_dir=nofollow resulting in origin not being followed is plain
unintuitive to me. Why not introduce another option if not following
origin is so important.
Thanks
Vivek
> BTW, I have another patch that does not follow mismatched lower dir origin
> with redirect_dir=nofollow, i.e.:
>
> bool ovl_verify_lower(struct super_block *sb)
> {
> struct ovl_fs *ofs = sb->s_fs_info;
>
> - return ofs->config.nfs_export && ofs->config.index;
> + return ofs->config.nfs_export || !ofs->config.redirect_follow;
> }
>
> That makes even more sense for "paranoia" IMO.
>
> Thanks,
> Amir.
>
