| Message ID | 20170511175511.GC4519@birch.djwong.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hi, > There were a number of handwaving complaints that one could "possibly" > use inode numbers and extent maps to fingerprint a filesystem hosting > multiple containers and somehow use the information to guess at the > contents of other containers and attack them. Despite the total lack of > any demonstration that this is actually possible, it's easier to > restrict access now and broaden it later, so use the rmapbt fsmap > backends only if the caller has CAP_SYS_ADMIN. Unprivileged users will > just have to make do with only getting the free space information. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_fsmap.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c > index 3683819..814ed729 100644 > --- a/fs/xfs/xfs_fsmap.c > +++ b/fs/xfs/xfs_fsmap.c > @@ -828,6 +828,7 @@ xfs_getfsmap( > struct xfs_fsmap dkeys[2]; /* per-dev keys */ > struct xfs_getfsmap_dev handlers[XFS_GETFSMAP_DEVS]; > struct xfs_getfsmap_info info = { NULL }; > + bool use_rmap; > int i; > int error = 0; > > @@ -837,12 +838,14 @@ xfs_getfsmap( > !xfs_getfsmap_is_valid_device(mp, &head->fmh_keys[1])) > return -EINVAL; > > + use_rmap = capable(CAP_SYS_ADMIN) && > + xfs_sb_version_hasrmapbt(&mp->m_sb); > head->fmh_entries = 0; > > /* Set up our device handlers. */ > memset(handlers, 0, sizeof(handlers)); > handlers[0].dev = new_encode_dev(mp->m_ddev_targp->bt_dev); > - if (xfs_sb_version_hasrmapbt(&mp->m_sb)) > + if (use_rmap) > handlers[0].fn = xfs_getfsmap_datadev_rmapbt; > else > handlers[0].fn = xfs_getfsmap_datadev_bnobt; > I've followed the discussion too, and alhtough it just look as a very remote theoretical problem, restricting this is simple. Overall this looks fine to me, but, I just wonder if somebody in the future will complain to be getting freespace only, instead of the fsmap itself, without actually getting a permission denied error, or something like that. This is a minor concern from my side though, so, unless somebody agrees with my point here, you can add: Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com> Cheers. -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 12, 2017 at 10:53:57AM +0200, Carlos Maiolino wrote: > Hi, > > > There were a number of handwaving complaints that one could "possibly" > > use inode numbers and extent maps to fingerprint a filesystem hosting > > multiple containers and somehow use the information to guess at the > > contents of other containers and attack them. Despite the total lack of > > any demonstration that this is actually possible, it's easier to > > restrict access now and broaden it later, so use the rmapbt fsmap > > backends only if the caller has CAP_SYS_ADMIN. Unprivileged users will > > just have to make do with only getting the free space information. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/xfs_fsmap.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c > > index 3683819..814ed729 100644 > > --- a/fs/xfs/xfs_fsmap.c > > +++ b/fs/xfs/xfs_fsmap.c > > @@ -828,6 +828,7 @@ xfs_getfsmap( > > struct xfs_fsmap dkeys[2]; /* per-dev keys */ > > struct xfs_getfsmap_dev handlers[XFS_GETFSMAP_DEVS]; > > struct xfs_getfsmap_info info = { NULL }; > > + bool use_rmap; > > int i; > > int error = 0; > > > > @@ -837,12 +838,14 @@ xfs_getfsmap( > > !xfs_getfsmap_is_valid_device(mp, &head->fmh_keys[1])) > > return -EINVAL; > > > > + use_rmap = capable(CAP_SYS_ADMIN) && > > + xfs_sb_version_hasrmapbt(&mp->m_sb); > > head->fmh_entries = 0; > > > > /* Set up our device handlers. */ > > memset(handlers, 0, sizeof(handlers)); > > handlers[0].dev = new_encode_dev(mp->m_ddev_targp->bt_dev); > > - if (xfs_sb_version_hasrmapbt(&mp->m_sb)) > > + if (use_rmap) > > handlers[0].fn = xfs_getfsmap_datadev_rmapbt; > > else > > handlers[0].fn = xfs_getfsmap_datadev_bnobt; > > > > I've followed the discussion too, and alhtough it just look as a very remote > theoretical problem, restricting this is simple. > > Overall this looks fine to me, but, I just wonder if somebody in the future will > complain to be getting freespace only, instead of the fsmap itself, without > actually getting a permission denied error, or something like that. > > This is a minor concern from my side though, so, unless somebody agrees with my > point here, you can add: I think I'll just add a blurb to the manpage explicitly stating that filesystems have discretion to reveal as much or as little detail as they like. Therefore, it's perfectly fine not to reveal inode numbers, which is what you'd get with a pre-rmap xfs (or ext4) today. --D > > Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com> > > Cheers. > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Carlos > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 16, 2017 at 10:54:39AM -0700, Darrick J. Wong wrote: > On Fri, May 12, 2017 at 10:53:57AM +0200, Carlos Maiolino wrote: > > Hi, > > > > > There were a number of handwaving complaints that one could "possibly" > > > use inode numbers and extent maps to fingerprint a filesystem hosting > > > multiple containers and somehow use the information to guess at the > > > contents of other containers and attack them. Despite the total lack of > > > any demonstration that this is actually possible, it's easier to > > > restrict access now and broaden it later, so use the rmapbt fsmap > > > backends only if the caller has CAP_SYS_ADMIN. Unprivileged users will > > > just have to make do with only getting the free space information. > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > --- > > > fs/xfs/xfs_fsmap.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c > > > index 3683819..814ed729 100644 > > > --- a/fs/xfs/xfs_fsmap.c > > > +++ b/fs/xfs/xfs_fsmap.c > > > @@ -828,6 +828,7 @@ xfs_getfsmap( > > > struct xfs_fsmap dkeys[2]; /* per-dev keys */ > > > struct xfs_getfsmap_dev handlers[XFS_GETFSMAP_DEVS]; > > > struct xfs_getfsmap_info info = { NULL }; > > > + bool use_rmap; > > > int i; > > > int error = 0; > > > > > > @@ -837,12 +838,14 @@ xfs_getfsmap( > > > !xfs_getfsmap_is_valid_device(mp, &head->fmh_keys[1])) > > > return -EINVAL; > > > > > > + use_rmap = capable(CAP_SYS_ADMIN) && > > > + xfs_sb_version_hasrmapbt(&mp->m_sb); > > > head->fmh_entries = 0; > > > > > > /* Set up our device handlers. */ > > > memset(handlers, 0, sizeof(handlers)); > > > handlers[0].dev = new_encode_dev(mp->m_ddev_targp->bt_dev); > > > - if (xfs_sb_version_hasrmapbt(&mp->m_sb)) > > > + if (use_rmap) > > > handlers[0].fn = xfs_getfsmap_datadev_rmapbt; > > > else > > > handlers[0].fn = xfs_getfsmap_datadev_bnobt; > > > > > > > I've followed the discussion too, and alhtough it just look as a very remote > > theoretical problem, restricting this is simple. > > > > Overall this looks fine to me, but, I just wonder if somebody in the future will > > complain to be getting freespace only, instead of the fsmap itself, without > > actually getting a permission denied error, or something like that. > > > > This is a minor concern from my side though, so, unless somebody agrees with my > > point here, you can add: > > I think I'll just add a blurb to the manpage explicitly stating that > filesystems have discretion to reveal as much or as little detail as > they like. Therefore, it's perfectly fine not to reveal inode numbers, > which is what you'd get with a pre-rmap xfs (or ext4) today. > This sounds perfectly fine to me, you can keep my review tag. Cheers > --D > > > > > Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com> > > > > Cheers. > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > > Carlos > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c index 3683819..814ed729 100644 --- a/fs/xfs/xfs_fsmap.c +++ b/fs/xfs/xfs_fsmap.c @@ -828,6 +828,7 @@ xfs_getfsmap( struct xfs_fsmap dkeys[2]; /* per-dev keys */ struct xfs_getfsmap_dev handlers[XFS_GETFSMAP_DEVS]; struct xfs_getfsmap_info info = { NULL }; + bool use_rmap; int i; int error = 0; @@ -837,12 +838,14 @@ xfs_getfsmap( !xfs_getfsmap_is_valid_device(mp, &head->fmh_keys[1])) return -EINVAL; + use_rmap = capable(CAP_SYS_ADMIN) && + xfs_sb_version_hasrmapbt(&mp->m_sb); head->fmh_entries = 0; /* Set up our device handlers. */ memset(handlers, 0, sizeof(handlers)); handlers[0].dev = new_encode_dev(mp->m_ddev_targp->bt_dev); - if (xfs_sb_version_hasrmapbt(&mp->m_sb)) + if (use_rmap) handlers[0].fn = xfs_getfsmap_datadev_rmapbt; else handlers[0].fn = xfs_getfsmap_datadev_bnobt;
There were a number of handwaving complaints that one could "possibly" use inode numbers and extent maps to fingerprint a filesystem hosting multiple containers and somehow use the information to guess at the contents of other containers and attack them. Despite the total lack of any demonstration that this is actually possible, it's easier to restrict access now and broaden it later, so use the rmapbt fsmap backends only if the caller has CAP_SYS_ADMIN. Unprivileged users will just have to make do with only getting the free space information. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --- fs/xfs/xfs_fsmap.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html