* Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt() @ 2021-05-31 14:59 tianyu zhou 2021-05-31 16:40 ` Al Viro 2021-05-31 17:07 ` Matthew Wilcox 0 siblings, 2 replies; 5+ messages in thread From: tianyu zhou @ 2021-05-31 14:59 UTC (permalink / raw) To: Alexander Viro, linux-fsdevel Hi, function do_remount() in fs/namespace.c checks the CAP_SYS_ADMIN before it calls set_mount_attributes(). -------------------- // fs/namespace.c static int do_remount(struct path *path, int ms_flags, int sb_flags, int mnt_flags, void *data) { .... if (ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) { err = reconfigure_super(fc); if (!err) { lock_mount_hash(); set_mount_attributes(mnt, mnt_flags); // <=== protected function unlock_mount_hash(); } ... } -------------------- However, in another caller of set_mount_attributes(), do_reconfigure_mnt(), I have not found any check for CAP_SYS_ADMIN. So, is there a missing check bug inside do_reconfigure_mnt() ? (which makes it possible for normal user to reach set_mount_attributes()) Thanks! Best regards, Tianyu ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt() 2021-05-31 14:59 Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt() tianyu zhou @ 2021-05-31 16:40 ` Al Viro 2021-05-31 17:07 ` Matthew Wilcox 1 sibling, 0 replies; 5+ messages in thread From: Al Viro @ 2021-05-31 16:40 UTC (permalink / raw) To: tianyu zhou; +Cc: linux-fsdevel On Mon, May 31, 2021 at 10:59:54PM +0800, tianyu zhou wrote: > Hi, function do_remount() in fs/namespace.c checks the CAP_SYS_ADMIN > before it calls set_mount_attributes(). > > -------------------- > // fs/namespace.c > static int do_remount(struct path *path, int ms_flags, int sb_flags, > int mnt_flags, void *data) > { > .... > if (ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) { > err = reconfigure_super(fc); > if (!err) { > lock_mount_hash(); > set_mount_attributes(mnt, mnt_flags); // <=== > protected function > unlock_mount_hash(); > } > ... > } > -------------------- > > However, in another caller of set_mount_attributes(), > do_reconfigure_mnt(), I have not found any check for CAP_SYS_ADMIN. > So, is there a missing check bug inside do_reconfigure_mnt() ? (which > makes it possible for normal user to reach set_mount_attributes()) IDGI. By the same token, there are callers of e.g. memcpy() with CAP_SYS_ADMIN checks upstream of those, as well as those that are called without any such checks whatsoever. The answer to such observation would obviously be "what of that?" and I really wonder what your criteria are. For another example, in the same function you have lock_mount_hash() calls as well; are you going to report the calls of that made without CAP_SYS_ADMIN? IOW, what are the heuristics you are using to select the functions you deem suspicious? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt() 2021-05-31 14:59 Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt() tianyu zhou 2021-05-31 16:40 ` Al Viro @ 2021-05-31 17:07 ` Matthew Wilcox 2021-05-31 19:02 ` tianyu zhou 1 sibling, 1 reply; 5+ messages in thread From: Matthew Wilcox @ 2021-05-31 17:07 UTC (permalink / raw) To: tianyu zhou; +Cc: Alexander Viro, linux-fsdevel On Mon, May 31, 2021 at 10:59:54PM +0800, tianyu zhou wrote: > Hi, function do_remount() in fs/namespace.c checks the CAP_SYS_ADMIN > before it calls set_mount_attributes(). > > However, in another caller of set_mount_attributes(), > do_reconfigure_mnt(), I have not found any check for CAP_SYS_ADMIN. > So, is there a missing check bug inside do_reconfigure_mnt() ? (which > makes it possible for normal user to reach set_mount_attributes()) You weren't looking hard enough ... path_mount() if (!may_mount()) return -EPERM; ... if ((flags & (MS_REMOUNT | MS_BIND)) == (MS_REMOUNT | MS_BIND)) return do_reconfigure_mnt(path, mnt_flags); (this is the only call to do_reconfigure_mnt()) and: static inline bool may_mount(void) { return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN); } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt() 2021-05-31 17:07 ` Matthew Wilcox @ 2021-05-31 19:02 ` tianyu zhou 2021-06-01 8:45 ` Christian Brauner 0 siblings, 1 reply; 5+ messages in thread From: tianyu zhou @ 2021-05-31 19:02 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Alexander Viro, linux-fsdevel Thanks for reminding! But here is one more question about the CAP_SYS_ADMIN check inside may_mount(): why does it check the CAP in current->nsproxy->mnt_ns->user_ns? for do_remount(), it checks CAP_SYS_ADMIN in path->mnt->mnt_sb->s_user_ns; for path_mount(), it checks CAP_SYS_ADMIN in current->nsproxy->mnt_ns->user_ns. Is these two user ns are same during runtime? (If they are same, then it will be redundant check in path_mount() and its callee do_remount(); if they are not same, maybe do_reconfigure_mnt() need more check for path->mnt->mnt_sb->s_user_ns) Tianyu Matthew Wilcox <willy@infradead.org> 于2021年6月1日周二 上午1:07写道: > > On Mon, May 31, 2021 at 10:59:54PM +0800, tianyu zhou wrote: > > Hi, function do_remount() in fs/namespace.c checks the CAP_SYS_ADMIN > > before it calls set_mount_attributes(). > > > > However, in another caller of set_mount_attributes(), > > do_reconfigure_mnt(), I have not found any check for CAP_SYS_ADMIN. > > So, is there a missing check bug inside do_reconfigure_mnt() ? (which > > makes it possible for normal user to reach set_mount_attributes()) > > You weren't looking hard enough ... > > path_mount() > if (!may_mount()) > return -EPERM; > ... > if ((flags & (MS_REMOUNT | MS_BIND)) == (MS_REMOUNT | MS_BIND)) > return do_reconfigure_mnt(path, mnt_flags); > > (this is the only call to do_reconfigure_mnt()) > > and: > > static inline bool may_mount(void) > { > return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN); > } > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt() 2021-05-31 19:02 ` tianyu zhou @ 2021-06-01 8:45 ` Christian Brauner 0 siblings, 0 replies; 5+ messages in thread From: Christian Brauner @ 2021-06-01 8:45 UTC (permalink / raw) To: tianyu zhou; +Cc: Matthew Wilcox, Alexander Viro, linux-fsdevel On Tue, Jun 01, 2021 at 03:02:42AM +0800, tianyu zhou wrote: > Thanks for reminding! > > But here is one more question about the CAP_SYS_ADMIN check inside > may_mount(): why does it check the CAP in > current->nsproxy->mnt_ns->user_ns? Because the caller needs to be privileged enough to create mounts in that mount namespace. That's the first requirement. You don't want a caller creating mounts or altering mounts even though someone explicitly wanted to prevent them from doing that. Or from someone creating a new user namespace mapping their own uid and gaining cap_sys_admin in the userns and then starting to change mount options for everyone in that mount namespace. > > for do_remount(), it checks CAP_SYS_ADMIN in path->mnt->mnt_sb->s_user_ns; > for path_mount(), it checks CAP_SYS_ADMIN in current->nsproxy->mnt_ns->user_ns. You can't reach do_remount() other via path_mount(). So you get: 1. may_mount() 2. check_mnt() which translates to: 1. Is the caller allowed to create/destroy etc. mounts in that mnt namespace? 2. Is the caller in the same mnt namespace that the mount is visible in they try to operate on? The it comes down to the question whether the caller is trying to change an option that affects the superblock, i.e. _all places where that filesystem is visible_ or whether they try to change mount specific options, i.e. options that only affect a specific mount. For do_remount() it affects the whole filesystem so you need to check whether the caller is privileged over the filesystem itself, i.e. whether they are capable in the s_user_ns of the filesystem. That should also answer the next question: > > Is these two user ns are same during runtime? (If they are same, then No they don't need to be the same. Take a look at in do_remount(): fc->oldapi = true; err = parse_monolithic_mount_data(fc, data); if (!err) { down_write(&sb->s_umount); err = -EPERM; if (ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) { ^^ Here is were we check that the caller is allowed to alter the superblock. So if you end up there you have checked 3 things so far: [x] Allowed to mount in that mnt namespace [x] Located in the same mnt namespace as the mount [x] Privileged wrt to the superblock > it will be redundant check in path_mount() and its callee > do_remount(); if they are not same, maybe do_reconfigure_mnt() need No, do_reconfigure_mnt() affects mount specific options, not the superblock. So it's enough to know: [x] Allowed to mount in that mnt namespace [x] Located in the same mnt namespace as the mount > more check for path->mnt->mnt_sb->s_user_ns) > > Tianyu > > Matthew Wilcox <willy@infradead.org> 于2021年6月1日周二 上午1:07写道: > > > > On Mon, May 31, 2021 at 10:59:54PM +0800, tianyu zhou wrote: > > > Hi, function do_remount() in fs/namespace.c checks the CAP_SYS_ADMIN > > > before it calls set_mount_attributes(). > > > > > > However, in another caller of set_mount_attributes(), > > > do_reconfigure_mnt(), I have not found any check for CAP_SYS_ADMIN. > > > So, is there a missing check bug inside do_reconfigure_mnt() ? (which > > > makes it possible for normal user to reach set_mount_attributes()) > > > > You weren't looking hard enough ... > > > > path_mount() > > if (!may_mount()) > > return -EPERM; > > ... > > if ((flags & (MS_REMOUNT | MS_BIND)) == (MS_REMOUNT | MS_BIND)) > > return do_reconfigure_mnt(path, mnt_flags); > > > > (this is the only call to do_reconfigure_mnt()) > > > > and: > > > > static inline bool may_mount(void) > > { > > return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN); > > } > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-01 8:45 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-31 14:59 Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt() tianyu zhou 2021-05-31 16:40 ` Al Viro 2021-05-31 17:07 ` Matthew Wilcox 2021-05-31 19:02 ` tianyu zhou 2021-06-01 8:45 ` Christian Brauner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.