linux-kernel - Re: [PATCH v2 04/10] ovl: make ioctl() safe

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: <CAOQ4uxhv+33nVxNQmZtf-uzZN0gMXBaDoiJYm88cWwa1fRQTTg@mail.gmail.com>
Date:   Tue, 8 Dec 2020 13:11:19 +0200
From:   Amir Goldstein <amir73il@...il.com>
To:     Miklos Szeredi <mszeredi@...hat.com>
Cc:     "Eric W . Biederman" <ebiederm@...ssion.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        overlayfs <linux-unionfs@...r.kernel.org>,
        LSM List <linux-security-module@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [PATCH v2 04/10] ovl: make ioctl() safe

On Mon, Dec 7, 2020 at 6:36 PM Miklos Szeredi <mszeredi@...hat.com> wrote:
>
> ovl_ioctl_set_flags() does a capability check using flags, but then the
> real ioctl double-fetches flags and uses potentially different value.
>
> The "Check the capability before cred override" comment misleading: user
> can skip this check by presenting benign flags first and then overwriting
> them to non-benign flags.
>
> Just remove the cred override for now, hoping this doesn't cause a
> regression.
>
> The proper solution is to create a new setxflags i_op (patches are in the
> works).
>
> Xfstests don't show a regression.
>
> Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
> Signed-off-by: Miklos Szeredi <mszeredi@...hat.com>

Looks reasonable

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

> ---
>  fs/overlayfs/file.c | 75 ++-------------------------------------------
>  1 file changed, 3 insertions(+), 72 deletions(-)
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index efccb7c1f9bc..3cd1590f2030 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -541,46 +541,26 @@ static long ovl_real_ioctl(struct file *file, unsigned int cmd,
>                            unsigned long arg)
>  {
>         struct fd real;
> -       const struct cred *old_cred;
>         long ret;
>
>         ret = ovl_real_fdget(file, &real);
>         if (ret)
>                 return ret;
>
> -       old_cred = ovl_override_creds(file_inode(file)->i_sb);
>         ret = security_file_ioctl(real.file, cmd, arg);
>         if (!ret)
>                 ret = vfs_ioctl(real.file, cmd, arg);
> -       revert_creds(old_cred);
>
>         fdput(real);
>
>         return ret;
>  }
>


I wonder if we shouldn't leave a comment behind to explain
that no override is intentional.

I also wonder if "Permission model" sections shouldn't be saying
something about ioctl() (current task checks only)? or we just treat
this is a breakage of the permission model that needs to be fixed?

Thanks,
Amir.