overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

PATCH In review Mark Salyzyn 2 participants 4 messages Aug 28, 2018

Subsystems (2)

Documentation Overlay filesystem

lore

Share & tools

lore

Mark Salyzyn

author

2018-08-28 16:52 UTC #0 lore raw

PATCH v5 1/3 overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

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]

v5:
- dependency of "overlayfs: override_creds=off option bypass creator_cred"

---
 fs/overlayfs/namei.c | 5 +++++
 1 file changed, 5 insertions(+)

fs/overlayfs/namei.c +5 -0

index c993dd8db739..84982b6525fb 100644
@@ -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,

Signature · 1 line

2.19.0.rc0.228.g281dcd1b4d0-goog

Amir Goldstein

maintainer

2018-08-28 17:32 UTC #1 lore raw

Quoted 27 lines · Mark Salyzyn

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(+)
fs/overlayfs/namei.c +0 -0
index c993dd8db739..84982b6525fb 100644

fs/overlayfs/namei.c struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt, · line 198

Quoted 6 lines

fs/overlayfs/namei.c +3 -0

@@ -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.

fs/overlayfs/namei.c struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,

Quoted 6 lines

fs/overlayfs/namei.c +2 -1

+       }
+
        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.

Mark Salyzyn

author

2018-08-28 17:44 UTC #2 lore raw

Quoted 30 lines · Amir Goldstein

Quoted 29 lines · Mark Salyzyn
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(+)
fs/overlayfs/namei.c +0 -0
index c993dd8db739..84982b6525fb 100644
@@ -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;
 

Quoted 5 lines

fs/overlayfs/namei.c struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt, · line 198
Quoted 3 lines
fs/overlayfs/namei.c +3 -0
+       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.

Quoted 8 lines

fs/overlayfs/namei.c struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
Quoted 7 lines
fs/overlayfs/namei.c +2 -1
+       }
+
         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

Quoted 5 lines

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.

Quoted 2 lines

Thanks,
Amir.

Amir Goldstein

maintainer

2018-08-28 18:40 UTC #3 lore raw

Quoted 32 lines · Mark Salyzyn

Quoted 30 lines · Amir Goldstein
Quoted 29 lines · Mark Salyzyn
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(+)
fs/overlayfs/namei.c +0 -0
index c993dd8db739..84982b6525fb 100644
@@ -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;
 

Quoted 8 lines

Quoted 5 lines
fs/overlayfs/namei.c struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt, · line 198
Quoted 3 lines
fs/overlayfs/namei.c +3 -0
+       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.

Quoted 20 lines

Quoted 8 lines
fs/overlayfs/namei.c struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
Quoted 7 lines
fs/overlayfs/namei.c +2 -1
+       }
+
         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
Quoted 5 lines
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.
Quoted 2 lines
Thanks,
Amir.