| Message ID | 20201207163255.564116-4-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | allow unprivileged overlay mounts | expand |
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.
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
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
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 --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.
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(+)