Re: fsnotify events for overlayfs real file

Re: fsnotify events for overlayfs real file

[Amir Goldstein]

> > > > FYI, a privileged user can already mount an overlayfs in order to indirectly
> > > > open and write to a file.
> > > >
> > > > Because overlayfs opens the underlying file FMODE_NONOTIFY this will
> > > > hide OPEN/ACCESS/MODIFY/CLOSE events also for inode/sb marks.
> > > > Since 459c7c565ac3 ("ovl: unprivieged mounts"), so can unprivileged users.
> > > >
> > > > I wonder if that is a problem that we need to fix...
> > >
> > > I assume you are speaking of the filesystem that is absorbing the changes?
> > > AFAIU usually you are not supposed to access that filesystem alone but
> > > always access it only through overlayfs and in that case you won't see the
> > > problem?
> > >
> >
> > Yes I am talking about the "backend" store for overlayfs.
> > Normally, that would be a subtree where changes are not expected
> > except through overlayfs and indeed it is documented that:
> > "If the underlying filesystem is changed, the behavior of the overlay
> >  is undefined, though it will not result in a crash or deadlock."
> > Not reporting events falls well under "undefined".
> >
> > But that is not the problem.
> > The problem is that if user A is watching a directory D for changes, then
> > an adversary user B which has read/write access to D can:
> > - Clone a userns wherein user B id is 0
> > - Mount a private overlayfs instance using D as upperdir
> > - Open file in D indirectly via private overlayfs and edit it
> >
> > So it does not require any special privileges to circumvent generating
> > events. Unless I am missing something.
>
> I see, right. I agree that is unfortunate especially for stuff like audit
> or fanotify permission events so we should fix that.
>

Miklos,

Do you recall what is the reason for using FMODE_NONOTIFY
for realfile?

I can see that events won't be generated anyway for watchers of
underlying file, because fsnotify_file() looks at the "fake" path
(i.e. the overlay file path).

I recently looked at a similar issue w.r.t file_remove_privs() when
I was looking at passing mnt context to notify_change() [1].

My thinking was that we can change d_real() to provide the real path:

static inline struct path d_real_path(struct path *path,
                                    const struct inode *inode)
{
        struct realpath = {};
        if (!unlikely(dentry->d_flags & DCACHE_OP_REAL))
               return *path;
        dentry->d_op->d_real(path->dentry, inode, &realpath);
        return realpath;
}

static inline struct dentry *d_real(struct dentry *dentry,
                                    const struct inode *inode)
{
        struct realpath = {};
        if (!unlikely(dentry->d_flags & DCACHE_OP_REAL))
               return dentry;
        dentry->d_op->d_real(path->dentry, inode, &realpath);
        return realpath.dentry;
}


Another option, instead of getting the realpath, just detect the
mismatch of file_inode(file) != d_inode(path->dentry) in
fanotify_file() and pass FSNOTIFY_EVENT_DENTRY data type
with d_real() dentry to backend instead of FSNOTIFY_EVENT_PATH.

For inotify it should be enough and for fanotify it is enough for
FAN_REPORT_FID and legacy fanotify can report FAN_NOFD,
so at least permission events listeners can identify the situation and
be able to block access to unknown paths.

Am I overcomplicating this?

Any magic solution that I am missing?

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/CAOQ4uxiWb5Auyrbrj44hvdMcvMhx1YPRrR90RkicntmyfF+Ugw@mail.gmail.com/

Re: fsnotify events for overlayfs real file

[Miklos Szeredi]

On Mon, 10 May 2021 at 18:32, Amir Goldstein <amir73il@gmail.com> wrote:
>

> > I see, right. I agree that is unfortunate especially for stuff like audit
> > or fanotify permission events so we should fix that.
> >
>
> Miklos,
>
> Do you recall what is the reason for using FMODE_NONOTIFY
> for realfile?

Commit d989903058a8 ("ovl: do not generate duplicate fsnotify events
for "fake" path").

> I can see that events won't be generated anyway for watchers of
> underlying file, because fsnotify_file() looks at the "fake" path
> (i.e. the overlay file path).
>
> I recently looked at a similar issue w.r.t file_remove_privs() when
> I was looking at passing mnt context to notify_change() [1].
>
> My thinking was that we can change d_real() to provide the real path:
>
> static inline struct path d_real_path(struct path *path,
>                                     const struct inode *inode)
> {
>         struct realpath = {};
>         if (!unlikely(dentry->d_flags & DCACHE_OP_REAL))
>                return *path;
>         dentry->d_op->d_real(path->dentry, inode, &realpath);
>         return realpath;
> }
>
> static inline struct dentry *d_real(struct dentry *dentry,
>                                     const struct inode *inode)
> {
>         struct realpath = {};
>         if (!unlikely(dentry->d_flags & DCACHE_OP_REAL))
>                return dentry;
>         dentry->d_op->d_real(path->dentry, inode, &realpath);
>         return realpath.dentry;
> }
>
>
> Another option, instead of getting the realpath, just detect the
> mismatch of file_inode(file) != d_inode(path->dentry) in
> fanotify_file() and pass FSNOTIFY_EVENT_DENTRY data type
> with d_real() dentry to backend instead of FSNOTIFY_EVENT_PATH.
>
> For inotify it should be enough and for fanotify it is enough for
> FAN_REPORT_FID and legacy fanotify can report FAN_NOFD,
> so at least permission events listeners can identify the situation and
> be able to block access to unknown paths.
>
> Am I overcomplicating this?
>
> Any magic solution that I am missing?

Agree, dentry events should still happen.

Path events: what happens if you bind mount, then detach (lazy
umount)?   Isn't that exactly the same as what overlayfs does on the
underlying mounts?

Thanks,
Miklos

Re: fsnotify events for overlayfs real file

[Amir Goldstein]

On Tue, May 18, 2021 at 5:43 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 10 May 2021 at 18:32, Amir Goldstein <amir73il@gmail.com> wrote:
> >
>
> > > I see, right. I agree that is unfortunate especially for stuff like audit
> > > or fanotify permission events so we should fix that.
> > >
> >
> > Miklos,
> >
> > Do you recall what is the reason for using FMODE_NONOTIFY
> > for realfile?
>
> Commit d989903058a8 ("ovl: do not generate duplicate fsnotify events
> for "fake" path").
>
> > I can see that events won't be generated anyway for watchers of
> > underlying file, because fsnotify_file() looks at the "fake" path
> > (i.e. the overlay file path).
> >
> > I recently looked at a similar issue w.r.t file_remove_privs() when
> > I was looking at passing mnt context to notify_change() [1].
> >
> > My thinking was that we can change d_real() to provide the real path:
> >
> > static inline struct path d_real_path(struct path *path,
> >                                     const struct inode *inode)
> > {
> >         struct realpath = {};
> >         if (!unlikely(dentry->d_flags & DCACHE_OP_REAL))
> >                return *path;
> >         dentry->d_op->d_real(path->dentry, inode, &realpath);
> >         return realpath;
> > }
> >
> > static inline struct dentry *d_real(struct dentry *dentry,
> >                                     const struct inode *inode)
> > {
> >         struct realpath = {};
> >         if (!unlikely(dentry->d_flags & DCACHE_OP_REAL))
> >                return dentry;
> >         dentry->d_op->d_real(path->dentry, inode, &realpath);
> >         return realpath.dentry;
> > }
> >
> >
> > Another option, instead of getting the realpath, just detect the
> > mismatch of file_inode(file) != d_inode(path->dentry) in
> > fanotify_file() and pass FSNOTIFY_EVENT_DENTRY data type
> > with d_real() dentry to backend instead of FSNOTIFY_EVENT_PATH.
> >
> > For inotify it should be enough and for fanotify it is enough for
> > FAN_REPORT_FID and legacy fanotify can report FAN_NOFD,
> > so at least permission events listeners can identify the situation and
> > be able to block access to unknown paths.
> >
> > Am I overcomplicating this?
> >
> > Any magic solution that I am missing?
>
> Agree, dentry events should still happen.

Alas, the more critical issue is with fanotify permission events
which are path events.

>
> Path events: what happens if you bind mount, then detach (lazy
> umount)?   Isn't that exactly the same as what overlayfs does on the
> underlying mounts?
>

No, it isn't.

In the detached mount case, the fanotify listener will get an open fd
from fanotify_user.c::create_fd() using dentry_open() on the detached
path. When the listener will read proc/self/fd/<fd> symlink, it will see
the path from the fs root.

Assuming that marks were set on a group of files or directories
with FAN_OPEN_PERM in the event mask, the path from fs root provides
enough information to understand what is going on and in any case the
fd can be used to read metadata or content from the file before making the
permission decision.

Is there a reason for the fake path besides the displayed path in
/proc/self/maps?

Does it make sense to keep one realfile with fake path for mmaped
files along side a realfile with private/detached path used for all the
other operations?

While at it, we can also cache both upper and lower realfiles in case
file was copied up after open.

Better ideas?

Thanks,
Amir.

Re: fsnotify events for overlayfs real file

[Miklos Szeredi]

On Tue, 18 May 2021 at 19:56, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, May 18, 2021 at 5:43 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, 10 May 2021 at 18:32, Amir Goldstein <amir73il@gmail.com> wrote:

> > > My thinking was that we can change d_real() to provide the real path:
> > >
> > > static inline struct path d_real_path(struct path *path,
> > >                                     const struct inode *inode)
> > > {
> > >         struct realpath = {};
> > >         if (!unlikely(dentry->d_flags & DCACHE_OP_REAL))
> > >                return *path;
> > >         dentry->d_op->d_real(path->dentry, inode, &realpath);
> > >         return realpath;
> > > }

Real paths are internal, we can't pass them (as fd in permission
events) to userspace.

> > >
> > >
> > > Another option, instead of getting the realpath, just detect the
> > > mismatch of file_inode(file) != d_inode(path->dentry) in
> > > fanotify_file() and pass FSNOTIFY_EVENT_DENTRY data type
> > > with d_real() dentry to backend instead of FSNOTIFY_EVENT_PATH.
> > >
> > > For inotify it should be enough and for fanotify it is enough for
> > > FAN_REPORT_FID and legacy fanotify can report FAN_NOFD,
> > > so at least permission events listeners can identify the situation and
> > > be able to block access to unknown paths.

That sounds like a good short term solution.


>
> Is there a reason for the fake path besides the displayed path in
> /proc/self/maps?

I'm not aware of any.

>
> Does it make sense to keep one realfile with fake path for mmaped
> files along side a realfile with private/detached path used for all the
> other operations?

This should work, but it would add more open files, so needs some good
justifications.

> While at it, we can also cache both upper and lower realfiles in case
> file was copied up after open.

Right, although this doesn't seem to be an issue (it's a rare corner
case that is being cared for).

Thanks,
Miklos

Re: fsnotify events for overlayfs real file

[Amir Goldstein]

On Mon, May 31, 2021 at 6:18 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 18 May 2021 at 19:56, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, May 18, 2021 at 5:43 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Mon, 10 May 2021 at 18:32, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > > > My thinking was that we can change d_real() to provide the real path:
> > > >
> > > > static inline struct path d_real_path(struct path *path,
> > > >                                     const struct inode *inode)
> > > > {
> > > >         struct realpath = {};
> > > >         if (!unlikely(dentry->d_flags & DCACHE_OP_REAL))
> > > >                return *path;
> > > >         dentry->d_op->d_real(path->dentry, inode, &realpath);
> > > >         return realpath;
> > > > }
>
> Real paths are internal, we can't pass them (as fd in permission
> events) to userspace.
>
> > > >
> > > >
> > > > Another option, instead of getting the realpath, just detect the
> > > > mismatch of file_inode(file) != d_inode(path->dentry) in
> > > > fanotify_file() and pass FSNOTIFY_EVENT_DENTRY data type
> > > > with d_real() dentry to backend instead of FSNOTIFY_EVENT_PATH.
> > > >
> > > > For inotify it should be enough and for fanotify it is enough for
> > > > FAN_REPORT_FID and legacy fanotify can report FAN_NOFD,
> > > > so at least permission events listeners can identify the situation and
> > > > be able to block access to unknown paths.
>
> That sounds like a good short term solution.
>

It may be a fine academic solution, but I don't think it solves any real
world problem.
I am not aware of any security oriented solutions that use permission
events on inode or directory (let alone sb).

The security oriented users of fanotify are anti-virus on-access
protection engines and those are using mount marks anyway
(dynamically adding them as far as I know).
[cc Marko who may be able to shed some light]

For those products, creating a bind mount inside a new mount ns
may actually escape the on-access policy or the new mount will
also be marked I am not sure. I suppose cloning mount ns may be
prohibited by another LSM or something(?).

>
> >
> > Is there a reason for the fake path besides the displayed path in
> > /proc/self/maps?
>
> I'm not aware of any.
>
> >
> > Does it make sense to keep one realfile with fake path for mmaped
> > files along side a realfile with private/detached path used for all the
> > other operations?
>
> This should work, but it would add more open files,

True, but only for the mmaped files.

> so needs some good justifications.
>

I'm afraid I don't have one yet..
Let's see what the AV vendors have to say.

Thanks,
Amir.

Re: fsnotify events for overlayfs real file

[Christian Brauner]

On Mon, May 31, 2021 at 09:26:35PM +0300, Amir Goldstein wrote:
> On Mon, May 31, 2021 at 6:18 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, 18 May 2021 at 19:56, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, May 18, 2021 at 5:43 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Mon, 10 May 2021 at 18:32, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > > > My thinking was that we can change d_real() to provide the real path:
> > > > >
> > > > > static inline struct path d_real_path(struct path *path,
> > > > >                                     const struct inode *inode)
> > > > > {
> > > > >         struct realpath = {};
> > > > >         if (!unlikely(dentry->d_flags & DCACHE_OP_REAL))
> > > > >                return *path;
> > > > >         dentry->d_op->d_real(path->dentry, inode, &realpath);
> > > > >         return realpath;
> > > > > }
> >
> > Real paths are internal, we can't pass them (as fd in permission
> > events) to userspace.
> >
> > > > >
> > > > >
> > > > > Another option, instead of getting the realpath, just detect the
> > > > > mismatch of file_inode(file) != d_inode(path->dentry) in
> > > > > fanotify_file() and pass FSNOTIFY_EVENT_DENTRY data type
> > > > > with d_real() dentry to backend instead of FSNOTIFY_EVENT_PATH.
> > > > >
> > > > > For inotify it should be enough and for fanotify it is enough for
> > > > > FAN_REPORT_FID and legacy fanotify can report FAN_NOFD,
> > > > > so at least permission events listeners can identify the situation and
> > > > > be able to block access to unknown paths.
> >
> > That sounds like a good short term solution.
> >
> 
> It may be a fine academic solution, but I don't think it solves any real
> world problem.
> I am not aware of any security oriented solutions that use permission
> events on inode or directory (let alone sb).
> 
> The security oriented users of fanotify are anti-virus on-access
> protection engines and those are using mount marks anyway
> (dynamically adding them as far as I know).
> [cc Marko who may be able to shed some light]
> 
> For those products, creating a bind mount inside a new mount ns
> may actually escape the on-access policy or the new mount will
> also be marked I am not sure. I suppose cloning mount ns may be
> prohibited by another LSM or something(?).

Yes, this can be restricted in multiple ways. Three spring to mind right
away:
- procfs: write a really low number to /proc/sys/user/max_mnt_namespaces
- seccomp: prevent the clone3() syscall, prevent the legacy clone()
  syscall with CLONE_NEWNS, prevent unshare(CLONE_NEWNS)
- use LSM

> 
> >
> > >
> > > Is there a reason for the fake path besides the displayed path in
> > > /proc/self/maps?
> >
> > I'm not aware of any.
> >
> > >
> > > Does it make sense to keep one realfile with fake path for mmaped
> > > files along side a realfile with private/detached path used for all the
> > > other operations?
> >
> > This should work, but it would add more open files,
> 
> True, but only for the mmaped files.
> 
> > so needs some good justifications.
> >
> 
> I'm afraid I don't have one yet..
> Let's see what the AV vendors have to say.
> 
> Thanks,
> Amir.

Re: fsnotify events for overlayfs real file

[Marko Rauhamaa]

Amir Goldstein <amir73il@gmail.com>:
> The security oriented users of fanotify are anti-virus on-access
> protection engines and those are using mount marks anyway
> (dynamically adding them as far as I know).
> [cc Marko who may be able to shed some light]

(Thanks for the CC; back from vacation.)

Yes.

> For those products, creating a bind mount inside a new mount ns
> may actually escape the on-access policy or the new mount will
> also be marked I am not sure. I suppose cloning mount ns may be
> prohibited by another LSM or something(?).

Not sure I appreciate all dimensions of the problem space, but I don't
immediately foresee overwhelming escaping problems.


Marko