diff mbox series

[v2,03/10] ovl: check privs before decoding file handle

Message ID 20201207163255.564116-4-mszeredi@redhat.com (mailing list archive)
State New
Headers show
Series allow unprivileged overlay mounts | expand

Commit Message

Miklos Szeredi Dec. 7, 2020, 4:32 p.m. UTC
CAP_DAC_READ_SEARCH is required by open_by_handle_at(2) so check it in
ovl_decode_real_fh() as well to prevent privilege escalation for
unprivileged overlay mounts.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/namei.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Amir Goldstein Dec. 8, 2020, 1:49 p.m. UTC | #1
On Mon, Dec 7, 2020 at 6:36 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> CAP_DAC_READ_SEARCH is required by open_by_handle_at(2) so check it in
> ovl_decode_real_fh() as well to prevent privilege escalation for
> unprivileged overlay mounts.
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/namei.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index a6162c4076db..82a55fdb1e7a 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -156,6 +156,9 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
>         struct dentry *real;
>         int bytes;
>
> +       if (!capable(CAP_DAC_READ_SEARCH))
> +               return NULL;
> +

If the mounter is not capable in init ns, ovl_check_origin() and
ovl_verify_index()
will not function as expected and this will break index and nfs export features.
So I think we need to also check capability in ovl_can_decode_fh(), to auto
disable those features.

Thanks,
Amir.
Miklos Szeredi Dec. 9, 2020, 10:13 a.m. UTC | #2
On Tue, Dec 8, 2020 at 2:53 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Dec 7, 2020 at 6:36 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
> >
> > CAP_DAC_READ_SEARCH is required by open_by_handle_at(2) so check it in
> > ovl_decode_real_fh() as well to prevent privilege escalation for
> > unprivileged overlay mounts.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >  fs/overlayfs/namei.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index a6162c4076db..82a55fdb1e7a 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -156,6 +156,9 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
> >         struct dentry *real;
> >         int bytes;
> >
> > +       if (!capable(CAP_DAC_READ_SEARCH))
> > +               return NULL;
> > +
>
> If the mounter is not capable in init ns, ovl_check_origin() and
> ovl_verify_index()
> will not function as expected and this will break index and nfs export features.

NFS export is clear-cut.

Hard link indexing should work without fh decoding, since it is only
encoding the file handle to search for the index entry, and encoding
is not privileged.

Not sure how ovl_verify_index will choke on that, will have to try...
but worse case we just need to disable verification.

And yeah, using .overlay.origin attribute for inode number consistency
won't work either, but it should fail silently (which is probably a
good thing).  Haven't tested this yet, though.

Thanks,
Miklos
Miklos Szeredi Dec. 9, 2020, 4:20 p.m. UTC | #3
On Wed, Dec 9, 2020 at 11:13 AM Miklos Szeredi <miklos@szeredi.hu> wrote:

> Hard link indexing should work without fh decoding, since it is only
> encoding the file handle to search for the index entry, and encoding
> is not privileged.

Tested this a bit and while hard link indexing does work,  inode
lookup is broken since it uses the origin inode as a key (which is not
available) instead of using the origin value directly.  This is
fixable, but needs a fair amount of restructuring, so let's just
postpone this and disable index for now, as you suggested.

Thanks,
Miklos
Amir Goldstein Dec. 9, 2020, 6:16 p.m. UTC | #4
On Wed, Dec 9, 2020 at 6:20 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, Dec 9, 2020 at 11:13 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> > Hard link indexing should work without fh decoding, since it is only
> > encoding the file handle to search for the index entry, and encoding
> > is not privileged.
>
> Tested this a bit and while hard link indexing does work,  inode
> lookup is broken since it uses the origin inode as a key (which is not

Yes, that is what I meant by ovl_check_origin() is broken.

> available) instead of using the origin value directly.  This is
> fixable, but needs a fair amount of restructuring, so let's just

Maybe it also requires on-disk changes.
We should be able to use the origin fh as the key for lower inode,
but we need the lower st_inode for initializing the ovl inode with
correct ino. If we cannot decode lower inode from origin fh, I think
we would need to store the ino in user.overlay.ino on copy up or
maintain redirect, but redirect is not supported either with user ns mount...

> postpone this and disable index for now, as you suggested.
>

Nobody seems to be enabling it anyway :-/

Thanks,
Amir.
diff mbox series

Patch

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index a6162c4076db..82a55fdb1e7a 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -156,6 +156,9 @@  struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
 	struct dentry *real;
 	int bytes;
 
+	if (!capable(CAP_DAC_READ_SEARCH))
+		return NULL;
+
 	/*
 	 * Make sure that the stored uuid matches the uuid of the lower
 	 * layer where file handle will be decoded.