linux-kernel - Re: [PATCH 2/2] security.capability: fix conversions on getxattr

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: <CAJfpegt=qKzyu76b_vNF5_Be2-1dovZ6t06=haVgtC8sq1qsbA@mail.gmail.com>
Date:   Wed, 20 Jan 2021 08:58:53 +0100
From:   Miklos Szeredi <miklos@...redi.hu>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Miklos Szeredi <mszeredi@...hat.com>,
        linux-fsdevel@...r.kernel.org,
        overlayfs <linux-unionfs@...r.kernel.org>,
        LSM <linux-security-module@...r.kernel.org>,
        linux-kernel@...r.kernel.org, "Serge E . Hallyn" <serge@...lyn.com>
Subject: Re: [PATCH 2/2] security.capability: fix conversions on getxattr

On Wed, Jan 20, 2021 at 2:39 AM Eric W. Biederman <ebiederm@...ssion.com> wrote:
>
> Miklos Szeredi <mszeredi@...hat.com> writes:
>
> > If a capability is stored on disk in v2 format cap_inode_getsecurity() will
> > currently return in v2 format unconditionally.
> >
> > This is wrong: v2 cap should be equivalent to a v3 cap with zero rootid,
> > and so the same conversions performed on it.
> >
> > If the rootid cannot be mapped v3 is returned unconverted.  Fix this so
> > that both v2 and v3 return -EOVERFLOW if the rootid (or the owner of the fs
> > user namespace in case of v2) cannot be mapped in the current user
> > namespace.
>
> This looks like a good cleanup.
>
> I do wonder how well this works with stacking.  In particular
> ovl_xattr_set appears to call vfs_getxattr without overriding the creds.
> What the purpose of that is I haven't quite figured out.  It looks like
> it is just a probe to see if an xattr is present so maybe it is ok.

Yeah, it's checking in the removexattr case whether copy-up is needed
or not (i.e. if trying to remove a non-existent xattr, then no need to
copy up).

But for consistency it should also be wrapped in override creds.
Adding fix to this series.

I'll also audit for any remaining omissions.  One known and documented
case is vfs_ioctl(FS_IOC_{[SG]ETFLAGS,FS[SG]ETXATTR}), but that
shouldn't be affected by user namespaces.

Thanks,
Miklos