Assumption never checked, should fail if the mounter creds are not
sufficient.
Signed-off-by: Mark Salyzyn <[email protected]>
Cc: Miklos Szeredi <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Vivek Goyal <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Cc: Amir Goldstein <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Stephen Smalley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
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;
+ }
+
bytes = (fh->len - offsetof(struct ovl_fh, fid));
real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
bytes >> 2, (int)fh->type,
--
2.19.0.rc0.228.g281dcd1b4d0-goog
On Tue, Aug 28, 2018 at 7:53 PM Mark Salyzyn <[email protected]> wrote:
>
> Assumption never checked, should fail if the mounter creds are not
> sufficient.
>
> Signed-off-by: Mark Salyzyn <[email protected]>
> Cc: Miklos Szeredi <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Vivek Goyal <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Cc: Amir Goldstein <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> 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.
> + }
> +
> 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().
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.
Thanks,
Amir.
On 08/28/2018 10:34 AM, Amir Goldstein wrote:
> On Tue, Aug 28, 2018 at 7:53 PM Mark Salyzyn <[email protected]> wrote:
>> Assumption never checked, should fail if the mounter creds are not
>> sufficient.
>>
>> Signed-off-by: Mark Salyzyn <[email protected]>
>> Cc: Miklos Szeredi <[email protected]>
>> Cc: Jonathan Corbet <[email protected]>
>> Cc: Vivek Goyal <[email protected]>
>> Cc: Eric W. Biederman <[email protected]>
>> Cc: Amir Goldstein <[email protected]>
>> Cc: Randy Dunlap <[email protected]>
>> Cc: Stephen Smalley <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>>
>> 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.
>
>> + }
>> +
>> 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.
On Tue, Aug 28, 2018 at 8:44 PM Mark Salyzyn <[email protected]> wrote:
>
> On 08/28/2018 10:34 AM, Amir Goldstein wrote:
> > On Tue, Aug 28, 2018 at 7:53 PM Mark Salyzyn <[email protected]> wrote:
> >> Assumption never checked, should fail if the mounter creds are not
> >> sufficient.
> >>
> >> Signed-off-by: Mark Salyzyn <[email protected]>
> >> Cc: Miklos Szeredi <[email protected]>
> >> Cc: Jonathan Corbet <[email protected]>
> >> Cc: Vivek Goyal <[email protected]>
> >> Cc: Eric W. Biederman <[email protected]>
> >> Cc: Amir Goldstein <[email protected]>
> >> Cc: Randy Dunlap <[email protected]>
> >> Cc: Stephen Smalley <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Cc: [email protected]
> >>
> >> 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.
>
>