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: <CAOQ4uxjHXmYGH3wUO=w+tM+CiFWBjWvZEZZkSXA5FO8T+VP4mA@mail.gmail.com>
Date: Tue, 12 Nov 2024 23:37:23 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Josef Bacik <josef@...icpanda.com>, kernel-team@...com, linux-fsdevel@...r.kernel.org, 
	jack@...e.cz, brauner@...nel.org, linux-xfs@...r.kernel.org, 
	linux-btrfs@...r.kernel.org, linux-mm@...ck.org, linux-ext4@...r.kernel.org
Subject: Re: [PATCH v7 01/18] fsnotify: opt-in for permission events at
 file_open_perm() time

On Tue, Nov 12, 2024 at 8:46 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@...icpanda.com> wrote:
> >
> > @@ -119,14 +118,37 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
> >          * handle creation / destruction events and not "real" file events.
> >          */
> >         if (file->f_mode & (FMODE_NONOTIFY | FMODE_PATH))
> > +               return false;
> > +
> > +       /* Permission events require that watches are set before FS_OPEN_PERM */
> > +       if (mask & ALL_FSNOTIFY_PERM_EVENTS & ~FS_OPEN_PERM &&
> > +           !(file->f_mode & FMODE_NOTIFY_PERM))
> > +               return false;
>
> This still all looks very strange.
>
> As far as I can tell, there is exactly one user of FS_OPEN_PERM in
> 'mask', and that's fsnotify_open_perm(). Which is called in exactly
> one place: security_file_open(), which is the wrong place to call it
> anyway and is the only place where fsnotify is called from the
> security layer.
>
> In fact, that looks like an active bug: if you enable FSNOTIFY, but
> you *don't* enable CONFIG_SECURITY, the whole fsnotify_open_perm()
> will never be called at all.
>
> And I just verified that yes, you can very much generate such a config.
>

See: 1cda52f1b461 fsnotify, lsm: Decouple fsnotify from lsm
in linux-next. This patch set is based on the fs-next branch.

> So the whole FS_OPEN_PERM thing looks like a special case, called from
> a (broken) special place, and now polluting this "fsnotify_file()"
> logic for no actual reason and making it all look unnecessarily messy.
>
> I'd suggest that the whole fsnotify_open_perm() simply be moved to
> where it *should* be - in the open path - and not make a bad and
> broken attempt at hiding inside the security layer, and not use this
> "fsnotify_file()" logic at all.
>
> The open-time logic is different. It shouldn't even attempt - badly -
> to look like it's the same thing as some regular file access.
>

OK, we can move setting the FMODE_NOTIFY_PERM to the open path.
I have considered that it may be better to unhide it, but wasn't sure.

Thanks,
Amir.

Powered by blists - more mailing lists