| Message ID | 1469777680-3687-2-git-send-email-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 29, 2016 at 10:34:36AM +0300, Elena Reshetova wrote: > This introduces two new LSM hooks operating on paths. > > - security_path_fchdir() checks for permission on > changing working directory. It can be used by > LSMs concerned on fchdir system call I don't think security_path_fchdir() is a good abstraction level. It neither covers the whole case of "cwd is changed" nor does it cover the whole case of "someone uses a file descriptor to a directory to look up stuff outside that directory". For example, security_path_fchdir() seems to be intended to prevent the use of a leaked file descriptor to the outside world for accessing other files in the outside world. But this is trivially bypassed by first using openat() directly instead of fchdir()+open() (something that used to work against grsecurity, but was fixed quite a while ago).
On Fri, Jul 29, 2016 at 10:34:36AM +0300, Elena Reshetova wrote: > This introduces two new LSM hooks operating on paths. > > - security_path_fchdir() checks for permission on > changing working directory. It can be used by > LSMs concerned on fchdir system call >I don't think security_path_fchdir() is a good abstraction level. It neither covers the whole case of "cwd is changed" nor does it cover the whole case of "someone uses a file descriptor to a directory to look up stuff outside that directory". Do you have a suggestion on what can be a good place? >For example, security_path_fchdir() seems to be intended to prevent the use of a leaked file descriptor to the outside world for accessing other files in the outside world. Yes, this was exactly the use case. >But this is trivially bypassed by first using openat() directly instead of fchdir()+open() (something that used to work against grsecurity, but was fixed quite a while ago). The way it has been addressed in grsecurity is having a check inside filename_lookup() , but it doesn't look a very great place for putting a hook. I was thinking about it , but so far didn't find any other good alternatives.
On Sun, Jul 31, 2016 at 10:55:04AM +0000, Reshetova, Elena wrote: > On Fri, Jul 29, 2016 at 10:34:36AM +0300, Elena Reshetova wrote: > > This introduces two new LSM hooks operating on paths. > > > > - security_path_fchdir() checks for permission on > > changing working directory. It can be used by > > LSMs concerned on fchdir system call > > >I don't think security_path_fchdir() is a good abstraction level. It > neither covers the whole case of "cwd is changed" nor does it cover the > whole case of "someone uses a file descriptor to a directory to look up > stuff outside that directory". > Do you have a suggestion on what can be a good place? > > >For example, security_path_fchdir() seems to be intended to prevent the use > of a leaked file descriptor to the outside world for accessing other files > in the outside world. > Yes, this was exactly the use case. > > >But this is trivially bypassed by first using openat() directly instead of > fchdir()+open() (something that used to work against grsecurity, but was > fixed quite a while ago). > The way it has been addressed in grsecurity is having a check inside > filename_lookup() , but it doesn't look a very great place for putting a > hook. I was thinking about it , but so far didn't find any other good > alternatives. Yeah, if you want to have such a hook, I think it needs to be in filename_lookup() or below - but that's a relatively hot function, so it might have a measurable performance impact. Alternatively, you could forbid double-chroots and use the LSM hooks for file descriptor passing via unix domain sockets and binder to check incoming file descriptors. As long as no directories are moved out of the chroot directory by something outside the chroot (a process chrooted into a parent directory or a process that isn't chrooted), that should prooobably work?
On Sun, Jul 31, 2016 at 10:55:04AM +0000, Reshetova, Elena wrote: > On Fri, Jul 29, 2016 at 10:34:36AM +0300, Elena Reshetova wrote: > > This introduces two new LSM hooks operating on paths. > > > > - security_path_fchdir() checks for permission on > > changing working directory. It can be used by > > LSMs concerned on fchdir system call > > >I don't think security_path_fchdir() is a good abstraction level. It > neither covers the whole case of "cwd is changed" nor does it cover > the whole case of "someone uses a file descriptor to a directory to > look up stuff outside that directory". > Do you have a suggestion on what can be a good place? > > >For example, security_path_fchdir() seems to be intended to prevent > >the use > of a leaked file descriptor to the outside world for accessing other > files in the outside world. > Yes, this was exactly the use case. > > >But this is trivially bypassed by first using openat() directly > >instead of > fchdir()+open() (something that used to work against grsecurity, but > was fixed quite a while ago). > The way it has been addressed in grsecurity is having a check inside > filename_lookup() , but it doesn't look a very great place for putting > a hook. I was thinking about it , but so far didn't find any other > good alternatives. >Yeah, if you want to have such a hook, I think it needs to be in >filename_lookup() or below - but that's a relatively hot function, so it might have a measurable performance impact. Yes, it is way too much used from everywhere and doesn't even fit into LSM design... >Alternatively, you could forbid double-chroots and use the LSM hooks for file descriptor passing via unix domain sockets and binder to check incoming file descriptors. This would not prevent guessing the file descriptor unfortunately. I was planning to continue on chroot features and like grsecurity have an option to disable unix domain sockets outside of chroot. Binder is a different story since it is Android specific and we don't expect it to be used on normal Linux system. And on Android nobody uses chroot, so I don't think there is a use case for this. >As long as no directories are moved out of the chroot directory by something outside the chroot (a process chrooted into a parent directory or a process that isn't chrooted), that should prooobably work? Yes, I think this might work for given use case since we are not expecting processes outside of chroot to help exploited process inside chroot to escape. But guessable file descriptor is still an issue (not sure how real is this issue in practice nowadays).
On Sun, Jul 31, 2016 at 06:28:08PM +0000, Reshetova, Elena wrote: > On Sun, Jul 31, 2016 at 10:55:04AM +0000, Reshetova, Elena wrote: [...] > >Alternatively, you could forbid double-chroots and use the LSM hooks for > file descriptor passing via unix domain sockets and binder to check incoming > file descriptors. > > This would not prevent guessing the file descriptor unfortunately. That doesn't make sense to me. Can you elaborate on that, please? How would you "guess" a file descriptor? Are you talking about file descriptors opened before chroot() that have been leaked accidentally? In that case, you could just do on chroot() what SELinux does on a domain transition and replace all dangerous open file descriptors with /dev/null. Or are you concerned about shared file descriptor tables (which really shouldn't happen accidentally, at least when you keep in mind that for this to be an issue, the fs_struct would have to not be shared)?
>On Sun, Jul 31, 2016 at 06:28:08PM +0000, Reshetova, Elena wrote: > On Sun, Jul 31, 2016 at 10:55:04AM +0000, Reshetova, Elena wrote: [...] > >Alternatively, you could forbid double-chroots and use the LSM hooks > >for > file descriptor passing via unix domain sockets and binder to check > incoming file descriptors. > > This would not prevent guessing the file descriptor unfortunately. >That doesn't make sense to me. Can you elaborate on that, please? >How would you "guess" a file descriptor? Are you talking about file descriptors opened before chroot() that have been leaked accidentally? Yes, these ones. Also I guess in general security-wise it is better approach to have a check in a place where descriptor will be attempted to use/resolved vs. trying to make sure you caught all cases where/how process might obtain some. Various IPC, leaked descriptors, some other potential surprises... But I think in this case it might be worth trying to do what you suggest since I don't see good alternatives either. >In that case, you could just do on chroot() what SELinux does on a domain transition and replace all dangerous open file descriptors with /dev/null. I guess this could work, if I can correctly close the ones that are outside of the chroot. I will check how SELinux does it. Thank you for the tip! >Or are you concerned about shared file descriptor tables (which really shouldn't happen accidentally, You mean CLONE_FILES on clone()? If yes, then I am less concerned of this since it really not common as far as I understood for legitimate processes/daemons to be started this way. However, if this case needs to be addressed, it is trickier, you cannot just substitute these ones with /dev/null without breaking the parent also and you would need to check them all, not just opened ones. > at least when you keep in mind that for this to be an issue, the fs_struct would have to not be shared)? What do you mean by the last part? Not sure I understand here...
diff --git a/fs/fhandle.c b/fs/fhandle.c index ca3c3dd..6e8aba5 100644 --- a/fs/fhandle.c +++ b/fs/fhandle.c @@ -8,6 +8,7 @@ #include <linux/fs_struct.h> #include <linux/fsnotify.h> #include <linux/personality.h> +#include <linux/security.h> #include <asm/uaccess.h> #include "internal.h" #include "mount.h" @@ -179,6 +180,10 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh, retval = -EPERM; goto out_err; } + retval = security_path_fhandle(path); + if (retval) + goto out_err; + if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) { retval = -EFAULT; goto out_err; diff --git a/fs/open.c b/fs/open.c index 93ae3cd..9c260d4 100644 --- a/fs/open.c +++ b/fs/open.c @@ -458,6 +458,9 @@ SYSCALL_DEFINE1(fchdir, unsigned int, fd) goto out_putf; error = inode_permission(inode, MAY_EXEC | MAY_CHDIR); + if (error) + goto out_putf; + error = security_path_fchdir(&f.file->f_path); if (!error) set_fs_pwd(current->fs, &f.file->f_path); out_putf: diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 7ae3976..25164b6 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -308,6 +308,14 @@ * Check for permission to change root directory. * @path contains the path structure. * Return 0 if permission is granted. + * @path_fchdir: + * Check for permission to change working directory. + * @path contains the path structure. + * Return 0 if permission is granted. + * @path_fhandle: + * Check for permission to convert handle to path. + * @path contains the path structure. + * Return 0 if permission is granted. * @inode_readlink: * Check the permission to read the symbolic link. * @dentry contains the dentry structure for the file link. @@ -1378,6 +1386,8 @@ union security_list_options { int (*path_chmod)(const struct path *path, umode_t mode); int (*path_chown)(const struct path *path, kuid_t uid, kgid_t gid); int (*path_chroot)(const struct path *path); + int (*path_fchdir)(const struct path *path); + int (*path_fhandle)(const struct path *path); #endif int (*inode_alloc_security)(struct inode *inode); @@ -1668,6 +1678,8 @@ struct security_hook_heads { struct list_head path_chmod; struct list_head path_chown; struct list_head path_chroot; + struct list_head path_fchdir; + struct list_head path_fhandle; #endif struct list_head inode_alloc_security; struct list_head inode_free_security; diff --git a/include/linux/security.h b/include/linux/security.h index 14df373..6745c06 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -1472,6 +1472,9 @@ int security_path_rename(const struct path *old_dir, struct dentry *old_dentry, int security_path_chmod(const struct path *path, umode_t mode); int security_path_chown(const struct path *path, kuid_t uid, kgid_t gid); int security_path_chroot(const struct path *path); +int security_path_fchdir(const struct path *path); +int security_path_fhandle(const struct path *path); + #else /* CONFIG_SECURITY_PATH */ static inline int security_path_unlink(const struct path *dir, struct dentry *dentry) { @@ -1536,6 +1539,16 @@ static inline int security_path_chroot(const struct path *path) { return 0; } + +static inline int security_path_fchdir(const struct path *path) +{ + return 0; +} + +static inline int security_path_fhandle(const struct path *path) +{ + return 0; +} #endif /* CONFIG_SECURITY_PATH */ #ifdef CONFIG_KEYS diff --git a/security/security.c b/security/security.c index 7095693..cd82276 100644 --- a/security/security.c +++ b/security/security.c @@ -504,6 +504,15 @@ int security_path_chroot(const struct path *path) { return call_int_hook(path_chroot, 0, path); } + +int security_path_fchdir(const struct path *path) +{ + return call_int_hook(path_fchdir, 0, path); +} +int security_path_fhandle(const struct path *path) +{ + return call_int_hook(path_fhandle, 0, path); +} #endif int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode) @@ -1615,6 +1624,8 @@ struct security_hook_heads security_hook_heads = { .path_chmod = LIST_HEAD_INIT(security_hook_heads.path_chmod), .path_chown = LIST_HEAD_INIT(security_hook_heads.path_chown), .path_chroot = LIST_HEAD_INIT(security_hook_heads.path_chroot), + .path_fchdir = LIST_HEAD_INIT(security_hook_heads.path_fchdir), + .path_fhandle = LIST_HEAD_INIT(security_hook_heads.path_fhandle), #endif .inode_alloc_security = LIST_HEAD_INIT(security_hook_heads.inode_alloc_security),
This introduces two new LSM hooks operating on paths. - security_path_fchdir() checks for permission on changing working directory. It can be used by LSMs concerned on fchdir system call - security_path_fhandle() checks for permission before converting file handle to path. It can be used by LSMs concerned with file handle transfers Both hooks are under CONFIG_SECURITY_PATH. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> --- fs/fhandle.c | 5 +++++ fs/open.c | 3 +++ include/linux/lsm_hooks.h | 12 ++++++++++++ include/linux/security.h | 13 +++++++++++++ security/security.c | 11 +++++++++++ 5 files changed, 44 insertions(+)