linux-kernel - Re: [RFC PATCH] f*xattr: allow O

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]

Date:   Wed, 8 Jun 2022 08:13:07 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Christian Göttsche <cgzones@...glemail.com>
Cc:     selinux@...r.kernel.org, Miklos Szeredi <mszeredi@...hat.com>,
        Linux API <linux-api@...r.kernel.org>,
        linux-man <linux-man@...r.kernel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Alejandro Colomar <alx.manpages@...il.com>
Subject: Re: [RFC PATCH] f*xattr: allow O_PATH descriptors

On Wed, Jun 8, 2022 at 5:23 AM Christian Göttsche
<cgzones@...glemail.com> wrote:
>
> From: Miklos Szeredi <mszeredi@...hat.com>
>
> Support file descriptors obtained via O_PATH for extended attribute
> operations.
>
> Extended attributes are for example used by SELinux for the security
> context of file objects. To avoid time-of-check-time-of-use issues while
> setting those contexts it is advisable to pin the file in question and
> operate on a file descriptor instead of the path name. This can be
> emulated in userspace via /proc/self/fd/NN [1] but requires a procfs,
> which might not be mounted e.g. inside of chroots, see[2].
>
> [1]: https://github.com/SELinuxProject/selinux/commit/7e979b56fd2cee28f647376a7233d2ac2d12ca50
> [2]: https://github.com/SELinuxProject/selinux/commit/de285252a1801397306032e070793889c9466845
>
> Original patch by Miklos Szeredi <mszeredi@...hat.com>
> https://patchwork.kernel.org/project/linux-fsdevel/patch/20200505095915.11275-6-mszeredi@redhat.com/
>
> > While this carries a minute risk of someone relying on the property of
> > xattr syscalls rejecting O_PATH descriptors, it saves the trouble of
> > introducing another set of syscalls.

The bitter irony is that we now want to add another set of syscalls ;-)

https://lore.kernel.org/linux-fsdevel/CAOQ4uxiqG-w8s+zRqk945UtJcE4u0zjPhSs=MSYJ0jMLLjUTFg@mail.gmail.com/

> >
> > Only file->f_path and file->f_inode are accessed in these functions.
> >
> > Current versions return EBADF, hence easy to detect the presense of
> > this feature and fall back in case it's missing.
>
> CC: linux-api@...r.kernel.org
> CC: linux-man@...r.kernel.org
> Signed-off-by: Christian Göttsche <cgzones@...glemail.com>

I think it is important to inspect this with consistency of the UAPI in mind.
What I see is that fchdir(), fcntl(), fstat(), fstatat() already accept O_PATH
so surely they behave the same w.r.t old kernels and EBADF.
Those could all be better documented in their man pages.

w.r.t permission checks, this is no different than what *xattr() variants
already provide.

Therefore, I see no reason to object to this UAPI change.

You may add:
Reviewed-by: Amir Goldstein <amir73il@...il.com>

Thanks,
Amir.

> ---
>  fs/xattr.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index e8dd03e4561e..16360ac4eb1b 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -656,7 +656,7 @@ SYSCALL_DEFINE5(lsetxattr, const char __user *, pathname,
>  SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name,
>                 const void __user *,value, size_t, size, int, flags)
>  {
> -       struct fd f = fdget(fd);
> +       struct fd f = fdget_raw(fd);
>         int error = -EBADF;
>
>         if (!f.file)
> @@ -768,7 +768,7 @@ SYSCALL_DEFINE4(lgetxattr, const char __user *, pathname,
>  SYSCALL_DEFINE4(fgetxattr, int, fd, const char __user *, name,
>                 void __user *, value, size_t, size)
>  {
> -       struct fd f = fdget(fd);
> +       struct fd f = fdget_raw(fd);
>         ssize_t error = -EBADF;
>
>         if (!f.file)
> @@ -844,7 +844,7 @@ SYSCALL_DEFINE3(llistxattr, const char __user *, pathname, char __user *, list,
>
>  SYSCALL_DEFINE3(flistxattr, int, fd, char __user *, list, size_t, size)
>  {
> -       struct fd f = fdget(fd);
> +       struct fd f = fdget_raw(fd);
>         ssize_t error = -EBADF;
>
>         if (!f.file)
> @@ -910,7 +910,7 @@ SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname,
>
>  SYSCALL_DEFINE2(fremovexattr, int, fd, const char __user *, name)
>  {
> -       struct fd f = fdget(fd);
> +       struct fd f = fdget_raw(fd);
>         int error = -EBADF;
>
>         if (!f.file)
> --
> 2.36.1
>