linux-kernel - Re: [PATCH v5 1/3] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode

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] [day] [month] [year] [list]

Date:   Tue, 28 Aug 2018 21:40:10 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Mark Salyzyn <salyzyn@...roid.com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Miklos Szeredi <miklos@...redi.hu>,
        Jonathan Corbet <corbet@....net>,
        Vivek Goyal <vgoyal@...hat.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Stephen Smalley <sds@...ho.nsa.gov>,
        overlayfs <linux-unionfs@...r.kernel.org>,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH v5 1/3] overlayfs: check CAP_DAC_READ_SEARCH before
 issuing exportfs_decode_fh

On Tue, Aug 28, 2018 at 8:44 PM Mark Salyzyn <salyzyn@...roid.com> wrote:
>
> On 08/28/2018 10:34 AM, Amir Goldstein wrote:
> > On Tue, Aug 28, 2018 at 7:53 PM Mark Salyzyn <salyzyn@...roid.com> wrote:
> >> Assumption never checked, should fail if the mounter creds are not
> >> sufficient.
> >>
> >> Signed-off-by: Mark Salyzyn <salyzyn@...roid.com>
> >> Cc: Miklos Szeredi <miklos@...redi.hu>
> >> Cc: Jonathan Corbet <corbet@....net>
> >> Cc: Vivek Goyal <vgoyal@...hat.com>
> >> Cc: Eric W. Biederman <ebiederm@...ssion.com>
> >> Cc: Amir Goldstein <amir73il@...il.com>
> >> Cc: Randy Dunlap <rdunlap@...radead.org>
> >> Cc: Stephen Smalley <sds@...ho.nsa.gov>
> >> Cc: linux-unionfs@...r.kernel.org
> >> Cc: linux-doc@...r.kernel.org
> >> Cc: linux-kernel@...r.kernel.org
> >>
> >> v5:
> >> - dependency of "overlayfs: override_creds=off option bypass creator_cred"
> >> ---
> >>   fs/overlayfs/namei.c | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >>
> >> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> >> index c993dd8db739..84982b6525fb 100644
> >> --- a/fs/overlayfs/namei.c
> >> +++ b/fs/overlayfs/namei.c
> >> @@ -193,6 +193,11 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
> >>          if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
> >>                  return NULL;
> >>
> >> +       if (!capable(CAP_DAC_READ_SEARCH)) {
> >> +               origin = ERR_PTR(-EPERM);
> >> +               goto out;
> > Which branch is this works based on?
> > I don't see any out label in current code.
>
> <sigh> I can only truly test this on 4.14 (android's current top of
> tree) and on Hikey with that. Lack of due diligence for Top of Linux.

Well, not sure how that review is going to work out.
anyway, this case should not return an error.
returning NULL should be just fine.

> >
> >> +       }
> >> +
> >>          bytes = (fh->len - offsetof(struct ovl_fh, fid));
> >>          real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
> >>                                    bytes >> 2, (int)fh->type,
> >> --
> > Please add same test in ovl_can_decode_fh().
>
> Ahhhh
> > Problem: none of the ovl_export_operations functions override creds.
> > I guess things are working now because nfsd is privileged enough.
> > IOW, the capability check you added doesn't check mounter creds
> > when coming from nfs export ops - I guess that is not what you want
> > although you probably don'r enable nfs export.
> NFS export/import blocked on Android devices.
> > Thanks,
> > Amir.
>
>