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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHC9VhRp5hMsmZ9jUok+5c20U37XLiXmoEAguorTqRF5MQq2Gg@mail.gmail.com>
Date: Mon, 12 Aug 2024 18:26:58 -0400
From: Paul Moore <paul@...l-moore.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Christian Brauner <brauner@...nel.org>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org, 
	selinux@...r.kernel.org, Jan Kara <jack@...e.cz>, 
	Tahera Fahimi <fahimitahera@...il.com>, Al Viro <viro@...iv.linux.org.uk>, 
	Casey Schaufler <casey@...aufler-ca.com>, James Morris <jmorris@...ei.org>, Jann Horn <jannh@...gle.com>, 
	Ondrej Mosnacek <omosnace@...hat.com>, "Serge E . Hallyn" <serge@...lyn.com>, 
	Stephen Smalley <stephen.smalley.work@...il.com>
Subject: Re: [PATCH v2] fs,security: Fix file_set_fowner LSM hook inconsistencies

On Mon, Aug 12, 2024 at 1:44 PM Mickaël Salaün <mic@...ikod.net> wrote:
>
> The fcntl's F_SETOWN command sets the process that handle SIGIO/SIGURG
> for the related file descriptor.  Before this change, the
> file_set_fowner LSM hook was used to store this information.  However,
> there are three issues with this approach:
>
> - Because security_file_set_fowner() only get one argument, all hook
>   implementations ignore the VFS logic which may not actually change the
>   process that handles SIGIO (e.g. TUN, TTY, dnotify).
>
> - Because security_file_set_fowner() is called before f_modown() without
>   lock (e.g. f_owner.lock), concurrent F_SETOWN commands could result to
>   a race condition and inconsistent LSM states (e.g. SELinux's fown_sid)
>   compared to struct fown_struct's UID/EUID.
>
> - Because the current hook implementations does not use explicit atomic
>   operations, they may create inconsistencies.  It would help to
>   completely remove this constraint, as well as the requirements of the
>   RCU read-side critical section for the hook.
>
> Fix these issues by replacing f_owner.uid and f_owner.euid with a new
> f_owner.cred [1].  This also saves memory by removing dedicated LSM
> blobs, and simplifies code by removing file_set_fowner hook
> implementations for SELinux and Smack.
>
> This changes enables to remove the smack_file_alloc_security
> implementation, Smack's file blob, and SELinux's
> file_security_struct->fown_sid field.
>
> As for the UID/EUID, f_owner.cred is not always updated.  Move the
> file_set_fowner hook to align with the VFS semantic.  This hook does not
> have user anymore [2].
>
> Before this change, f_owner's UID/EUID were initialized to zero
> (i.e. GLOBAL_ROOT_UID), but to simplify code, f_owner's cred is now
> initialized with the file descriptor creator's credentials (i.e.
> file->f_cred), which is more consistent and simplifies LSMs logic.  The
> sigio_perm()'s semantic does not need any change because SIGIO/SIGURG
> are only sent when a process is explicitly set with __f_setown().
>
> Rename f_modown() to __f_setown() to simplify code.
>
> Cc: Al Viro <viro@...iv.linux.org.uk>
> Cc: Casey Schaufler <casey@...aufler-ca.com>
> Cc: Christian Brauner <brauner@...nel.org>
> Cc: James Morris <jmorris@...ei.org>
> Cc: Jann Horn <jannh@...gle.com>
> Cc: Ondrej Mosnacek <omosnace@...hat.com>
> Cc: Paul Moore <paul@...l-moore.com>
> Cc: Serge E. Hallyn <serge@...lyn.com>
> Cc: Stephen Smalley <stephen.smalley.work@...il.com>
> Link: https://lore.kernel.org/r/20240809-explosionsartig-ablesen-b039dbc6ce82@brauner [1]
> Link: https://lore.kernel.org/r/CAHC9VhQY+H7n2zCn8ST0Vu672UA=_eiUikRDW2sUDSN3c=gVQw@mail.gmail.com [2]
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> ---
>
> Changes since v1:
> https://lore.kernel.org/r/20240812144936.1616628-1-mic@digikod.net
> - Add back the file_set_fowner hook (but without user) as
>   requested by Paul, but move it for consistency.
> ---
>  fs/fcntl.c                        | 42 +++++++++++++++----------------
>  fs/file_table.c                   |  3 +++
>  include/linux/fs.h                |  2 +-
>  security/security.c               |  5 +++-
>  security/selinux/hooks.c          | 22 +++-------------
>  security/selinux/include/objsec.h |  1 -
>  security/smack/smack.h            |  6 -----
>  security/smack/smack_lsm.c        | 39 +---------------------------
>  8 files changed, 33 insertions(+), 87 deletions(-)
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 300e5d9ad913..4217b66a4e99 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -87,8 +87,8 @@ static int setfl(int fd, struct file * filp, unsigned int arg)
>         return error;
>  }
>
> -static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
> -                     int force)
> +void __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
> +               int force)
>  {
>         write_lock_irq(&filp->f_owner.lock);
>         if (force || !filp->f_owner.pid) {
> @@ -97,20 +97,15 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
>                 filp->f_owner.pid_type = type;
>
>                 if (pid) {
> -                       const struct cred *cred = current_cred();
> -                       filp->f_owner.uid = cred->uid;
> -                       filp->f_owner.euid = cred->euid;
> +                       security_file_set_fowner(filp);
> +                       put_cred(rcu_replace_pointer(
> +                               filp->f_owner.cred,
> +                               get_cred_rcu(current_cred()),
> +                               lockdep_is_held(&filp->f_owner.lock)));
>                 }
>         }
>         write_unlock_irq(&filp->f_owner.lock);
>  }

Looking at this quickly, why can't we accomplish pretty much the same
thing by moving the security_file_set_fowner() into f_modown (as
you've done above) and leveraging the existing file->f_security field
as Smack and SELinux do today?  I'm seeing a lot of churn to get a
cred pointer into fown_struct which doesn't seem to offer that much
additional value.

>From what I can see this seems really focused on adding a cred
reference when it isn't clear an additional one is needed.  If a new
cred reference *is* needed, please provide an explanation as to why;
reading the commit description this isn't clear.  Of course, if I'm
mistaken, feel free to correct me, although I'm sure all the people on
the Internet don't need to be told that ;)

-- 
paul-moore.com

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ