* [kernel-hardening] [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks @ 2016-07-29 7:34 Elena Reshetova 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks Elena Reshetova ` (5 more replies) 0 siblings, 6 replies; 33+ messages in thread From: Elena Reshetova @ 2016-07-29 7:34 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova This introduces a new Hardchroot LSM that is based on GRKERNSEC_CHROOT feature, as well as couple of new LSM hooks needed for its operation. The Hardchroot is implemented as LSM in order to better fit the upstream kernel design, as well as coexist with other security subsystems. The Hardchroot LSM is a minor LSM, and can easily be stacked with other major and minor LSMs. Chroot is still quite widely used in Linux for jailing some system daemons (despite its security limitations), and therefore having an LSM that can be used to make such jails more secure can bring value. For more information about Harchroot and its features, see commit message on Patch 5. In order to implement certain features of Hardchroot, it was necessary to create additional LSM hooks. If there is a way to achieve the same functionality with existing hooks, the implementation can be easily adjusted. Elena Reshetova (5): path_fchdir and path_fhandle LSM hooks task_unshare LSM hook sb_unsharefs LSM hook invoke path_chroot() LSM hook on mntns_install() Hardchroot LSM fs/fhandle.c | 5 + fs/fs_struct.c | 7 +- fs/namespace.c | 15 +- fs/open.c | 3 + include/linux/lsm_hooks.h | 37 ++ include/linux/security.h | 19 + kernel/fork.c | 5 + security/Kconfig | 1 + security/Makefile | 2 + security/hardchroot/Kconfig | 10 + security/hardchroot/Makefile | 3 + security/hardchroot/hardchroot_lsm.c | 654 +++++++++++++++++++++++++++++++++++ security/security.c | 30 ++ 13 files changed, 786 insertions(+), 5 deletions(-) create mode 100644 security/hardchroot/Kconfig create mode 100644 security/hardchroot/Makefile create mode 100644 security/hardchroot/hardchroot_lsm.c -- 1.9.1 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks 2016-07-29 7:34 [kernel-hardening] [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks Elena Reshetova @ 2016-07-29 7:34 ` Elena Reshetova 2016-07-29 18:12 ` Jann Horn 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 2/5] task_unshare LSM hook Elena Reshetova ` (4 subsequent siblings) 5 siblings, 1 reply; 33+ messages in thread From: Elena Reshetova @ 2016-07-29 7:34 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova 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(+) 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), -- 1.9.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks Elena Reshetova @ 2016-07-29 18:12 ` Jann Horn 2016-07-31 10:55 ` Reshetova, Elena 0 siblings, 1 reply; 33+ messages in thread From: Jann Horn @ 2016-07-29 18:12 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova [-- Attachment #1: Type: text/plain, Size: 884 bytes --] 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). [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks 2016-07-29 18:12 ` Jann Horn @ 2016-07-31 10:55 ` Reshetova, Elena 2016-07-31 12:02 ` Jann Horn 0 siblings, 1 reply; 33+ messages in thread From: Reshetova, Elena @ 2016-07-31 10:55 UTC (permalink / raw) To: Jann Horn, kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 1191 bytes --] 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. [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks 2016-07-31 10:55 ` Reshetova, Elena @ 2016-07-31 12:02 ` Jann Horn 2016-07-31 18:28 ` Reshetova, Elena 0 siblings, 1 reply; 33+ messages in thread From: Jann Horn @ 2016-07-31 12:02 UTC (permalink / raw) To: Reshetova, Elena Cc: kernel-hardening, linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 1898 bytes --] 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? [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks 2016-07-31 12:02 ` Jann Horn @ 2016-07-31 18:28 ` Reshetova, Elena 2016-07-31 21:23 ` Jann Horn 0 siblings, 1 reply; 33+ messages in thread From: Reshetova, Elena @ 2016-07-31 18:28 UTC (permalink / raw) To: Jann Horn Cc: kernel-hardening, linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 2623 bytes --] 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). [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks 2016-07-31 18:28 ` Reshetova, Elena @ 2016-07-31 21:23 ` Jann Horn 2016-08-01 8:38 ` Reshetova, Elena 0 siblings, 1 reply; 33+ messages in thread From: Jann Horn @ 2016-07-31 21:23 UTC (permalink / raw) To: Reshetova, Elena Cc: kernel-hardening, linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 970 bytes --] 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)? [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks 2016-07-31 21:23 ` Jann Horn @ 2016-08-01 8:38 ` Reshetova, Elena 0 siblings, 0 replies; 33+ messages in thread From: Reshetova, Elena @ 2016-08-01 8:38 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 1996 bytes --] >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... [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* [kernel-hardening] [RFC] [PATCH 2/5] task_unshare LSM hook 2016-07-29 7:34 [kernel-hardening] [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks Elena Reshetova 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks Elena Reshetova @ 2016-07-29 7:34 ` Elena Reshetova 2016-07-29 17:58 ` Jann Horn 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs " Elena Reshetova ` (3 subsequent siblings) 5 siblings, 1 reply; 33+ messages in thread From: Elena Reshetova @ 2016-07-29 7:34 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova This adds a new security_task_unshare() LSM hook. It can be used by LSMs concerned about unshare system call. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> --- include/linux/lsm_hooks.h | 14 ++++++++++++++ include/linux/security.h | 5 +++++ kernel/fork.c | 5 +++++ security/security.c | 11 +++++++++++ 4 files changed, 35 insertions(+) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 25164b6..e8b839e 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -667,6 +667,14 @@ * security attributes, e.g. for /proc/pid inodes. * @p contains the task_struct for the task. * @inode contains the inode structure for the inode. + * @task_unshare: + * Check if process is allowed to unshare its namespaces + * @unshare_flags flags + * @new_fs contains the new fs_struct if created. + * @new_fd contains the new files_struct if created. + * @new_creds contains the new cred if created. + * @new_nsproxy contains the new nsproxy if created. + * Return 0 if permission is granted. * * Security hooks for Netlink messaging. * @@ -1489,6 +1497,11 @@ union security_list_options { int (*task_prctl)(int option, unsigned long arg2, unsigned long arg3, unsigned long arg4, unsigned long arg5); void (*task_to_inode)(struct task_struct *p, struct inode *inode); + int (*task_unshare)(unsigned long unshare_flags, + const struct fs_struct *new_fs, + const struct files_struct *new_fd, + const struct cred *new_cred, + const struct nsproxy *new_nsproxy); int (*ipc_permission)(struct kern_ipc_perm *ipcp, short flag); void (*ipc_getsecid)(struct kern_ipc_perm *ipcp, u32 *secid); @@ -1748,6 +1761,7 @@ struct security_hook_heads { struct list_head task_wait; struct list_head task_prctl; struct list_head task_to_inode; + struct list_head task_unshare; struct list_head ipc_permission; struct list_head ipc_getsecid; struct list_head msg_msg_alloc_security; diff --git a/include/linux/security.h b/include/linux/security.h index 6745c06..6f935dc 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -331,6 +331,11 @@ int security_task_wait(struct task_struct *p); int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, unsigned long arg4, unsigned long arg5); void security_task_to_inode(struct task_struct *p, struct inode *inode); +int security_task_unshare(unsigned long unshare_flags, + const struct fs_struct *new_fs, + const struct files_struct *new_fd, + const struct cred *new_cred, + const struct nsproxy *new_nsproxy); int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag); void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid); int security_msg_msg_alloc(struct msg_msg *msg); diff --git a/kernel/fork.c b/kernel/fork.c index 4a7ec0c..24cfd66 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2052,6 +2052,11 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags) if (err) goto bad_unshare_cleanup_cred; + err = security_task_unshare(unshare_flags, new_fs, new_fd, + new_cred, new_nsproxy); + if (err) + goto bad_unshare_cleanup_cred; + if (new_fs || new_fd || do_sysvsem || new_cred || new_nsproxy) { if (do_sysvsem) { /* diff --git a/security/security.c b/security/security.c index cd82276..0e9544c 100644 --- a/security/security.c +++ b/security/security.c @@ -1020,6 +1020,16 @@ void security_task_to_inode(struct task_struct *p, struct inode *inode) call_void_hook(task_to_inode, p, inode); } +int security_task_unshare(unsigned long unshare_flags, + const struct fs_struct *new_fs, + const struct files_struct *new_fd, + const struct cred *new_cred, + const struct nsproxy *new_nsproxy) +{ + return call_int_hook(task_unshare, 0, unshare_flags, new_fs, + new_fd, new_cred, new_nsproxy); +} + int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag) { return call_int_hook(ipc_permission, 0, ipcp, flag); @@ -1736,6 +1746,7 @@ struct security_hook_heads security_hook_heads = { .task_prctl = LIST_HEAD_INIT(security_hook_heads.task_prctl), .task_to_inode = LIST_HEAD_INIT(security_hook_heads.task_to_inode), + .task_unshare = LIST_HEAD_INIT(security_hook_heads.task_unshare), .ipc_permission = LIST_HEAD_INIT(security_hook_heads.ipc_permission), .ipc_getsecid = LIST_HEAD_INIT(security_hook_heads.ipc_getsecid), -- 1.9.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 2/5] task_unshare LSM hook 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 2/5] task_unshare LSM hook Elena Reshetova @ 2016-07-29 17:58 ` Jann Horn 2016-07-29 18:17 ` Reshetova, Elena 0 siblings, 1 reply; 33+ messages in thread From: Jann Horn @ 2016-07-29 17:58 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova [-- Attachment #1: Type: text/plain, Size: 781 bytes --] On Fri, Jul 29, 2016 at 10:34:37AM +0300, Elena Reshetova wrote: > This adds a new security_task_unshare() LSM hook. > It can be used by LSMs concerned about unshare > system call. > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > --- [...] > @@ -2052,6 +2052,11 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags) > if (err) > goto bad_unshare_cleanup_cred; > > + err = security_task_unshare(unshare_flags, new_fs, new_fd, > + new_cred, new_nsproxy); > + if (err) > + goto bad_unshare_cleanup_cred; > + > if (new_fs || new_fd || do_sysvsem || new_cred || new_nsproxy) { > if (do_sysvsem) { > /* Why would you have an LSM hook just for the unshare() syscall given that clone() exposes nearly the same functionality? [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [kernel-hardening] [RFC] [PATCH 2/5] task_unshare LSM hook 2016-07-29 17:58 ` Jann Horn @ 2016-07-29 18:17 ` Reshetova, Elena 0 siblings, 0 replies; 33+ messages in thread From: Reshetova, Elena @ 2016-07-29 18:17 UTC (permalink / raw) To: Jann Horn, kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 707 bytes --] > Why would you have an LSM hook just for the unshare() syscall given that clone() exposes nearly the same functionality? My trace of thought was like this: Clone creates new process, so we have two options: - do one more hook here also (or have a joint hook) and then also add the info about this process into the hardchroot info list or - do not add this child process to the list and therefore we don't need updated pointers on fs for it, but just treat it as a child (since it would be chrooted to the same location unless it calls unshare, chroot, pivot_root or similar). I went with the second approach to minimize the hooks changes needed and number of processes to store in internal list. [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs LSM hook 2016-07-29 7:34 [kernel-hardening] [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks Elena Reshetova 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks Elena Reshetova 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 2/5] task_unshare LSM hook Elena Reshetova @ 2016-07-29 7:34 ` Elena Reshetova 2016-07-29 18:02 ` Jann Horn 2016-07-29 18:15 ` Jann Horn 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install() Elena Reshetova ` (2 subsequent siblings) 5 siblings, 2 replies; 33+ messages in thread From: Elena Reshetova @ 2016-07-29 7:34 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova This adds a new security_sb_unsharefs() LSM hook. It can be used by LSMs concerned about unsharefs() system call. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> --- fs/fs_struct.c | 7 ++++++- include/linux/lsm_hooks.h | 6 ++++++ include/linux/security.h | 1 + security/security.c | 7 +++++++ 4 files changed, 20 insertions(+), 1 deletion(-) diff --git a/fs/fs_struct.c b/fs/fs_struct.c index 7dca743..eba0fda 100644 --- a/fs/fs_struct.c +++ b/fs/fs_struct.c @@ -4,6 +4,7 @@ #include <linux/path.h> #include <linux/slab.h> #include <linux/fs_struct.h> +#include <linux/security.h> #include "internal.h" /* @@ -132,11 +133,15 @@ int unshare_fs_struct(void) { struct fs_struct *fs = current->fs; struct fs_struct *new_fs = copy_fs_struct(fs); - int kill; + int kill, retval; if (!new_fs) return -ENOMEM; + retval = security_sb_unsharefs(&new_fs->root); + if (retval) + return retval; + task_lock(current); spin_lock(&fs->lock); kill = !--fs->users; diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index e8b839e..f30cf47 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -143,6 +143,10 @@ * Parse a string of security data filling in the opts structure * @options string containing all mount options known by the LSM * @opts binary data structure usable by the LSM + * @sb_unsharefs: + * Check permission before allowing to unshare fs_struct from process. + * @path contains the path for the new root structure. + * Return 0 if permission is granted. * @dentry_init_security: * Compute a context for a dentry as the inode is not yet available * since NFSv4 has no label backed by an EA anyway. @@ -1371,6 +1375,7 @@ union security_list_options { int (*sb_clone_mnt_opts)(const struct super_block *oldsb, struct super_block *newsb); int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts); + int (*sb_unsharefs)(const struct path *path); int (*dentry_init_security)(struct dentry *dentry, int mode, struct qstr *name, void **ctx, u32 *ctxlen); @@ -1678,6 +1683,7 @@ struct security_hook_heads { struct list_head sb_set_mnt_opts; struct list_head sb_clone_mnt_opts; struct list_head sb_parse_opts_str; + struct list_head sb_unsharefs; struct list_head dentry_init_security; #ifdef CONFIG_SECURITY_PATH struct list_head path_unlink; diff --git a/include/linux/security.h b/include/linux/security.h index 6f935dc..5ad746f 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -239,6 +239,7 @@ int security_sb_set_mnt_opts(struct super_block *sb, int security_sb_clone_mnt_opts(const struct super_block *oldsb, struct super_block *newsb); int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts); +int security_sb_unsharefs(const struct path *path); int security_dentry_init_security(struct dentry *dentry, int mode, struct qstr *name, void **ctx, u32 *ctxlen); diff --git a/security/security.c b/security/security.c index 0e9544c..95487b9 100644 --- a/security/security.c +++ b/security/security.c @@ -343,6 +343,11 @@ int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts) } EXPORT_SYMBOL(security_sb_parse_opts_str); +int security_sb_unsharefs(const struct path *path) +{ + return call_int_hook(sb_unsharefs, 0, path); +} + int security_inode_alloc(struct inode *inode) { inode->i_security = NULL; @@ -1619,6 +1624,8 @@ struct security_hook_heads security_hook_heads = { LIST_HEAD_INIT(security_hook_heads.sb_clone_mnt_opts), .sb_parse_opts_str = LIST_HEAD_INIT(security_hook_heads.sb_parse_opts_str), + .sb_unsharefs = + LIST_HEAD_INIT(security_hook_heads.sb_unsharefs), .dentry_init_security = LIST_HEAD_INIT(security_hook_heads.dentry_init_security), #ifdef CONFIG_SECURITY_PATH -- 1.9.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs LSM hook 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs " Elena Reshetova @ 2016-07-29 18:02 ` Jann Horn 2016-07-29 18:09 ` Reshetova, Elena 2016-07-29 18:15 ` Jann Horn 1 sibling, 1 reply; 33+ messages in thread From: Jann Horn @ 2016-07-29 18:02 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova [-- Attachment #1: Type: text/plain, Size: 388 bytes --] On Fri, Jul 29, 2016 at 10:34:38AM +0300, Elena Reshetova wrote: > This adds a new security_sb_unsharefs() LSM hook. > It can be used by LSMs concerned about unsharefs() > system call. There is no unsharefs() system call. Your patch touches a kernel function unshare_fs_struct() that is called by the NFS server kernel thread and some lustre stuff, which also looks like kernel threads. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs LSM hook 2016-07-29 18:02 ` Jann Horn @ 2016-07-29 18:09 ` Reshetova, Elena 0 siblings, 0 replies; 33+ messages in thread From: Reshetova, Elena @ 2016-07-29 18:09 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 958 bytes --] On Fri, Jul 29, 2016 at 10:34:38AM +0300, Elena Reshetova wrote: > This adds a new security_sb_unsharefs() LSM hook. > It can be used by LSMs concerned about unsharefs() system call. >There is no unsharefs() system call. Your patch touches a kernel function >unshare_fs_struct() that is called by the NFS server kernel thread and some lustre stuff, which also looks like kernel threads. Sorry, wrong wording, it isn't the system call, but it is an exported function: http://lxr.free-electrons.com/source/fs/fs_struct.c#L152 So, in principle it can be used in many other places in future. Yes, currently it is used by NFS server and Lustre, but no guarantees on what is next in line. Or are you saying that that having a check done in this palce doesn't make sense? The reason I thought it is important is that since we need to store the pointer to correct fs root and since it is updated in this case, we don't want to miss this. Best Regards, Elena. [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs LSM hook 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs " Elena Reshetova 2016-07-29 18:02 ` Jann Horn @ 2016-07-29 18:15 ` Jann Horn 2016-07-29 18:19 ` Reshetova, Elena 1 sibling, 1 reply; 33+ messages in thread From: Jann Horn @ 2016-07-29 18:15 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova [-- Attachment #1: Type: text/plain, Size: 724 bytes --] On Fri, Jul 29, 2016 at 10:34:38AM +0300, Elena Reshetova wrote: > This adds a new security_sb_unsharefs() LSM hook. > It can be used by LSMs concerned about unsharefs() > system call. > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > --- [...] > @@ -132,11 +133,15 @@ int unshare_fs_struct(void) > { > struct fs_struct *fs = current->fs; > struct fs_struct *new_fs = copy_fs_struct(fs); > - int kill; > + int kill, retval; > > if (!new_fs) > return -ENOMEM; > > + retval = security_sb_unsharefs(&new_fs->root); > + if (retval) > + return retval; Oh, and this is a memory leak. If copy_fs_struct() succeeds but security_sb_unsharefs() fails, new_fs isn't deallocated. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs LSM hook 2016-07-29 18:15 ` Jann Horn @ 2016-07-29 18:19 ` Reshetova, Elena 0 siblings, 0 replies; 33+ messages in thread From: Reshetova, Elena @ 2016-07-29 18:19 UTC (permalink / raw) To: Jann Horn, kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 848 bytes --] >On Fri, Jul 29, 2016 at 10:34:38AM +0300, Elena Reshetova wrote: > This adds a new security_sb_unsharefs() LSM hook. > It can be used by LSMs concerned about unsharefs() system call. > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > --- [...] > @@ -132,11 +133,15 @@ int unshare_fs_struct(void) { > struct fs_struct *fs = current->fs; > struct fs_struct *new_fs = copy_fs_struct(fs); > - int kill; > + int kill, retval; > > if (!new_fs) > return -ENOMEM; > > + retval = security_sb_unsharefs(&new_fs->root); > + if (retval) > + return retval; >Oh, and this is a memory leak. If copy_fs_struct() succeeds but >security_sb_unsharefs() fails, new_fs isn't deallocated. That's true, thank you, missed this. Will fix. I don't fail security_sb_unsharefs check ever (I use it to update info only), so I missed it fully. [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install() 2016-07-29 7:34 [kernel-hardening] [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks Elena Reshetova ` (2 preceding siblings ...) 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs " Elena Reshetova @ 2016-07-29 7:34 ` Elena Reshetova 2016-07-29 18:11 ` Jann Horn 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM Elena Reshetova 2016-08-03 6:36 ` [kernel-hardening] Re: [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks James Morris 5 siblings, 1 reply; 33+ messages in thread From: Elena Reshetova @ 2016-07-29 7:34 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova This adds an additional invocation of the security_path_chroot LSM hook inside mntns_install(). Currently only capabilities are checked at this point, while process root actually changes. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> --- fs/namespace.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/fs/namespace.c b/fs/namespace.c index 419f746..c5dcb09 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -3325,6 +3325,7 @@ static int mntns_install(struct nsproxy *nsproxy, struct ns_common *ns) struct fs_struct *fs = current->fs; struct mnt_namespace *mnt_ns = to_mnt_ns(ns); struct path root; + int retval; if (!ns_capable(mnt_ns->user_ns, CAP_SYS_ADMIN) || !ns_capable(current_user_ns(), CAP_SYS_CHROOT) || @@ -3334,10 +3335,6 @@ static int mntns_install(struct nsproxy *nsproxy, struct ns_common *ns) if (fs->users != 1) return -EINVAL; - get_mnt_ns(mnt_ns); - put_mnt_ns(nsproxy->mnt_ns); - nsproxy->mnt_ns = mnt_ns; - /* Find the root */ root.mnt = &mnt_ns->root->mnt; root.dentry = mnt_ns->root->mnt.mnt_root; @@ -3345,6 +3342,16 @@ static int mntns_install(struct nsproxy *nsproxy, struct ns_common *ns) while(d_mountpoint(root.dentry) && follow_down_one(&root)) ; + retval = security_path_chroot(&root); + if (retval) { + path_put(&root); + return retval; + } + + get_mnt_ns(mnt_ns); + put_mnt_ns(nsproxy->mnt_ns); + nsproxy->mnt_ns = mnt_ns; + /* Update the pwd and root */ set_fs_pwd(fs, &root); set_fs_root(fs, &root); -- 1.9.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install() 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install() Elena Reshetova @ 2016-07-29 18:11 ` Jann Horn 2016-07-31 10:39 ` Reshetova, Elena 0 siblings, 1 reply; 33+ messages in thread From: Jann Horn @ 2016-07-29 18:11 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova [-- Attachment #1: Type: text/plain, Size: 1185 bytes --] On Fri, Jul 29, 2016 at 10:34:39AM +0300, Elena Reshetova wrote: > This adds an additional invocation of the > security_path_chroot LSM hook inside mntns_install(). > Currently only capabilities are checked at this point, > while process root actually changes. Are you aware that unprivileged user namespace creation doesn't work in a chrooted process? See the invocation of current_chrooted() in create_user_ns(). This means that for this new LSM hook to make any sense, a namespace admin has to attempt to sandbox himself with chroot(). If the current namespace is the init namespace, the process has CAP_SYS_ADMIN in the init namespace, meaning that filesystem sandboxing is probably useless. If the current namespace is not the init namespace, the process probably used namespaces to sandbox itself, in which case it wouldn't be using chroot in the first place, or it is running in a container with admin privileges. In the latter case, this mitigation miiight make a difference, I'm not sure exactly how powerful the APIs for namespace admins are - but a mitigation that only makes a difference inside containers would be weird anyway. So: What is your specific usecase here? [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install() 2016-07-29 18:11 ` Jann Horn @ 2016-07-31 10:39 ` Reshetova, Elena 2016-07-31 11:29 ` Jann Horn 0 siblings, 1 reply; 33+ messages in thread From: Reshetova, Elena @ 2016-07-31 10:39 UTC (permalink / raw) To: Jann Horn, kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 2542 bytes --] On Fri, Jul 29, 2016 at 10:34:39AM +0300, Elena Reshetova wrote: > This adds an additional invocation of the security_path_chroot LSM > hook inside mntns_install(). > Currently only capabilities are checked at this point, while process > root actually changes. >Are you aware that unprivileged user namespace creation doesn't work in a chrooted process? See the invocation of current_chrooted() in create_user_ns(). This means that for this new LSM hook to make any sense, a namespace admin has to attempt >to sandbox himself with chroot(). I am not sure I understand you fully here. It is possible to create new mount namespace without creating new user namespace, and when this happens, if I understand the code right, there is no check like current_chrooted() or smth like this. So, how does it relate to user namespace? >If the current namespace is the init namespace, the process has CAP_SYS_ADMIN in the init namespace, meaning that filesystem sandboxing is probably useless. >If the current namespace is not the init namespace, the process probably used namespaces to sandbox itself, in which case it wouldn't be using chroot in the first place, Why? It is possible that we might be running in the namespace some daemon which just uses chroot by default (legacy or whatever). I think you even proposed that in another email. >r it is running in a container with admin privileges. >In the latter case, this mitigation miiight make a difference, I'm not sure exactly how powerful the APIs for namespace admins are - but a mitigation that only makes a difference inside containers would be weird anyway. >So: What is your specific usecase here? The usecase was similar to the unshare cases: we need to find places where current process's root changes in the code and make sure we catch this change in order to have the up-to-date information about each process's root. So, if we have a process that was put into chroot, but then also creates a mount ns inside? If we recorded the dentry of chrooted process , but then not update it upon mount ns creation, we might end up with wrong entry and all kinds of problems around it. But yes, the whole relationship with namespaces is tricky and there are sooo many edge cases. I will try to create a list of possible combinations and use cases. It is true that this hardening LSM has a particular set of use-cases in mind and there is no need to support all possible combinations. I haven't even tested this at all with namespaces yet, which would be separate fun on its own... [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install() 2016-07-31 10:39 ` Reshetova, Elena @ 2016-07-31 11:29 ` Jann Horn 2016-08-01 9:26 ` Reshetova, Elena 0 siblings, 1 reply; 33+ messages in thread From: Jann Horn @ 2016-07-31 11:29 UTC (permalink / raw) To: Reshetova, Elena Cc: kernel-hardening, linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 1999 bytes --] On Sun, Jul 31, 2016 at 10:39:04AM +0000, Reshetova, Elena wrote: > On Fri, Jul 29, 2016 at 10:34:39AM +0300, Elena Reshetova wrote: > > This adds an additional invocation of the security_path_chroot LSM > > hook inside mntns_install(). > > Currently only capabilities are checked at this point, while process > > root actually changes. > > >Are you aware that unprivileged user namespace creation doesn't work in a > chrooted process? See the invocation of current_chrooted() in > create_user_ns(). This means that for this new LSM hook to make any sense, a > namespace admin has to attempt >to sandbox himself with chroot(). > > I am not sure I understand you fully here. It is possible to create new > mount namespace without creating new user namespace, and when this happens, > if I understand the code right, there is no check like current_chrooted() or > smth like this. > So, how does it relate to user namespace? You can only create a new mount namespace if you're privileged relative to your current namespace. See the second and third invocation of ns_capable() in mntns_install(). To get those privileges, you have to either be privileged already or unshare the user namespace to get new namespaced privileges. An unprivileged, chrooted process doesn't have existing privileges and can't unshare the user namespace to get new ones, so it can't unshare other namespaces (like the mount namespace) anymore. > >If the current namespace is the init namespace, the process has > CAP_SYS_ADMIN in the init namespace, meaning that filesystem sandboxing is > probably useless. > >If the current namespace is not the init namespace, the process probably > used namespaces to sandbox itself, in which case it wouldn't be using chroot > in the first place, > Why? It is possible that we might be running in the namespace some daemon > which just uses chroot by default (legacy or whatever). I think you even > proposed that in another email. True. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install() 2016-07-31 11:29 ` Jann Horn @ 2016-08-01 9:26 ` Reshetova, Elena 0 siblings, 0 replies; 33+ messages in thread From: Reshetova, Elena @ 2016-08-01 9:26 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 2177 bytes --] On Sun, Jul 31, 2016 at 10:39:04AM +0000, Reshetova, Elena wrote: > On Fri, Jul 29, 2016 at 10:34:39AM +0300, Elena Reshetova wrote: > > This adds an additional invocation of the security_path_chroot LSM > > hook inside mntns_install(). > > Currently only capabilities are checked at this point, while process > > root actually changes. > > >Are you aware that unprivileged user namespace creation doesn't work > >in a > chrooted process? See the invocation of current_chrooted() in > create_user_ns(). This means that for this new LSM hook to make any > sense, a namespace admin has to attempt >to sandbox himself with chroot(). > > I am not sure I understand you fully here. It is possible to create > new mount namespace without creating new user namespace, and when this > happens, if I understand the code right, there is no check like > current_chrooted() or smth like this. > So, how does it relate to user namespace? >You can only create a new mount namespace if you're privileged relative to your current namespace. See the second and third invocation of ns_capable() in mntns_install(). To get those privileges, you have to either be privileged already or unshare the user namespace to get new namespaced privileges. An unprivileged, chrooted process doesn't have existing privileges and can't unshare the user namespace to get new ones, so it can't unshare other namespaces (like the mount namespace) anymore. Yes, all true, I just got confused that you said that checks for chroot are done. So, yes, you need to have CAP_SYS_ADMIN in current namespace, and then you can get your new mount ns. After thinking more on this, I think I will get rid of this altogether. Yes, there is a case when a process first enters a new user ns, then enters chroot and forgets to drop its caps. Then it runs for a while, gets exploited and now it tries to get out of chroot. It can create a new mount ns since it has CAP_SYS_ADMIN in his user ns, but what is the outcome? Probably not much, it certainly doesn't help it much from chroot perspective. And I can't think of a reason why a legitimate process would want to create a new namespace inside the chroot.... [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM 2016-07-29 7:34 [kernel-hardening] [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks Elena Reshetova ` (3 preceding siblings ...) 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install() Elena Reshetova @ 2016-07-29 7:34 ` Elena Reshetova 2016-07-29 11:44 ` [kernel-hardening] " Brad Spengler 2016-07-29 18:53 ` [kernel-hardening] " Jann Horn 2016-08-03 6:36 ` [kernel-hardening] Re: [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks James Morris 5 siblings, 2 replies; 33+ messages in thread From: Elena Reshetova @ 2016-07-29 7:34 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova This adds a new Hardchroot LSM that is intended to make classical chroot more secure. It is based on GRKERNSEC_CHROOT feature with necessary changes needed to make it fit inside LSM. Currently not all GRKERNSEC_CHROOT features are supported, but support is planned to be added on granular basis. The credits for feature itself should go to the original authors of GRKERNSEC_CHROOT. Since there is no way to share security metadata between LSMs yet, the Hardchroot info task management is done based on Yama LSM. When support is added, the required info can be stored as part of task struct and it can drastically simplify the internal management. Currently supported features from GRKERNSEC_CHROOT are - GRKERNSEC_CHROOT_MOUNT: prevents process inside chroot from mounting and unmounting filesystems - GRKERNSEC_CHROOT_DOUBLE: prevents process inside chroot from chrooting again outside of current chroot location - GRKERNSEC_CHROOT_PIVOT: prevents process inside chroot from calling pivot_root() system call. - GRKERNSEC_CHROOT_CHDIR: make sure the current working dir of processes inside chroot is set to chroot location. - GRKERNSEC_CHROOT_CHMOD: prevents process inside chroot from executing chmod or fchmod on files to make them have suid or sgid bits. - GRKERNSEC_CHROOT_FCHDIR: prevent process inside chroot from fchdir to a file outside of chroot location. Also prevents opening a file by a file descriptor located outside of chroot. - GRKERNSEC_CHROOT_MKNOD: prevents process inside chroot from making new device nodes. - GRKERNSEC_CHROOT_SHMAT: prevents process inside chroot from attaching to shared memory segments that were created outside of chroot. - GRKERNSEC_CHROOT_NICE: prevents process inside chroot from raising priority of processes inside or outside of chroot. Note: this feature currently prevents rtkit daemon from normal operation. Also original GRKERNSEC_CHROOT_NICE feature did not allowed processes inside chroot to make any priority changes to processed outside chroot. The same behavior can be enforced also, but it breaks rtkit daemon even more. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> --- include/linux/lsm_hooks.h | 5 + security/Kconfig | 1 + security/Makefile | 2 + security/hardchroot/Kconfig | 10 + security/hardchroot/Makefile | 3 + security/hardchroot/hardchroot_lsm.c | 654 +++++++++++++++++++++++++++++++++++ security/security.c | 1 + 7 files changed, 676 insertions(+) create mode 100644 security/hardchroot/Kconfig create mode 100644 security/hardchroot/Makefile create mode 100644 security/hardchroot/hardchroot_lsm.c diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index f30cf47..c87fcf7 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1930,5 +1930,10 @@ void __init loadpin_add_hooks(void); #else static inline void loadpin_add_hooks(void) { }; #endif +#ifdef CONFIG_SECURITY_HARDCHROOT +void __init hardchroot_add_hooks(void); +#else +static inline void hardchroot_add_hooks(void) { }; +#endif #endif /* ! __LINUX_LSM_HOOKS_H */ diff --git a/security/Kconfig b/security/Kconfig index 176758c..cd7821d 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -124,6 +124,7 @@ source security/tomoyo/Kconfig source security/apparmor/Kconfig source security/loadpin/Kconfig source security/yama/Kconfig +source security/hardchroot/Kconfig source security/integrity/Kconfig diff --git a/security/Makefile b/security/Makefile index f2d71cd..b102b60 100644 --- a/security/Makefile +++ b/security/Makefile @@ -9,6 +9,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO) += tomoyo subdir-$(CONFIG_SECURITY_APPARMOR) += apparmor subdir-$(CONFIG_SECURITY_YAMA) += yama subdir-$(CONFIG_SECURITY_LOADPIN) += loadpin +subdir-$(CONFIG_SECURITY_HARDCHROOT)+= hardchroot # always enable default capabilities obj-y += commoncap.o @@ -25,6 +26,7 @@ obj-$(CONFIG_SECURITY_APPARMOR) += apparmor/ obj-$(CONFIG_SECURITY_YAMA) += yama/ obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/ obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o +obj-$(CONFIG_SECURITY_HARDCHROOT) += hardchroot/ # Object integrity file lists subdir-$(CONFIG_INTEGRITY) += integrity diff --git a/security/hardchroot/Kconfig b/security/hardchroot/Kconfig new file mode 100644 index 0000000..a20d758 --- /dev/null +++ b/security/hardchroot/Kconfig @@ -0,0 +1,10 @@ +config SECURITY_HARDCHROOT + bool "Hardchroot support" + depends on SECURITY + select SECURITY_PATH + default n + help + This selects Hardchroot LSM, which makes a traditional Linux chroot + environment stronger. Like capabilities, this security module + stacks with other LSMs. + If you are unsure how to answer this question, answer N. diff --git a/security/hardchroot/Makefile b/security/hardchroot/Makefile new file mode 100644 index 0000000..b03461b --- /dev/null +++ b/security/hardchroot/Makefile @@ -0,0 +1,3 @@ +obj-$(CONFIG_SECURITY_HARDCHROOT) := hardchroot.o + +hardchroot-y := hardchroot_lsm.o diff --git a/security/hardchroot/hardchroot_lsm.c b/security/hardchroot/hardchroot_lsm.c new file mode 100644 index 0000000..01aa95c --- /dev/null +++ b/security/hardchroot/hardchroot_lsm.c @@ -0,0 +1,654 @@ +/* + * Hardchroot Linux Security Module + * + * Author: Elena Reshetova <elena.reshetova@intel.com> + * + * Done based on GRSECURITY_CHROOT feature. + * + * Management of task-related chroot information + * is done based on Yama ptrace relationship management + * + * Copyright (c) 2016, Intel Corporation + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2, as + * published by the Free Software Foundation. + * + */ + +#include <linux/lsm_hooks.h> +#include <linux/sysctl.h> +#include <linux/fs_struct.h> +#include <linux/nsproxy.h> +#include <linux/pid_namespace.h> +#include <linux/printk.h> +#include "../fs/mount.h" + +/* describe a hardchroot info for a task */ +struct hardchroot_info { + struct task_struct *task; + struct dentry *dentry; + bool invalid; + struct list_head node; + struct rcu_head rcu; +}; + +static LIST_HEAD(hardchroot_infos); +static DEFINE_SPINLOCK(hardchroot_infos_lock); + +static void hardchroot_info_cleanup(struct work_struct *work); +static DECLARE_WORK(hardchroot_info_work, hardchroot_info_cleanup); + +/** + * hardchroot_info_cleanup - remove invalid entries from + * the hardchroot info list. + */ +static void hardchroot_info_cleanup(struct work_struct *work) +{ + struct hardchroot_info *entry; + + spin_lock(&hardchroot_infos_lock); + rcu_read_lock(); + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { + if (entry->invalid) { + list_del_rcu(&entry->node); + kfree_rcu(entry, rcu); + } + } + rcu_read_unlock(); + spin_unlock(&hardchroot_infos_lock); +} + +/** + * hardchroot_info_add - add/replace + * @task: the task_struct of the process entering chroot + * @dentry: the chroot dentry + * + * Returns 0 if info was added, -ve on error. + */ +static int hardchroot_info_add(struct task_struct *task, + struct dentry *dentry) +{ + struct hardchroot_info *entry; + struct hardchroot_info *added; + + added = kmalloc(sizeof(*added), GFP_KERNEL); + if (!added) + return -ENOMEM; + + added->task = task; + added->dentry = dentry; + added->invalid = false; + + spin_lock(&hardchroot_infos_lock); + rcu_read_lock(); + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { + if (entry->invalid) + continue; + if (entry->task == task) { + list_replace_rcu(&entry->node, &added->node); + kfree_rcu(entry, rcu); + goto out; + } + } + + list_add_rcu(&added->node, &hardchroot_infos); + +out: + rcu_read_unlock(); + spin_unlock(&hardchroot_infos_lock); + return 0; +} + +/** + * hardchroot_info_del - remove hardchroot info for a given task + * @task: remove any relation where task matches + * @dentry: remove any relation where dentry matches + */ +static void hardchroot_info_del(struct task_struct *task, + struct dentry *dentry) +{ + struct hardchroot_info *entry; + bool marked = false; + + rcu_read_lock(); + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { + if (entry->invalid) + continue; + if (entry->task == task || + (dentry && entry->dentry == dentry)) { + entry->invalid = true; + marked = true; + } + } + rcu_read_unlock(); + + if (marked) + schedule_work(&hardchroot_info_work); +} + +/** + * hardchroot_task_free - remove task from exception list + * @task: task being removed + */ +void hardchroot_task_free(struct task_struct *task) +{ + hardchroot_info_del(task, NULL); +} + +/** + * task_is_descendant - walk up a process family tree looking for a match + * The function is taken from Yama LSM + * @parent: the process to compare against while walking up from child + * @child: the process to start from while looking upwards for parent + * + * Returns 1 if child is a descendant of parent, 0 if not. + */ +static int task_is_descendant(struct task_struct *parent, + struct task_struct *child) +{ + int rc = 0; + struct task_struct *walker = child; + + if (!parent || !child) + return 0; + + rcu_read_lock(); + if (!thread_group_leader(parent)) + parent = rcu_dereference(parent->group_leader); + while (walker->pid > 0) { + if (!thread_group_leader(walker)) + walker = rcu_dereference(walker->group_leader); + if (walker == parent) { + rc = 1; + break; + } + walker = rcu_dereference(walker->real_parent); + } + rcu_read_unlock(); + + return rc; +} + +/** + * is_process_chrooted - process is inside chroot + * @task: the task_struct of the process to be checked + * + * Returns 1 if task is inside chroot. + */ +static int is_process_chrooted(struct task_struct *task) +{ + int rc = 0; + struct hardchroot_info *entry; + + rcu_read_lock(); + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { + if (entry->invalid) + continue; + if ((entry->task == task) || + (task_is_descendant(entry->task, task))) { + rc = 1; + pr_info("HCRT: pid %d is already chrooted\n", + task_pid_nr(entry->task)); + break; + } + } + rcu_read_unlock(); + return rc; +} + +/** + * is_same_root - check if two tasks share the same root + * @task1: the task_struct of the first task to be checked + * @task2: the task_struct of the second task to be checked + * + * Returns 1 if tasks share the same root. + */ +static int is_same_root(struct task_struct *task1, struct task_struct *task2) +{ + int rc = 0; + struct hardchroot_info *entry; + struct dentry *dentry1 = NULL; + struct dentry *dentry2 = NULL; + + rcu_read_lock(); + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { + if (entry->invalid) + continue; + if (entry->task == task1) + dentry1 = entry->dentry; + if (entry->task == task2) + dentry2 = entry->dentry; + } + if (dentry1 && (dentry1 == dentry2)) { + rc = 1; + pr_info("HCRT: pids %d and %d have the same root\n", + task_pid_nr(task1), task_pid_nr(task2)); + } + rcu_read_unlock(); + return rc; +} + +/** + * is_inside_chroot - check if dentry and mount + * are inside the current process fs root + * @u_dentry: dentry to be checked + * @u_mnt: mnt to be checked + * + * Returns 1 if dentry and mount are under fs root. + */ +int is_inside_chroot(const struct dentry *u_dentry, + const struct vfsmount *u_mnt) +{ + struct path path; + struct path currentroot; + int ret = 0; + + path.dentry = (struct dentry *)u_dentry; + path.mnt = (struct vfsmount *)u_mnt; + get_fs_root(current->fs, ¤troot); + if (path_is_under(&path, ¤troot)) + ret = 1; + else + pr_info("HCRT: dentry %lu is outside current task %d root\n", + d_backing_inode(u_dentry)->i_ino, + task_pid_nr(current)); + path_put(¤troot); + return ret; +} + +/** + * hardchroot_path_chroot - validate chroot entry + * @path contains the path structure. + * + * Returns 0 if chroot is allowed, -ve on error. + */ +static int hardchroot_path_chroot(const struct path *path) +{ + int rc = 0; + struct task_struct *myself = current; + + pr_info("HCRT, chroot: inode %lu and pid %d\n", + d_backing_inode(path->dentry)->i_ino, + task_pid_nr(myself)); + + get_task_struct(myself); + if (is_process_chrooted(myself) && + !is_inside_chroot(path->dentry, path->mnt)) { + put_task_struct(myself); + pr_info("HCRT, chroot denied: for inode %lu and pid %d\n", + d_backing_inode(path->dentry)->i_ino, + task_pid_nr(myself)); + return -EACCES; + } + + if (task_pid_nr(myself) > 1 && + path->dentry != init_task.fs->root.dentry && + path->dentry != myself->nsproxy->mnt_ns->root->mnt.mnt_root) { + /* task is attempting to chroot, add it to the list */ + rc = hardchroot_info_add(myself, path->dentry); + pr_info("HCRT, chroot: adding %d to chrooted task list\n", + task_pid_nr(myself)); + } + + /* set the current working directory of all newly-chrooted + * processes to the the root directory of the chroot + */ + set_fs_pwd(myself->fs, path); + put_task_struct(myself); + + return rc; +} + +/** + * hardchroot_task_unshare - check if process is + * allowed to unshare its namespaces + * @unshare_flags flags + * @new_fs contains the new fs_struct if created. + * @new_fd contains the new files_struct if created. + * @new_creds contains the new cred if created. + * @new_nsproxy contains the new nsproxy if created. + * + * Returns 0 if unshare is allowed, -ve on error. + */ +static int hardchroot_task_unshare(unsigned long unshare_flags, + const struct fs_struct *new_fs, + const struct files_struct *new_fd, + const struct cred *new_cred, + const struct nsproxy *new_nsproxy) +{ + int rc = 0; + struct task_struct *myself = current; + const struct nsproxy *tnsproxy = new_nsproxy; + + pr_info("HCRT, unshare: unshare_flags %lu and pid %d\n", + unshare_flags, task_pid_nr(myself)); + if (new_fs) + pr_info("HCRT, unshare: new_fs->root.dentry inode%lu\n", + d_backing_inode(new_fs->root.dentry)->i_ino); + + if (!tnsproxy) + tnsproxy = myself->nsproxy; + + if (new_fs && task_pid_nr(myself) > 1 && + new_fs->root.dentry != init_task.fs->root.dentry && + new_fs->root.dentry != tnsproxy->mnt_ns->root->mnt.mnt_root) { + rc = hardchroot_info_add(myself, new_fs->root.dentry); + pr_info("HCRT, unshare: adding %d to chrooted task list\n", + task_pid_nr(myself)); + } + + return rc; +} + +/** + * hardchroot_sb_unsharefs - check if process is + * allowed to unshare fs_struct + * @path contains the path for the new root structure. + * + * Returns 0 if unsharefs is allowed, -ve on error. + */ +static int hardchroot_sb_unsharefs(const struct path *path) +{ + int rc = 0; + struct task_struct *myself = current; + + pr_info("HCRT, unsharefs: inode %lu and pid %d\n", + d_backing_inode(path->dentry)->i_ino, + task_pid_nr(myself)); + + if (task_pid_nr(myself) > 1 && + path->dentry != init_task.fs->root.dentry && + path->dentry != myself->nsproxy->mnt_ns->root->mnt.mnt_root) { + rc = hardchroot_info_add(myself, path->dentry); + pr_info("HCRT, unsharefs: adding %d to chrooted task list\n", + task_pid_nr(myself)); + } + + return rc; +} + +/** + * hardchroot_path_chmod - validate if chmod is allowed + * @mnt contains the vfsmnt structure. + * @mode contains DAC's mode + * + * Returns 0 if allowed, -ve on error. + */ +static int hardchroot_path_chmod(const struct path *path, umode_t mode) +{ + int rc = 0; + struct task_struct *myself = current; + + pr_info("HCRT, chmod: inode %lu, pid %d\n", + d_backing_inode(path->dentry)->i_ino, + task_pid_nr(myself)); + + /* allow chmod +s on directories, but not files */ + if (!S_ISDIR(path->dentry->d_inode->i_mode) && ((mode & S_ISUID) || + ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))) && + is_process_chrooted(myself)) { + pr_info("HCRT, chmod denied: inode %lu, pid %d\n", + d_backing_inode(path->dentry)->i_ino, + task_pid_nr(myself)); + return -EACCES; + } + + return rc; + +} + +/** + * hardchroot_path_fchdir - validate if fchdir is allowed + * @path: contains the path structure + * + * Returns 0 if allowed, -ve on error. + */ +static int hardchroot_path_fchdir(const struct path *path) +{ + int rc = 0; + struct task_struct *myself = current; + + pr_info("HCRT, fchdir: pid %d, path %lu", + task_pid_nr(myself), + d_backing_inode(path->dentry)->i_ino); + + if (!is_process_chrooted(myself)) + return rc; + if (!is_inside_chroot(path->dentry, path->mnt)) { + pr_info("HCRT, fchdir denied: pid %d, path %lu", + task_pid_nr(myself), + d_backing_inode(path->dentry)->i_ino); + return -EACCES; + } + + return rc; +} + +/** + * hardchroot_path_fhandle - validate if converting + * handle to path is allowed + * @path: contains the path structure + * + * Returns 0 if allowed, -ve on error. + */ +static int hardchroot_path_fhandle(const struct path *path) +{ + int rc = 0; + struct task_struct *myself = current; + + pr_info("HCRT, fhandle: pid %d, path %lu", + task_pid_nr(myself), + d_backing_inode(path->dentry)->i_ino); + + if (is_process_chrooted(myself)) { + pr_info("HCRT, fhandle denied: pid %d, path %lu", + task_pid_nr(myself), + d_backing_inode(path->dentry)->i_ino); + return -EACCES; + } + + return rc; +} + +/** + * hardchroot_task_setnice - check if setting nice is allowed + * @task contains the task_struct of process. + * @nice contains the new nice value. + * + * Return 0 if allowed, -ve on error. + */ +static int hardchroot_task_setnice(struct task_struct *task, int nice) +{ + int rc = 0; + struct task_struct *myself = current; + + pr_info("HCRT, setnice: current %d, nice %d, for pid %d and current nice %d\n", + task_pid_nr(myself), nice, + task_pid_nr(task), task_nice(task)); + + if (is_process_chrooted(myself) && (nice < task_nice(task))) { + pr_info("HCRT, setnice denied: current %d, nice %d, for pid %d and current nice %d\n", + task_pid_nr(myself), nice, + task_pid_nr(task), task_nice(task)); + return -EACCES; + } + return rc; +} + +/** + * hardchroot_path_mknod - check if mknod is allowed + * @dir contains the path structure of parent of the new file. + * @dentry contains the dentry structure of the new file. + * @mode contains the mode of the new file. + * @dev contains the undecoded device number. Use new_decode_dev() to get + * the decoded device number. + * + * Return 0 if allowed, -ve on error. + */ +static int hardchroot_path_mknod(const struct path *dir, struct dentry *dentry, + umode_t mode, unsigned int dev) +{ + int rc = 0; + struct task_struct *myself = current; + + pr_info("HCRT, mknod: dir %lu, mode %d pid %d\n", + d_backing_inode(dir->dentry)->i_ino, + mode, task_pid_nr(myself)); + + if (!S_ISFIFO(mode) && !S_ISREG(mode) && is_process_chrooted(myself)) { + pr_info("HCRT, mknod denied: dir %lu, mode %d pid %d\n", + d_backing_inode(dir->dentry)->i_ino, + mode, task_pid_nr(myself)); + return -EACCES; + } + return rc; +} + +/** + * hardchroot_sb_mount - check if mount is allowed + * @dev_name contains the name for object being mounted. + * @path contains the path for mount point object. + * @type contains the filesystem type. + * @flags contains the mount flags. + * @data contains the filesystem-specific data. + * + * Return 0 if allowed, -ve on error. + */ +static int hardchroot_sb_mount(const char *dev_name, const struct path *path, + const char *type, unsigned long flags, void *data) +{ + int rc = 0; + struct task_struct *myself = current; + + pr_info("HCRT, mount: dev %s, inode %lu, flags %lu pid %d\n", + dev_name, d_backing_inode(path->dentry)->i_ino, + flags, task_pid_nr(myself)); + + if (is_process_chrooted(myself)) { + pr_info("HCRT, mount denied: dev %s, inode %lu, flags %lu, pid %d\n", + dev_name, d_backing_inode(path->dentry)->i_ino, + flags, task_pid_nr(myself)); + return -EACCES; + } + return rc; +} + +/** + * hardchroot_sb_pivotroot - check if pivotroot is allowed + * @old_path contains the path for the new location of the + * current root (put_old). + * @new_path contains the path for the new root (new_root). + * + * Return 0 if allowed, -ve on error. + */ +static int hardchroot_sb_pivotroot(const struct path *old_path, + const struct path *new_path) +{ + int rc = 0; + struct task_struct *myself = current; + + pr_info("HCRT, pivotroot: old %lu, new %lu, pid %d\n", + d_backing_inode(old_path->dentry)->i_ino, + d_backing_inode(new_path->dentry)->i_ino, + task_pid_nr(myself)); + + if (is_process_chrooted(myself)) { + pr_info("HCRT, pivotroot denied: old %lu, new %lu, pid %d\n", + d_backing_inode(old_path->dentry)->i_ino, + d_backing_inode(new_path->dentry)->i_ino, + task_pid_nr(myself)); + return -EACCES; + } + return rc; +} + +/** + * hardchroot_shm_shmat - check if shmat is allowed + * @shp contains the shared memory structure to be modified. + * @shmaddr contains the address to attach memory region to. + * @shmflg contains the operational flags. + * + * Return 0 if allowed, -ve on error. + */ +int hardchroot_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr, + int shmflg) +{ + int rc = 0; + struct task_struct *p; + struct task_struct *myself = current; + u64 st; + time_t ct; + + pr_info("HCRT, shmat: shp %d, shmflg %d, pid %d\n", + shp->shm_perm.id, shmflg, + task_pid_nr(myself)); + + if (likely(!is_process_chrooted(myself))) + return rc; + + rcu_read_lock(); + read_lock(&tasklist_lock); + + p = find_task_by_vpid(shp->shm_cprid); + if (p) { + st = p->start_time; + ct = shp->shm_ctim; + if (time_before_eq((unsigned long)st, (unsigned long)ct)) { + if (is_same_root(myself, p)) + goto allow; + else { + read_unlock(&tasklist_lock); + rcu_read_unlock(); + pr_info("HCRT, shmat denied: shp %d, shmflg %d, pid %d\n", + shp->shm_perm.id, shmflg, + task_pid_nr(myself)); + return -EACCES; + } + } + /* creator exited, pid reuse, fall through to next check */ + } + p = find_task_by_vpid(shp->shm_lprid); + if (p) { + if (unlikely(!is_same_root(myself, p))) { + read_unlock(&tasklist_lock); + rcu_read_unlock(); + pr_info("HCRT, shmat denied: shp %d, shmflg %d, pid %d\n", + shp->shm_perm.id, shmflg, + task_pid_nr(myself)); + return -EACCES; + } + } + +allow: + read_unlock(&tasklist_lock); + rcu_read_unlock(); + + return rc; + + +} + +static struct security_hook_list hardchroot_hooks[] = { + LSM_HOOK_INIT(path_chroot, hardchroot_path_chroot), + LSM_HOOK_INIT(path_chmod, hardchroot_path_chmod), + LSM_HOOK_INIT(path_mknod, hardchroot_path_mknod), + LSM_HOOK_INIT(path_fchdir, hardchroot_path_fchdir), + LSM_HOOK_INIT(path_fhandle, hardchroot_path_fhandle), + LSM_HOOK_INIT(sb_mount, hardchroot_sb_mount), + LSM_HOOK_INIT(sb_pivotroot, hardchroot_sb_pivotroot), + LSM_HOOK_INIT(sb_unsharefs, hardchroot_sb_unsharefs), + LSM_HOOK_INIT(task_setnice, hardchroot_task_setnice), + LSM_HOOK_INIT(task_free, hardchroot_task_free), + LSM_HOOK_INIT(task_unshare, hardchroot_task_unshare), + LSM_HOOK_INIT(shm_shmat, hardchroot_shm_shmat), +}; + +static inline void hardchroot_init_sysctl(void) { } + +void __init hardchroot_add_hooks(void) +{ + pr_info("Hardchroot: Getting stronger.\n"); + security_add_hooks(hardchroot_hooks, ARRAY_SIZE(hardchroot_hooks)); + hardchroot_init_sysctl(); +} diff --git a/security/security.c b/security/security.c index 95487b9..ff65f06 100644 --- a/security/security.c +++ b/security/security.c @@ -61,6 +61,7 @@ int __init security_init(void) capability_add_hooks(); yama_add_hooks(); loadpin_add_hooks(); + hardchroot_add_hooks(); /* * Load all the remaining security modules. -- 1.9.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [kernel-hardening] Re: [RFC] [PATCH 5/5] Hardchroot LSM 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM Elena Reshetova @ 2016-07-29 11:44 ` Brad Spengler 2016-07-29 12:15 ` [kernel-hardening] " Reshetova, Elena 2016-07-29 12:25 ` Reshetova, Elena 2016-07-29 18:53 ` [kernel-hardening] " Jann Horn 1 sibling, 2 replies; 33+ messages in thread From: Brad Spengler @ 2016-07-29 11:44 UTC (permalink / raw) To: Elena Reshetova Cc: kernel-hardening, linux-security-module, keescook, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts [-- Attachment #1: Type: text/plain, Size: 20703 bytes --] Hi, This again is completely unacceptable. > + * Copyright (c) 2016, Intel Corporation You know full well several functions in this code are completely copy+pasted from grsecurity, keeping unnecessary types, same exact variable names, same exact casts, same exact code structure. In other cases you've simply renamed variables. Last time I had to mention this, the entirety of my RANDSTRUCT plugin had been copy+pasted by an Intel employee with Intel's copyright placed alone on the top of it. Why does Intel have such a problem with plagiarism? Yes, there are some original changes in here, but this doesn't seem to have been tested at all -- I see obvious ways of bypassing it, and some of the checks that have been modified (aka single lines that weren't copy+pasted) are now wrong and simply won't work at all. I'm sure Kees will save the day though by repeating the flaw I just mentioned on IRC. For the rest, Intel will have to do some actual original work for a change. -Brad > +#include <linux/lsm_hooks.h> > +#include <linux/sysctl.h> > +#include <linux/fs_struct.h> > +#include <linux/nsproxy.h> > +#include <linux/pid_namespace.h> > +#include <linux/printk.h> > +#include "../fs/mount.h" > + > +/* describe a hardchroot info for a task */ > +struct hardchroot_info { > + struct task_struct *task; > + struct dentry *dentry; > + bool invalid; > + struct list_head node; > + struct rcu_head rcu; > +}; > + > +static LIST_HEAD(hardchroot_infos); > +static DEFINE_SPINLOCK(hardchroot_infos_lock); > + > +static void hardchroot_info_cleanup(struct work_struct *work); > +static DECLARE_WORK(hardchroot_info_work, hardchroot_info_cleanup); > + > +/** > + * hardchroot_info_cleanup - remove invalid entries from > + * the hardchroot info list. > + */ > +static void hardchroot_info_cleanup(struct work_struct *work) > +{ > + struct hardchroot_info *entry; > + > + spin_lock(&hardchroot_infos_lock); > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) { > + list_del_rcu(&entry->node); > + kfree_rcu(entry, rcu); > + } > + } > + rcu_read_unlock(); > + spin_unlock(&hardchroot_infos_lock); > +} > + > +/** > + * hardchroot_info_add - add/replace > + * @task: the task_struct of the process entering chroot > + * @dentry: the chroot dentry > + * > + * Returns 0 if info was added, -ve on error. > + */ > +static int hardchroot_info_add(struct task_struct *task, > + struct dentry *dentry) > +{ > + struct hardchroot_info *entry; > + struct hardchroot_info *added; > + > + added = kmalloc(sizeof(*added), GFP_KERNEL); > + if (!added) > + return -ENOMEM; > + > + added->task = task; > + added->dentry = dentry; > + added->invalid = false; > + > + spin_lock(&hardchroot_infos_lock); > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) > + continue; > + if (entry->task == task) { > + list_replace_rcu(&entry->node, &added->node); > + kfree_rcu(entry, rcu); > + goto out; > + } > + } > + > + list_add_rcu(&added->node, &hardchroot_infos); > + > +out: > + rcu_read_unlock(); > + spin_unlock(&hardchroot_infos_lock); > + return 0; > +} > + > +/** > + * hardchroot_info_del - remove hardchroot info for a given task > + * @task: remove any relation where task matches > + * @dentry: remove any relation where dentry matches > + */ > +static void hardchroot_info_del(struct task_struct *task, > + struct dentry *dentry) > +{ > + struct hardchroot_info *entry; > + bool marked = false; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) > + continue; > + if (entry->task == task || > + (dentry && entry->dentry == dentry)) { > + entry->invalid = true; > + marked = true; > + } > + } > + rcu_read_unlock(); > + > + if (marked) > + schedule_work(&hardchroot_info_work); > +} > + > +/** > + * hardchroot_task_free - remove task from exception list > + * @task: task being removed > + */ > +void hardchroot_task_free(struct task_struct *task) > +{ > + hardchroot_info_del(task, NULL); > +} > + > +/** > + * task_is_descendant - walk up a process family tree looking for a match > + * The function is taken from Yama LSM > + * @parent: the process to compare against while walking up from child > + * @child: the process to start from while looking upwards for parent > + * > + * Returns 1 if child is a descendant of parent, 0 if not. > + */ > +static int task_is_descendant(struct task_struct *parent, > + struct task_struct *child) > +{ > + int rc = 0; > + struct task_struct *walker = child; > + > + if (!parent || !child) > + return 0; > + > + rcu_read_lock(); > + if (!thread_group_leader(parent)) > + parent = rcu_dereference(parent->group_leader); > + while (walker->pid > 0) { > + if (!thread_group_leader(walker)) > + walker = rcu_dereference(walker->group_leader); > + if (walker == parent) { > + rc = 1; > + break; > + } > + walker = rcu_dereference(walker->real_parent); > + } > + rcu_read_unlock(); > + > + return rc; > +} > + > +/** > + * is_process_chrooted - process is inside chroot > + * @task: the task_struct of the process to be checked > + * > + * Returns 1 if task is inside chroot. > + */ > +static int is_process_chrooted(struct task_struct *task) > +{ > + int rc = 0; > + struct hardchroot_info *entry; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) > + continue; > + if ((entry->task == task) || > + (task_is_descendant(entry->task, task))) { > + rc = 1; > + pr_info("HCRT: pid %d is already chrooted\n", > + task_pid_nr(entry->task)); > + break; > + } > + } > + rcu_read_unlock(); > + return rc; > +} > + > +/** > + * is_same_root - check if two tasks share the same root > + * @task1: the task_struct of the first task to be checked > + * @task2: the task_struct of the second task to be checked > + * > + * Returns 1 if tasks share the same root. > + */ > +static int is_same_root(struct task_struct *task1, struct task_struct *task2) > +{ > + int rc = 0; > + struct hardchroot_info *entry; > + struct dentry *dentry1 = NULL; > + struct dentry *dentry2 = NULL; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) > + continue; > + if (entry->task == task1) > + dentry1 = entry->dentry; > + if (entry->task == task2) > + dentry2 = entry->dentry; > + } > + if (dentry1 && (dentry1 == dentry2)) { > + rc = 1; > + pr_info("HCRT: pids %d and %d have the same root\n", > + task_pid_nr(task1), task_pid_nr(task2)); > + } > + rcu_read_unlock(); > + return rc; > +} > + > +/** > + * is_inside_chroot - check if dentry and mount > + * are inside the current process fs root > + * @u_dentry: dentry to be checked > + * @u_mnt: mnt to be checked > + * > + * Returns 1 if dentry and mount are under fs root. > + */ > +int is_inside_chroot(const struct dentry *u_dentry, > + const struct vfsmount *u_mnt) > +{ > + struct path path; > + struct path currentroot; > + int ret = 0; > + > + path.dentry = (struct dentry *)u_dentry; > + path.mnt = (struct vfsmount *)u_mnt; > + get_fs_root(current->fs, ¤troot); > + if (path_is_under(&path, ¤troot)) > + ret = 1; > + else > + pr_info("HCRT: dentry %lu is outside current task %d root\n", > + d_backing_inode(u_dentry)->i_ino, > + task_pid_nr(current)); > + path_put(¤troot); > + return ret; > +} > + > +/** > + * hardchroot_path_chroot - validate chroot entry > + * @path contains the path structure. > + * > + * Returns 0 if chroot is allowed, -ve on error. > + */ > +static int hardchroot_path_chroot(const struct path *path) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, chroot: inode %lu and pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + > + get_task_struct(myself); > + if (is_process_chrooted(myself) && > + !is_inside_chroot(path->dentry, path->mnt)) { > + put_task_struct(myself); > + pr_info("HCRT, chroot denied: for inode %lu and pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + return -EACCES; > + } > + > + if (task_pid_nr(myself) > 1 && > + path->dentry != init_task.fs->root.dentry && > + path->dentry != myself->nsproxy->mnt_ns->root->mnt.mnt_root) { > + /* task is attempting to chroot, add it to the list */ > + rc = hardchroot_info_add(myself, path->dentry); > + pr_info("HCRT, chroot: adding %d to chrooted task list\n", > + task_pid_nr(myself)); > + } > + > + /* set the current working directory of all newly-chrooted > + * processes to the the root directory of the chroot > + */ > + set_fs_pwd(myself->fs, path); > + put_task_struct(myself); > + > + return rc; > +} > + > +/** > + * hardchroot_task_unshare - check if process is > + * allowed to unshare its namespaces > + * @unshare_flags flags > + * @new_fs contains the new fs_struct if created. > + * @new_fd contains the new files_struct if created. > + * @new_creds contains the new cred if created. > + * @new_nsproxy contains the new nsproxy if created. > + * > + * Returns 0 if unshare is allowed, -ve on error. > + */ > +static int hardchroot_task_unshare(unsigned long unshare_flags, > + const struct fs_struct *new_fs, > + const struct files_struct *new_fd, > + const struct cred *new_cred, > + const struct nsproxy *new_nsproxy) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + const struct nsproxy *tnsproxy = new_nsproxy; > + > + pr_info("HCRT, unshare: unshare_flags %lu and pid %d\n", > + unshare_flags, task_pid_nr(myself)); > + if (new_fs) > + pr_info("HCRT, unshare: new_fs->root.dentry inode%lu\n", > + d_backing_inode(new_fs->root.dentry)->i_ino); > + > + if (!tnsproxy) > + tnsproxy = myself->nsproxy; > + > + if (new_fs && task_pid_nr(myself) > 1 && > + new_fs->root.dentry != init_task.fs->root.dentry && > + new_fs->root.dentry != tnsproxy->mnt_ns->root->mnt.mnt_root) { > + rc = hardchroot_info_add(myself, new_fs->root.dentry); > + pr_info("HCRT, unshare: adding %d to chrooted task list\n", > + task_pid_nr(myself)); > + } > + > + return rc; > +} > + > +/** > + * hardchroot_sb_unsharefs - check if process is > + * allowed to unshare fs_struct > + * @path contains the path for the new root structure. > + * > + * Returns 0 if unsharefs is allowed, -ve on error. > + */ > +static int hardchroot_sb_unsharefs(const struct path *path) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, unsharefs: inode %lu and pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + > + if (task_pid_nr(myself) > 1 && > + path->dentry != init_task.fs->root.dentry && > + path->dentry != myself->nsproxy->mnt_ns->root->mnt.mnt_root) { > + rc = hardchroot_info_add(myself, path->dentry); > + pr_info("HCRT, unsharefs: adding %d to chrooted task list\n", > + task_pid_nr(myself)); > + } > + > + return rc; > +} > + > +/** > + * hardchroot_path_chmod - validate if chmod is allowed > + * @mnt contains the vfsmnt structure. > + * @mode contains DAC's mode > + * > + * Returns 0 if allowed, -ve on error. > + */ > +static int hardchroot_path_chmod(const struct path *path, umode_t mode) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, chmod: inode %lu, pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + > + /* allow chmod +s on directories, but not files */ > + if (!S_ISDIR(path->dentry->d_inode->i_mode) && ((mode & S_ISUID) || > + ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))) && > + is_process_chrooted(myself)) { > + pr_info("HCRT, chmod denied: inode %lu, pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + return -EACCES; > + } > + > + return rc; > + > +} > + > +/** > + * hardchroot_path_fchdir - validate if fchdir is allowed > + * @path: contains the path structure > + * > + * Returns 0 if allowed, -ve on error. > + */ > +static int hardchroot_path_fchdir(const struct path *path) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, fchdir: pid %d, path %lu", > + task_pid_nr(myself), > + d_backing_inode(path->dentry)->i_ino); > + > + if (!is_process_chrooted(myself)) > + return rc; > + if (!is_inside_chroot(path->dentry, path->mnt)) { > + pr_info("HCRT, fchdir denied: pid %d, path %lu", > + task_pid_nr(myself), > + d_backing_inode(path->dentry)->i_ino); > + return -EACCES; > + } > + > + return rc; > +} > + > +/** > + * hardchroot_path_fhandle - validate if converting > + * handle to path is allowed > + * @path: contains the path structure > + * > + * Returns 0 if allowed, -ve on error. > + */ > +static int hardchroot_path_fhandle(const struct path *path) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, fhandle: pid %d, path %lu", > + task_pid_nr(myself), > + d_backing_inode(path->dentry)->i_ino); > + > + if (is_process_chrooted(myself)) { > + pr_info("HCRT, fhandle denied: pid %d, path %lu", > + task_pid_nr(myself), > + d_backing_inode(path->dentry)->i_ino); > + return -EACCES; > + } > + > + return rc; > +} > + > +/** > + * hardchroot_task_setnice - check if setting nice is allowed > + * @task contains the task_struct of process. > + * @nice contains the new nice value. > + * > + * Return 0 if allowed, -ve on error. > + */ > +static int hardchroot_task_setnice(struct task_struct *task, int nice) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, setnice: current %d, nice %d, for pid %d and current nice %d\n", > + task_pid_nr(myself), nice, > + task_pid_nr(task), task_nice(task)); > + > + if (is_process_chrooted(myself) && (nice < task_nice(task))) { > + pr_info("HCRT, setnice denied: current %d, nice %d, for pid %d and current nice %d\n", > + task_pid_nr(myself), nice, > + task_pid_nr(task), task_nice(task)); > + return -EACCES; > + } > + return rc; > +} > + > +/** > + * hardchroot_path_mknod - check if mknod is allowed > + * @dir contains the path structure of parent of the new file. > + * @dentry contains the dentry structure of the new file. > + * @mode contains the mode of the new file. > + * @dev contains the undecoded device number. Use new_decode_dev() to get > + * the decoded device number. > + * > + * Return 0 if allowed, -ve on error. > + */ > +static int hardchroot_path_mknod(const struct path *dir, struct dentry *dentry, > + umode_t mode, unsigned int dev) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, mknod: dir %lu, mode %d pid %d\n", > + d_backing_inode(dir->dentry)->i_ino, > + mode, task_pid_nr(myself)); > + > + if (!S_ISFIFO(mode) && !S_ISREG(mode) && is_process_chrooted(myself)) { > + pr_info("HCRT, mknod denied: dir %lu, mode %d pid %d\n", > + d_backing_inode(dir->dentry)->i_ino, > + mode, task_pid_nr(myself)); > + return -EACCES; > + } > + return rc; > +} > + > +/** > + * hardchroot_sb_mount - check if mount is allowed > + * @dev_name contains the name for object being mounted. > + * @path contains the path for mount point object. > + * @type contains the filesystem type. > + * @flags contains the mount flags. > + * @data contains the filesystem-specific data. > + * > + * Return 0 if allowed, -ve on error. > + */ > +static int hardchroot_sb_mount(const char *dev_name, const struct path *path, > + const char *type, unsigned long flags, void *data) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, mount: dev %s, inode %lu, flags %lu pid %d\n", > + dev_name, d_backing_inode(path->dentry)->i_ino, > + flags, task_pid_nr(myself)); > + > + if (is_process_chrooted(myself)) { > + pr_info("HCRT, mount denied: dev %s, inode %lu, flags %lu, pid %d\n", > + dev_name, d_backing_inode(path->dentry)->i_ino, > + flags, task_pid_nr(myself)); > + return -EACCES; > + } > + return rc; > +} > + > +/** > + * hardchroot_sb_pivotroot - check if pivotroot is allowed > + * @old_path contains the path for the new location of the > + * current root (put_old). > + * @new_path contains the path for the new root (new_root). > + * > + * Return 0 if allowed, -ve on error. > + */ > +static int hardchroot_sb_pivotroot(const struct path *old_path, > + const struct path *new_path) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, pivotroot: old %lu, new %lu, pid %d\n", > + d_backing_inode(old_path->dentry)->i_ino, > + d_backing_inode(new_path->dentry)->i_ino, > + task_pid_nr(myself)); > + > + if (is_process_chrooted(myself)) { > + pr_info("HCRT, pivotroot denied: old %lu, new %lu, pid %d\n", > + d_backing_inode(old_path->dentry)->i_ino, > + d_backing_inode(new_path->dentry)->i_ino, > + task_pid_nr(myself)); > + return -EACCES; > + } > + return rc; > +} > + > +/** > + * hardchroot_shm_shmat - check if shmat is allowed > + * @shp contains the shared memory structure to be modified. > + * @shmaddr contains the address to attach memory region to. > + * @shmflg contains the operational flags. > + * > + * Return 0 if allowed, -ve on error. > + */ > +int hardchroot_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr, > + int shmflg) > +{ > + int rc = 0; > + struct task_struct *p; > + struct task_struct *myself = current; > + u64 st; > + time_t ct; > + > + pr_info("HCRT, shmat: shp %d, shmflg %d, pid %d\n", > + shp->shm_perm.id, shmflg, > + task_pid_nr(myself)); > + > + if (likely(!is_process_chrooted(myself))) > + return rc; > + > + rcu_read_lock(); > + read_lock(&tasklist_lock); > + > + p = find_task_by_vpid(shp->shm_cprid); > + if (p) { > + st = p->start_time; > + ct = shp->shm_ctim; > + if (time_before_eq((unsigned long)st, (unsigned long)ct)) { > + if (is_same_root(myself, p)) > + goto allow; > + else { > + read_unlock(&tasklist_lock); > + rcu_read_unlock(); > + pr_info("HCRT, shmat denied: shp %d, shmflg %d, pid %d\n", > + shp->shm_perm.id, shmflg, > + task_pid_nr(myself)); > + return -EACCES; > + } > + } > + /* creator exited, pid reuse, fall through to next check */ > + } > + p = find_task_by_vpid(shp->shm_lprid); > + if (p) { > + if (unlikely(!is_same_root(myself, p))) { > + read_unlock(&tasklist_lock); > + rcu_read_unlock(); > + pr_info("HCRT, shmat denied: shp %d, shmflg %d, pid %d\n", > + shp->shm_perm.id, shmflg, > + task_pid_nr(myself)); > + return -EACCES; > + } > + } > + > +allow: > + read_unlock(&tasklist_lock); > + rcu_read_unlock(); > + > + return rc; > + > + > +} > + > +static struct security_hook_list hardchroot_hooks[] = { > + LSM_HOOK_INIT(path_chroot, hardchroot_path_chroot), > + LSM_HOOK_INIT(path_chmod, hardchroot_path_chmod), > + LSM_HOOK_INIT(path_mknod, hardchroot_path_mknod), > + LSM_HOOK_INIT(path_fchdir, hardchroot_path_fchdir), > + LSM_HOOK_INIT(path_fhandle, hardchroot_path_fhandle), > + LSM_HOOK_INIT(sb_mount, hardchroot_sb_mount), > + LSM_HOOK_INIT(sb_pivotroot, hardchroot_sb_pivotroot), > + LSM_HOOK_INIT(sb_unsharefs, hardchroot_sb_unsharefs), > + LSM_HOOK_INIT(task_setnice, hardchroot_task_setnice), > + LSM_HOOK_INIT(task_free, hardchroot_task_free), > + LSM_HOOK_INIT(task_unshare, hardchroot_task_unshare), > + LSM_HOOK_INIT(shm_shmat, hardchroot_shm_shmat), > +}; > + > +static inline void hardchroot_init_sysctl(void) { } > + > +void __init hardchroot_add_hooks(void) > +{ > + pr_info("Hardchroot: Getting stronger.\n"); > + security_add_hooks(hardchroot_hooks, ARRAY_SIZE(hardchroot_hooks)); > + hardchroot_init_sysctl(); > +} > diff --git a/security/security.c b/security/security.c > index 95487b9..ff65f06 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -61,6 +61,7 @@ int __init security_init(void) > capability_add_hooks(); > yama_add_hooks(); > loadpin_add_hooks(); > + hardchroot_add_hooks(); > > /* > * Load all the remaining security modules. > -- > 1.9.1 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* [kernel-hardening] RE: [RFC] [PATCH 5/5] Hardchroot LSM 2016-07-29 11:44 ` [kernel-hardening] " Brad Spengler @ 2016-07-29 12:15 ` Reshetova, Elena 2016-07-29 12:25 ` Reshetova, Elena 1 sibling, 0 replies; 33+ messages in thread From: Reshetova, Elena @ 2016-07-29 12:15 UTC (permalink / raw) To: Brad Spengler Cc: kernel-hardening, linux-security-module, keescook, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 21119 bytes --] >This again is completely unacceptable. >> + * Copyright (c) 2016, Intel Corporation >You know full well several functions in this code are completely >copy+pasted from grsecurity, keeping unnecessary types, same exact >variable names, same exact casts, same exact code structure. In other cases you've simply renamed variables. Last time I had to mention this, the entirety of my RANDSTRUCT plugin had been copy+pasted by an Intel employee with Intel's copyright >placed alone on the top of it. >Why does Intel have such a problem with plagiarism? I am not sure how to handle the copyright statement in this case. Part of code comes from your code (and acknowledgment provided), task management done based on Yama (also acknowledged), part of code is modified, new and etc. I will ask lawyers on this to clarify since this is not really my choice. I am not interested to take credit for this, I was just interested to make the feature work inside LSM and be accepted to upstream. >Yes, there are some original changes in here, but this doesn't seem to have been tested at all -- I see obvious ways of bypassing it, and some of the checks that have been modified (aka single lines that weren't copy+pasted) are now wrong and simply >won't work at all. It was actually tested, I won't send untested code for review. I have tested this on Ubuntu and Fedora, as well as with Ubuntu standard chroot. I don't have any automatic tests done yet and there are couple of corner cases that I omitted now, as I don't mark process from init_mount_tree(), don't restrict filename lookup yet, don't cover mount with MS_MOVE flag and etc. This is beginning of the work to bring all these features in place, and I figured I would get initial comments earlier than later on this. >I'm sure Kees will save the day though by repeating the flaw I just mentioned on IRC. For the rest, Intel will have to do some actual original work for a change. This was actually the main point of sending these patches now, I want feedback and I am grateful for it. So, thank you! Anything you can point out is valuable. > +#include <linux/lsm_hooks.h> > +#include <linux/sysctl.h> > +#include <linux/fs_struct.h> > +#include <linux/nsproxy.h> > +#include <linux/pid_namespace.h> > +#include <linux/printk.h> > +#include "../fs/mount.h" > + > +/* describe a hardchroot info for a task */ struct hardchroot_info { > + struct task_struct *task; > + struct dentry *dentry; > + bool invalid; > + struct list_head node; > + struct rcu_head rcu; > +}; > + > +static LIST_HEAD(hardchroot_infos); > +static DEFINE_SPINLOCK(hardchroot_infos_lock); > + > +static void hardchroot_info_cleanup(struct work_struct *work); static > +DECLARE_WORK(hardchroot_info_work, hardchroot_info_cleanup); > + > +/** > + * hardchroot_info_cleanup - remove invalid entries from > + * the hardchroot info list. > + */ > +static void hardchroot_info_cleanup(struct work_struct *work) { > + struct hardchroot_info *entry; > + > + spin_lock(&hardchroot_infos_lock); > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) { > + list_del_rcu(&entry->node); > + kfree_rcu(entry, rcu); > + } > + } > + rcu_read_unlock(); > + spin_unlock(&hardchroot_infos_lock); > +} > + > +/** > + * hardchroot_info_add - add/replace > + * @task: the task_struct of the process entering chroot > + * @dentry: the chroot dentry > + * > + * Returns 0 if info was added, -ve on error. > + */ > +static int hardchroot_info_add(struct task_struct *task, > + struct dentry *dentry) > +{ > + struct hardchroot_info *entry; > + struct hardchroot_info *added; > + > + added = kmalloc(sizeof(*added), GFP_KERNEL); > + if (!added) > + return -ENOMEM; > + > + added->task = task; > + added->dentry = dentry; > + added->invalid = false; > + > + spin_lock(&hardchroot_infos_lock); > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) > + continue; > + if (entry->task == task) { > + list_replace_rcu(&entry->node, &added->node); > + kfree_rcu(entry, rcu); > + goto out; > + } > + } > + > + list_add_rcu(&added->node, &hardchroot_infos); > + > +out: > + rcu_read_unlock(); > + spin_unlock(&hardchroot_infos_lock); > + return 0; > +} > + > +/** > + * hardchroot_info_del - remove hardchroot info for a given task > + * @task: remove any relation where task matches > + * @dentry: remove any relation where dentry matches */ static void > +hardchroot_info_del(struct task_struct *task, > + struct dentry *dentry) > +{ > + struct hardchroot_info *entry; > + bool marked = false; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) > + continue; > + if (entry->task == task || > + (dentry && entry->dentry == dentry)) { > + entry->invalid = true; > + marked = true; > + } > + } > + rcu_read_unlock(); > + > + if (marked) > + schedule_work(&hardchroot_info_work); > +} > + > +/** > + * hardchroot_task_free - remove task from exception list > + * @task: task being removed > + */ > +void hardchroot_task_free(struct task_struct *task) { > + hardchroot_info_del(task, NULL); > +} > + > +/** > + * task_is_descendant - walk up a process family tree looking for a > +match > + * The function is taken from Yama LSM > + * @parent: the process to compare against while walking up from > +child > + * @child: the process to start from while looking upwards for parent > + * > + * Returns 1 if child is a descendant of parent, 0 if not. > + */ > +static int task_is_descendant(struct task_struct *parent, > + struct task_struct *child) > +{ > + int rc = 0; > + struct task_struct *walker = child; > + > + if (!parent || !child) > + return 0; > + > + rcu_read_lock(); > + if (!thread_group_leader(parent)) > + parent = rcu_dereference(parent->group_leader); > + while (walker->pid > 0) { > + if (!thread_group_leader(walker)) > + walker = rcu_dereference(walker->group_leader); > + if (walker == parent) { > + rc = 1; > + break; > + } > + walker = rcu_dereference(walker->real_parent); > + } > + rcu_read_unlock(); > + > + return rc; > +} > + > +/** > + * is_process_chrooted - process is inside chroot > + * @task: the task_struct of the process to be checked > + * > + * Returns 1 if task is inside chroot. > + */ > +static int is_process_chrooted(struct task_struct *task) { > + int rc = 0; > + struct hardchroot_info *entry; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) > + continue; > + if ((entry->task == task) || > + (task_is_descendant(entry->task, task))) { > + rc = 1; > + pr_info("HCRT: pid %d is already chrooted\n", > + task_pid_nr(entry->task)); > + break; > + } > + } > + rcu_read_unlock(); > + return rc; > +} > + > +/** > + * is_same_root - check if two tasks share the same root > + * @task1: the task_struct of the first task to be checked > + * @task2: the task_struct of the second task to be checked > + * > + * Returns 1 if tasks share the same root. > + */ > +static int is_same_root(struct task_struct *task1, struct task_struct > +*task2) { > + int rc = 0; > + struct hardchroot_info *entry; > + struct dentry *dentry1 = NULL; > + struct dentry *dentry2 = NULL; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) > + continue; > + if (entry->task == task1) > + dentry1 = entry->dentry; > + if (entry->task == task2) > + dentry2 = entry->dentry; > + } > + if (dentry1 && (dentry1 == dentry2)) { > + rc = 1; > + pr_info("HCRT: pids %d and %d have the same root\n", > + task_pid_nr(task1), task_pid_nr(task2)); > + } > + rcu_read_unlock(); > + return rc; > +} > + > +/** > + * is_inside_chroot - check if dentry and mount > + * are inside the current process fs root > + * @u_dentry: dentry to be checked > + * @u_mnt: mnt to be checked > + * > + * Returns 1 if dentry and mount are under fs root. > + */ > +int is_inside_chroot(const struct dentry *u_dentry, > + const struct vfsmount *u_mnt) > +{ > + struct path path; > + struct path currentroot; > + int ret = 0; > + > + path.dentry = (struct dentry *)u_dentry; > + path.mnt = (struct vfsmount *)u_mnt; > + get_fs_root(current->fs, ¤troot); > + if (path_is_under(&path, ¤troot)) > + ret = 1; > + else > + pr_info("HCRT: dentry %lu is outside current task %d root\n", > + d_backing_inode(u_dentry)->i_ino, > + task_pid_nr(current)); > + path_put(¤troot); > + return ret; > +} > + > +/** > + * hardchroot_path_chroot - validate chroot entry > + * @path contains the path structure. > + * > + * Returns 0 if chroot is allowed, -ve on error. > + */ > +static int hardchroot_path_chroot(const struct path *path) { > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, chroot: inode %lu and pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + > + get_task_struct(myself); > + if (is_process_chrooted(myself) && > + !is_inside_chroot(path->dentry, path->mnt)) { > + put_task_struct(myself); > + pr_info("HCRT, chroot denied: for inode %lu and pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + return -EACCES; > + } > + > + if (task_pid_nr(myself) > 1 && > + path->dentry != init_task.fs->root.dentry && > + path->dentry != myself->nsproxy->mnt_ns->root->mnt.mnt_root) { > + /* task is attempting to chroot, add it to the list */ > + rc = hardchroot_info_add(myself, path->dentry); > + pr_info("HCRT, chroot: adding %d to chrooted task list\n", > + task_pid_nr(myself)); > + } > + > + /* set the current working directory of all newly-chrooted > + * processes to the the root directory of the chroot > + */ > + set_fs_pwd(myself->fs, path); > + put_task_struct(myself); > + > + return rc; > +} > + > +/** > + * hardchroot_task_unshare - check if process is > + * allowed to unshare its namespaces > + * @unshare_flags flags > + * @new_fs contains the new fs_struct if created. > + * @new_fd contains the new files_struct if created. > + * @new_creds contains the new cred if created. > + * @new_nsproxy contains the new nsproxy if created. > + * > + * Returns 0 if unshare is allowed, -ve on error. > + */ > +static int hardchroot_task_unshare(unsigned long unshare_flags, > + const struct fs_struct *new_fs, > + const struct files_struct *new_fd, > + const struct cred *new_cred, > + const struct nsproxy *new_nsproxy) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + const struct nsproxy *tnsproxy = new_nsproxy; > + > + pr_info("HCRT, unshare: unshare_flags %lu and pid %d\n", > + unshare_flags, task_pid_nr(myself)); > + if (new_fs) > + pr_info("HCRT, unshare: new_fs->root.dentry inode%lu\n", > + d_backing_inode(new_fs->root.dentry)->i_ino); > + > + if (!tnsproxy) > + tnsproxy = myself->nsproxy; > + > + if (new_fs && task_pid_nr(myself) > 1 && > + new_fs->root.dentry != init_task.fs->root.dentry && > + new_fs->root.dentry != tnsproxy->mnt_ns->root->mnt.mnt_root) { > + rc = hardchroot_info_add(myself, new_fs->root.dentry); > + pr_info("HCRT, unshare: adding %d to chrooted task list\n", > + task_pid_nr(myself)); > + } > + > + return rc; > +} > + > +/** > + * hardchroot_sb_unsharefs - check if process is > + * allowed to unshare fs_struct > + * @path contains the path for the new root structure. > + * > + * Returns 0 if unsharefs is allowed, -ve on error. > + */ > +static int hardchroot_sb_unsharefs(const struct path *path) { > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, unsharefs: inode %lu and pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + > + if (task_pid_nr(myself) > 1 && > + path->dentry != init_task.fs->root.dentry && > + path->dentry != myself->nsproxy->mnt_ns->root->mnt.mnt_root) { > + rc = hardchroot_info_add(myself, path->dentry); > + pr_info("HCRT, unsharefs: adding %d to chrooted task list\n", > + task_pid_nr(myself)); > + } > + > + return rc; > +} > + > +/** > + * hardchroot_path_chmod - validate if chmod is allowed > + * @mnt contains the vfsmnt structure. > + * @mode contains DAC's mode > + * > + * Returns 0 if allowed, -ve on error. > + */ > +static int hardchroot_path_chmod(const struct path *path, umode_t > +mode) { > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, chmod: inode %lu, pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + > + /* allow chmod +s on directories, but not files */ > + if (!S_ISDIR(path->dentry->d_inode->i_mode) && ((mode & S_ISUID) || > + ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))) && > + is_process_chrooted(myself)) { > + pr_info("HCRT, chmod denied: inode %lu, pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + return -EACCES; > + } > + > + return rc; > + > +} > + > +/** > + * hardchroot_path_fchdir - validate if fchdir is allowed > + * @path: contains the path structure > + * > + * Returns 0 if allowed, -ve on error. > + */ > +static int hardchroot_path_fchdir(const struct path *path) { > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, fchdir: pid %d, path %lu", > + task_pid_nr(myself), > + d_backing_inode(path->dentry)->i_ino); > + > + if (!is_process_chrooted(myself)) > + return rc; > + if (!is_inside_chroot(path->dentry, path->mnt)) { > + pr_info("HCRT, fchdir denied: pid %d, path %lu", > + task_pid_nr(myself), > + d_backing_inode(path->dentry)->i_ino); > + return -EACCES; > + } > + > + return rc; > +} > + > +/** > + * hardchroot_path_fhandle - validate if converting > + * handle to path is allowed > + * @path: contains the path structure > + * > + * Returns 0 if allowed, -ve on error. > + */ > +static int hardchroot_path_fhandle(const struct path *path) { > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, fhandle: pid %d, path %lu", > + task_pid_nr(myself), > + d_backing_inode(path->dentry)->i_ino); > + > + if (is_process_chrooted(myself)) { > + pr_info("HCRT, fhandle denied: pid %d, path %lu", > + task_pid_nr(myself), > + d_backing_inode(path->dentry)->i_ino); > + return -EACCES; > + } > + > + return rc; > +} > + > +/** > + * hardchroot_task_setnice - check if setting nice is allowed > + * @task contains the task_struct of process. > + * @nice contains the new nice value. > + * > + * Return 0 if allowed, -ve on error. > + */ > +static int hardchroot_task_setnice(struct task_struct *task, int > +nice) { > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, setnice: current %d, nice %d, for pid %d and current nice %d\n", > + task_pid_nr(myself), nice, > + task_pid_nr(task), task_nice(task)); > + > + if (is_process_chrooted(myself) && (nice < task_nice(task))) { > + pr_info("HCRT, setnice denied: current %d, nice %d, for pid %d and current nice %d\n", > + task_pid_nr(myself), nice, > + task_pid_nr(task), task_nice(task)); > + return -EACCES; > + } > + return rc; > +} > + > +/** > + * hardchroot_path_mknod - check if mknod is allowed > + * @dir contains the path structure of parent of the new file. > + * @dentry contains the dentry structure of the new file. > + * @mode contains the mode of the new file. > + * @dev contains the undecoded device number. Use new_decode_dev() to > +get > + * the decoded device number. > + * > + * Return 0 if allowed, -ve on error. > + */ > +static int hardchroot_path_mknod(const struct path *dir, struct dentry *dentry, > + umode_t mode, unsigned int dev) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, mknod: dir %lu, mode %d pid %d\n", > + d_backing_inode(dir->dentry)->i_ino, > + mode, task_pid_nr(myself)); > + > + if (!S_ISFIFO(mode) && !S_ISREG(mode) && is_process_chrooted(myself)) { > + pr_info("HCRT, mknod denied: dir %lu, mode %d pid %d\n", > + d_backing_inode(dir->dentry)->i_ino, > + mode, task_pid_nr(myself)); > + return -EACCES; > + } > + return rc; > +} > + > +/** > + * hardchroot_sb_mount - check if mount is allowed > + * @dev_name contains the name for object being mounted. > + * @path contains the path for mount point object. > + * @type contains the filesystem type. > + * @flags contains the mount flags. > + * @data contains the filesystem-specific data. > + * > + * Return 0 if allowed, -ve on error. > + */ > +static int hardchroot_sb_mount(const char *dev_name, const struct path *path, > + const char *type, unsigned long flags, void *data) { > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, mount: dev %s, inode %lu, flags %lu pid %d\n", > + dev_name, d_backing_inode(path->dentry)->i_ino, > + flags, task_pid_nr(myself)); > + > + if (is_process_chrooted(myself)) { > + pr_info("HCRT, mount denied: dev %s, inode %lu, flags %lu, pid %d\n", > + dev_name, d_backing_inode(path->dentry)->i_ino, > + flags, task_pid_nr(myself)); > + return -EACCES; > + } > + return rc; > +} > + > +/** > + * hardchroot_sb_pivotroot - check if pivotroot is allowed > + * @old_path contains the path for the new location of the > + * current root (put_old). > + * @new_path contains the path for the new root (new_root). > + * > + * Return 0 if allowed, -ve on error. > + */ > +static int hardchroot_sb_pivotroot(const struct path *old_path, > + const struct path *new_path) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, pivotroot: old %lu, new %lu, pid %d\n", > + d_backing_inode(old_path->dentry)->i_ino, > + d_backing_inode(new_path->dentry)->i_ino, > + task_pid_nr(myself)); > + > + if (is_process_chrooted(myself)) { > + pr_info("HCRT, pivotroot denied: old %lu, new %lu, pid %d\n", > + d_backing_inode(old_path->dentry)->i_ino, > + d_backing_inode(new_path->dentry)->i_ino, > + task_pid_nr(myself)); > + return -EACCES; > + } > + return rc; > +} > + > +/** > + * hardchroot_shm_shmat - check if shmat is allowed > + * @shp contains the shared memory structure to be modified. > + * @shmaddr contains the address to attach memory region to. > + * @shmflg contains the operational flags. > + * > + * Return 0 if allowed, -ve on error. > + */ > +int hardchroot_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr, > + int shmflg) > +{ > + int rc = 0; > + struct task_struct *p; > + struct task_struct *myself = current; > + u64 st; > + time_t ct; > + > + pr_info("HCRT, shmat: shp %d, shmflg %d, pid %d\n", > + shp->shm_perm.id, shmflg, > + task_pid_nr(myself)); > + > + if (likely(!is_process_chrooted(myself))) > + return rc; > + > + rcu_read_lock(); > + read_lock(&tasklist_lock); > + > + p = find_task_by_vpid(shp->shm_cprid); > + if (p) { > + st = p->start_time; > + ct = shp->shm_ctim; > + if (time_before_eq((unsigned long)st, (unsigned long)ct)) { > + if (is_same_root(myself, p)) > + goto allow; > + else { > + read_unlock(&tasklist_lock); > + rcu_read_unlock(); > + pr_info("HCRT, shmat denied: shp %d, shmflg %d, pid %d\n", > + shp->shm_perm.id, shmflg, > + task_pid_nr(myself)); > + return -EACCES; > + } > + } > + /* creator exited, pid reuse, fall through to next check */ > + } > + p = find_task_by_vpid(shp->shm_lprid); > + if (p) { > + if (unlikely(!is_same_root(myself, p))) { > + read_unlock(&tasklist_lock); > + rcu_read_unlock(); > + pr_info("HCRT, shmat denied: shp %d, shmflg %d, pid %d\n", > + shp->shm_perm.id, shmflg, > + task_pid_nr(myself)); > + return -EACCES; > + } > + } > + > +allow: > + read_unlock(&tasklist_lock); > + rcu_read_unlock(); > + > + return rc; > + > + > +} > + > +static struct security_hook_list hardchroot_hooks[] = { > + LSM_HOOK_INIT(path_chroot, hardchroot_path_chroot), > + LSM_HOOK_INIT(path_chmod, hardchroot_path_chmod), > + LSM_HOOK_INIT(path_mknod, hardchroot_path_mknod), > + LSM_HOOK_INIT(path_fchdir, hardchroot_path_fchdir), > + LSM_HOOK_INIT(path_fhandle, hardchroot_path_fhandle), > + LSM_HOOK_INIT(sb_mount, hardchroot_sb_mount), > + LSM_HOOK_INIT(sb_pivotroot, hardchroot_sb_pivotroot), > + LSM_HOOK_INIT(sb_unsharefs, hardchroot_sb_unsharefs), > + LSM_HOOK_INIT(task_setnice, hardchroot_task_setnice), > + LSM_HOOK_INIT(task_free, hardchroot_task_free), > + LSM_HOOK_INIT(task_unshare, hardchroot_task_unshare), > + LSM_HOOK_INIT(shm_shmat, hardchroot_shm_shmat), }; > + > +static inline void hardchroot_init_sysctl(void) { } > + > +void __init hardchroot_add_hooks(void) { > + pr_info("Hardchroot: Getting stronger.\n"); > + security_add_hooks(hardchroot_hooks, ARRAY_SIZE(hardchroot_hooks)); > + hardchroot_init_sysctl(); > +} > diff --git a/security/security.c b/security/security.c index > 95487b9..ff65f06 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -61,6 +61,7 @@ int __init security_init(void) > capability_add_hooks(); > yama_add_hooks(); > loadpin_add_hooks(); > + hardchroot_add_hooks(); > > /* > * Load all the remaining security modules. > -- > 1.9.1 [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* [kernel-hardening] RE: [RFC] [PATCH 5/5] Hardchroot LSM 2016-07-29 11:44 ` [kernel-hardening] " Brad Spengler 2016-07-29 12:15 ` [kernel-hardening] " Reshetova, Elena @ 2016-07-29 12:25 ` Reshetova, Elena 1 sibling, 0 replies; 33+ messages in thread From: Reshetova, Elena @ 2016-07-29 12:25 UTC (permalink / raw) To: Brad Spengler Cc: kernel-hardening, linux-security-module, keescook, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 316 bytes --] >This again is completely unacceptable. >> + * Copyright (c) 2016, Intel Corporation And actually while I am waiting for an answer from our lawyers, how would you want such cases to be handled to make sure this doesn't happen again? Do you want to be in copyright also or? Please suggest, it would help greatly! [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM Elena Reshetova 2016-07-29 11:44 ` [kernel-hardening] " Brad Spengler @ 2016-07-29 18:53 ` Jann Horn 2016-07-29 19:20 ` Casey Schaufler 2016-07-30 6:10 ` Reshetova, Elena 1 sibling, 2 replies; 33+ messages in thread From: Jann Horn @ 2016-07-29 18:53 UTC (permalink / raw) To: kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova [-- Attachment #1: Type: text/plain, Size: 7532 bytes --] On Fri, Jul 29, 2016 at 10:34:40AM +0300, Elena Reshetova wrote: > This adds a new Hardchroot LSM that is intended to make > classical chroot more secure. It is based on > GRKERNSEC_CHROOT feature with necessary changes needed to > make it fit inside LSM. Currently not all GRKERNSEC_CHROOT > features are supported, but support is planned to be added > on granular basis. > > The credits for feature itself should go to the original > authors of GRKERNSEC_CHROOT. Since there is no way to share > security metadata between LSMs yet, the Hardchroot info task > management is done based on Yama LSM. When support is added, > the required info can be stored as part of task struct and it > can drastically simplify the internal management. I really don't like this series. First off: On Linux, as far as I know, chroots were never meant to be a security feature, and chroot "jails" break in a number of different ways. A lot of effort went into making bind mounts actually secure with reasonable performance, and I doubt that something like this can provide anything close to that, at least not without gigantic runtime overhead. Instead of making people believe that it's now okay to use chroot for security, I would very much prefer to keep the "never use this for security purposes" warning in the chroot() manpage and encourage people to use namespaces with bind mounts instead. I think that a somewhat more formal specification of what this is supposed to be good for might help. As far as I can tell, you haven't done anything to restrict the access to unix domain sockets or ptrace, which are probably the main tricks people would actually use to break out of a chroot "jail". Instead, you add support for some weird edgecase things like "if you are privileged and inside a chroot, you can renice processes outside the chroot". > Note: this feature currently prevents rtkit daemon from > normal operation. And it probably also prevents people from using chroot() for what it's actually intended to do. > +/** > + * is_process_chrooted - process is inside chroot > + * @task: the task_struct of the process to be checked > + * > + * Returns 1 if task is inside chroot. > + */ > +static int is_process_chrooted(struct task_struct *task) > +{ > + int rc = 0; > + struct hardchroot_info *entry; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { Wait, what? Is this really a function that has execution time linear in the number of running processes and that is called for every fchdir()? That seems slow. Did you benchmark this on a system with lots of processes? > +/** > + * is_same_root - check if two tasks share the same root > + * @task1: the task_struct of the first task to be checked > + * @task2: the task_struct of the second task to be checked > + * > + * Returns 1 if tasks share the same root. > + */ > +static int is_same_root(struct task_struct *task1, struct task_struct *task2) > +{ > + int rc = 0; > + struct hardchroot_info *entry; > + struct dentry *dentry1 = NULL; > + struct dentry *dentry2 = NULL; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) > + continue; > + if (entry->task == task1) > + dentry1 = entry->dentry; > + if (entry->task == task2) > + dentry2 = entry->dentry; > + } > + if (dentry1 && (dentry1 == dentry2)) { > + rc = 1; > + pr_info("HCRT: pids %d and %d have the same root\n", > + task_pid_nr(task1), task_pid_nr(task2)); > + } You're comparing dentries, not paths? So using the same directory through different bind mounts as root directories will still count as "same root" even though for chroot breakouts, it's like having different roots? > +/** > + * hardchroot_path_chroot - validate chroot entry > + * @path contains the path structure. > + * > + * Returns 0 if chroot is allowed, -ve on error. > + */ > +static int hardchroot_path_chroot(const struct path *path) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, chroot: inode %lu and pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + > + get_task_struct(myself); > + if (is_process_chrooted(myself) && > + !is_inside_chroot(path->dentry, path->mnt)) { > + put_task_struct(myself); > + pr_info("HCRT, chroot denied: for inode %lu and pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + return -EACCES; > + } > + > + if (task_pid_nr(myself) > 1 && > + path->dentry != init_task.fs->root.dentry && > + path->dentry != myself->nsproxy->mnt_ns->root->mnt.mnt_root) { > + /* task is attempting to chroot, add it to the list */ > + rc = hardchroot_info_add(myself, path->dentry); > + pr_info("HCRT, chroot: adding %d to chrooted task list\n", > + task_pid_nr(myself)); > + } > + > + /* set the current working directory of all newly-chrooted > + * processes to the the root directory of the chroot > + */ > + set_fs_pwd(myself->fs, path); That seems pretty broken, considering the possibility of open file descriptors pointing to the old root or so. And again, chroot only works if the current process is privileged. If it is, there are probably other ways to break out. > +/** > + * hardchroot_task_unshare - check if process is > + * allowed to unshare its namespaces > + * @unshare_flags flags > + * @new_fs contains the new fs_struct if created. > + * @new_fd contains the new files_struct if created. > + * @new_creds contains the new cred if created. > + * @new_nsproxy contains the new nsproxy if created. > + * > + * Returns 0 if unshare is allowed, -ve on error. > + */ > +static int hardchroot_task_unshare(unsigned long unshare_flags, > + const struct fs_struct *new_fs, > + const struct files_struct *new_fd, > + const struct cred *new_cred, > + const struct nsproxy *new_nsproxy) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + const struct nsproxy *tnsproxy = new_nsproxy; > + > + pr_info("HCRT, unshare: unshare_flags %lu and pid %d\n", > + unshare_flags, task_pid_nr(myself)); > + if (new_fs) > + pr_info("HCRT, unshare: new_fs->root.dentry inode%lu\n", > + d_backing_inode(new_fs->root.dentry)->i_ino); > + > + if (!tnsproxy) > + tnsproxy = myself->nsproxy; > + > + if (new_fs && task_pid_nr(myself) > 1 && > + new_fs->root.dentry != init_task.fs->root.dentry && Hm? You're accessing the fs_struct of init_task without any explicit locking? > +/** > + * hardchroot_shm_shmat - check if shmat is allowed > + * @shp contains the shared memory structure to be modified. > + * @shmaddr contains the address to attach memory region to. > + * @shmflg contains the operational flags. > + * > + * Return 0 if allowed, -ve on error. > + */ > +int hardchroot_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr, > + int shmflg) > +{ > + int rc = 0; > + struct task_struct *p; > + struct task_struct *myself = current; > + u64 st; > + time_t ct; > + > + pr_info("HCRT, shmat: shp %d, shmflg %d, pid %d\n", > + shp->shm_perm.id, shmflg, > + task_pid_nr(myself)); > + > + if (likely(!is_process_chrooted(myself))) > + return rc; > + > + rcu_read_lock(); > + read_lock(&tasklist_lock); > + > + p = find_task_by_vpid(shp->shm_cprid); Not exactly your fault, but this is broken when PID namespaces are involved. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM 2016-07-29 18:53 ` [kernel-hardening] " Jann Horn @ 2016-07-29 19:20 ` Casey Schaufler 2016-07-29 20:53 ` Jann Horn 2016-07-30 6:10 ` Reshetova, Elena 1 sibling, 1 reply; 33+ messages in thread From: Casey Schaufler @ 2016-07-29 19:20 UTC (permalink / raw) To: Jann Horn, kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova On 7/29/2016 11:53 AM, Jann Horn wrote: > On Fri, Jul 29, 2016 at 10:34:40AM +0300, Elena Reshetova wrote: >> This adds a new Hardchroot LSM that is intended to make >> classical chroot more secure. It is based on >> GRKERNSEC_CHROOT feature with necessary changes needed to >> make it fit inside LSM. Currently not all GRKERNSEC_CHROOT >> features are supported, but support is planned to be added >> on granular basis. >> >> The credits for feature itself should go to the original >> authors of GRKERNSEC_CHROOT. Since there is no way to share >> security metadata between LSMs yet, the Hardchroot info task >> management is done based on Yama LSM. When support is added, >> the required info can be stored as part of task struct and it >> can drastically simplify the internal management. > I really don't like this series. > > First off: On Linux, as far as I know, chroots were never meant > to be a security feature, This is a common misconception. When chroot was introduced circa 1979 (the exact date is subject to interpretation and your skill with sccs) security, especially in the form of protecting the system from accidental corruption, was an important concern. > and chroot "jails" break in a number > of different ways. All of which were introduced after the fact, and most of which have been introduced in spite of the objections of the security community. Even sockets, which are the biggest single breakage (followed closely by the process namespace and SVIPC) came along well after chroot and really should have taken the "root" into account. > A lot of effort went into making bind mounts > actually secure with reasonable performance, and I doubt that > something like this can provide anything close to that, at least > not without gigantic runtime overhead. Instead of making people > believe that it's now okay to use chroot for security, I would > very much prefer to keep the "never use this for security > purposes" warning in the chroot() manpage and encourage people > to use namespaces with bind mounts instead. There is merit to the argument that namespaces are better than chroot jails. Nonetheless, we're all aware of just how much legacy code we're going to have to deal with for the next forever, and some of that can benefit from this work. > > I think that a somewhat more formal specification of what this > is supposed to be good for might help. > > As far as I can tell, you haven't done anything to restrict > the access to unix domain sockets or ptrace, which are probably > the main tricks people would actually use to break out of a > chroot "jail". Instead, you add support for some weird edgecase > things like "if you are privileged and inside a chroot, you can > renice processes outside the chroot". > > >> Note: this feature currently prevents rtkit daemon from >> normal operation. > And it probably also prevents people from using chroot() for > what it's actually intended to do. > >> +/** >> + * is_process_chrooted - process is inside chroot >> + * @task: the task_struct of the process to be checked >> + * >> + * Returns 1 if task is inside chroot. >> + */ >> +static int is_process_chrooted(struct task_struct *task) >> +{ >> + int rc = 0; >> + struct hardchroot_info *entry; >> + >> + rcu_read_lock(); >> + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > Wait, what? Is this really a function that has execution time linear > in the number of running processes and that is called for every > fchdir()? That seems slow. Did you benchmark this on a system with > lots of processes? > > >> +/** >> + * is_same_root - check if two tasks share the same root >> + * @task1: the task_struct of the first task to be checked >> + * @task2: the task_struct of the second task to be checked >> + * >> + * Returns 1 if tasks share the same root. >> + */ >> +static int is_same_root(struct task_struct *task1, struct task_struct *task2) >> +{ >> + int rc = 0; >> + struct hardchroot_info *entry; >> + struct dentry *dentry1 = NULL; >> + struct dentry *dentry2 = NULL; >> + >> + rcu_read_lock(); >> + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { >> + if (entry->invalid) >> + continue; >> + if (entry->task == task1) >> + dentry1 = entry->dentry; >> + if (entry->task == task2) >> + dentry2 = entry->dentry; >> + } >> + if (dentry1 && (dentry1 == dentry2)) { >> + rc = 1; >> + pr_info("HCRT: pids %d and %d have the same root\n", >> + task_pid_nr(task1), task_pid_nr(task2)); >> + } > You're comparing dentries, not paths? So using the same directory > through different bind mounts as root directories will still count > as "same root" even though for chroot breakouts, it's like having > different roots? > > >> +/** >> + * hardchroot_path_chroot - validate chroot entry >> + * @path contains the path structure. >> + * >> + * Returns 0 if chroot is allowed, -ve on error. >> + */ >> +static int hardchroot_path_chroot(const struct path *path) >> +{ >> + int rc = 0; >> + struct task_struct *myself = current; >> + >> + pr_info("HCRT, chroot: inode %lu and pid %d\n", >> + d_backing_inode(path->dentry)->i_ino, >> + task_pid_nr(myself)); >> + >> + get_task_struct(myself); >> + if (is_process_chrooted(myself) && >> + !is_inside_chroot(path->dentry, path->mnt)) { >> + put_task_struct(myself); >> + pr_info("HCRT, chroot denied: for inode %lu and pid %d\n", >> + d_backing_inode(path->dentry)->i_ino, >> + task_pid_nr(myself)); >> + return -EACCES; >> + } >> + >> + if (task_pid_nr(myself) > 1 && >> + path->dentry != init_task.fs->root.dentry && >> + path->dentry != myself->nsproxy->mnt_ns->root->mnt.mnt_root) { >> + /* task is attempting to chroot, add it to the list */ >> + rc = hardchroot_info_add(myself, path->dentry); >> + pr_info("HCRT, chroot: adding %d to chrooted task list\n", >> + task_pid_nr(myself)); >> + } >> + >> + /* set the current working directory of all newly-chrooted >> + * processes to the the root directory of the chroot >> + */ >> + set_fs_pwd(myself->fs, path); > That seems pretty broken, considering the possibility of open > file descriptors pointing to the old root or so. > > And again, chroot only works if the current process is privileged. > If it is, there are probably other ways to break out. > > >> +/** >> + * hardchroot_task_unshare - check if process is >> + * allowed to unshare its namespaces >> + * @unshare_flags flags >> + * @new_fs contains the new fs_struct if created. >> + * @new_fd contains the new files_struct if created. >> + * @new_creds contains the new cred if created. >> + * @new_nsproxy contains the new nsproxy if created. >> + * >> + * Returns 0 if unshare is allowed, -ve on error. >> + */ >> +static int hardchroot_task_unshare(unsigned long unshare_flags, >> + const struct fs_struct *new_fs, >> + const struct files_struct *new_fd, >> + const struct cred *new_cred, >> + const struct nsproxy *new_nsproxy) >> +{ >> + int rc = 0; >> + struct task_struct *myself = current; >> + const struct nsproxy *tnsproxy = new_nsproxy; >> + >> + pr_info("HCRT, unshare: unshare_flags %lu and pid %d\n", >> + unshare_flags, task_pid_nr(myself)); >> + if (new_fs) >> + pr_info("HCRT, unshare: new_fs->root.dentry inode%lu\n", >> + d_backing_inode(new_fs->root.dentry)->i_ino); >> + >> + if (!tnsproxy) >> + tnsproxy = myself->nsproxy; >> + >> + if (new_fs && task_pid_nr(myself) > 1 && >> + new_fs->root.dentry != init_task.fs->root.dentry && > Hm? You're accessing the fs_struct of init_task without any explicit locking? > > >> +/** >> + * hardchroot_shm_shmat - check if shmat is allowed >> + * @shp contains the shared memory structure to be modified. >> + * @shmaddr contains the address to attach memory region to. >> + * @shmflg contains the operational flags. >> + * >> + * Return 0 if allowed, -ve on error. >> + */ >> +int hardchroot_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr, >> + int shmflg) >> +{ >> + int rc = 0; >> + struct task_struct *p; >> + struct task_struct *myself = current; >> + u64 st; >> + time_t ct; >> + >> + pr_info("HCRT, shmat: shp %d, shmflg %d, pid %d\n", >> + shp->shm_perm.id, shmflg, >> + task_pid_nr(myself)); >> + >> + if (likely(!is_process_chrooted(myself))) >> + return rc; >> + >> + rcu_read_lock(); >> + read_lock(&tasklist_lock); >> + >> + p = find_task_by_vpid(shp->shm_cprid); > Not exactly your fault, but this is broken when PID namespaces are involved. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM 2016-07-29 19:20 ` Casey Schaufler @ 2016-07-29 20:53 ` Jann Horn 2016-07-29 21:10 ` Casey Schaufler 0 siblings, 1 reply; 33+ messages in thread From: Jann Horn @ 2016-07-29 20:53 UTC (permalink / raw) To: Casey Schaufler Cc: kernel-hardening, linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova [-- Attachment #1: Type: text/plain, Size: 4043 bytes --] On Fri, Jul 29, 2016 at 12:20:56PM -0700, Casey Schaufler wrote: > On 7/29/2016 11:53 AM, Jann Horn wrote: > > On Fri, Jul 29, 2016 at 10:34:40AM +0300, Elena Reshetova wrote: > >> This adds a new Hardchroot LSM that is intended to make > >> classical chroot more secure. It is based on > >> GRKERNSEC_CHROOT feature with necessary changes needed to > >> make it fit inside LSM. Currently not all GRKERNSEC_CHROOT > >> features are supported, but support is planned to be added > >> on granular basis. > >> > >> The credits for feature itself should go to the original > >> authors of GRKERNSEC_CHROOT. Since there is no way to share > >> security metadata between LSMs yet, the Hardchroot info task > >> management is done based on Yama LSM. When support is added, > >> the required info can be stored as part of task struct and it > >> can drastically simplify the internal management. > > I really don't like this series. > > > > First off: On Linux, as far as I know, chroots were never meant > > to be a security feature, > > This is a common misconception. When chroot was introduced circa 1979 > (the exact date is subject to interpretation and your skill with sccs) > security, especially in the form of protecting the system from > accidental corruption, was an important concern. I'm explicitly talking about the situation *on Linux*. I don't know much about old UNIX variants, and I don't think that they are very relevant here - IMO, what matters here are what chroot() was designed for *on Linux* and how it was treated during the development of the kernel, because that is what influences how easy it is going to be to add that stuff to Linux today. And when you look at Linux 0.10, you'll see that already back then, sys_chroot() just updated current->root; sending signals to other processes, setting the system time and so on just did UID checks. > > and chroot "jails" break in a number > > of different ways. > > All of which were introduced after the fact, and most of which > have been introduced in spite of the objections of the security > community. Even sockets, which are the biggest single breakage > (followed closely by the process namespace and SVIPC) came along > well after chroot and really should have taken the "root" into > account. Namespaces on Linux actually take chroots into account - you can't create a new namespace if you're unprivileged and inside a chroot, see commit 3151527ee0. I'm not sure whether that was added before or after unprivileged user namespaces were enabled. > > A lot of effort went into making bind mounts > > actually secure with reasonable performance, and I doubt that > > something like this can provide anything close to that, at least > > not without gigantic runtime overhead. Instead of making people > > believe that it's now okay to use chroot for security, I would > > very much prefer to keep the "never use this for security > > purposes" warning in the chroot() manpage and encourage people > > to use namespaces with bind mounts instead. > > There is merit to the argument that namespaces are better than > chroot jails. Nonetheless, we're all aware of just how much > legacy code we're going to have to deal with for the next > forever, and some of that can benefit from this work. Eh. For that, you could also make a shim that turns chroot into namespace creation automatically - either as a libc feature or as a personality flag in the kernel. The biggest issue with this would probably be dealing with multithreaded processes that call chroot() while being multithreaded - in that case, a personality flag would have the advantage of allowing the kernel to have a variant of unshare() that synchronizes new user and mount namespaces across all threads. That approach would probably be less of a maintenance and performance burden and have less security issues popping up over time compared to attempting to have two orthogonal filesystem sandboxing implementations. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM 2016-07-29 20:53 ` Jann Horn @ 2016-07-29 21:10 ` Casey Schaufler 2016-07-29 21:50 ` Jann Horn 0 siblings, 1 reply; 33+ messages in thread From: Casey Schaufler @ 2016-07-29 21:10 UTC (permalink / raw) To: Jann Horn Cc: kernel-hardening, linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova On 7/29/2016 1:53 PM, Jann Horn wrote: > On Fri, Jul 29, 2016 at 12:20:56PM -0700, Casey Schaufler wrote: >> On 7/29/2016 11:53 AM, Jann Horn wrote: >>> On Fri, Jul 29, 2016 at 10:34:40AM +0300, Elena Reshetova wrote: >>>> This adds a new Hardchroot LSM that is intended to make >>>> classical chroot more secure. It is based on >>>> GRKERNSEC_CHROOT feature with necessary changes needed to >>>> make it fit inside LSM. Currently not all GRKERNSEC_CHROOT >>>> features are supported, but support is planned to be added >>>> on granular basis. >>>> >>>> The credits for feature itself should go to the original >>>> authors of GRKERNSEC_CHROOT. Since there is no way to share >>>> security metadata between LSMs yet, the Hardchroot info task >>>> management is done based on Yama LSM. When support is added, >>>> the required info can be stored as part of task struct and it >>>> can drastically simplify the internal management. >>> I really don't like this series. >>> >>> First off: On Linux, as far as I know, chroots were never meant >>> to be a security feature, >> This is a common misconception. When chroot was introduced circa 1979 >> (the exact date is subject to interpretation and your skill with sccs) >> security, especially in the form of protecting the system from >> accidental corruption, was an important concern. > I'm explicitly talking about the situation *on Linux*. I don't > know much about old UNIX variants, and I don't think that they > are very relevant here - IMO, what matters here are what chroot() > was designed for *on Linux* and how it was treated during the > development of the kernel, because that is what influences how > easy it is going to be to add that stuff to Linux today. Sorry, but you can't separate the Linux behavior from the UNIX behavior. The Linux behavior came from the UNIX behavior. Although I wasn't in Finland at the time, I think that it's pretty safe to conclude that the "design" of chroot for Linux was pretty well limited to duplicating what it did on UNIX. > And when you look at Linux 0.10, you'll see that already back > then, sys_chroot() just updated current->root; sending signals > to other processes, setting the system time and so on just did > UID checks. > > >>> and chroot "jails" break in a number >>> of different ways. >> All of which were introduced after the fact, and most of which >> have been introduced in spite of the objections of the security >> community. Even sockets, which are the biggest single breakage >> (followed closely by the process namespace and SVIPC) came along >> well after chroot and really should have taken the "root" into >> account. > Namespaces on Linux actually take chroots into account - you can't > create a new namespace if you're unprivileged and inside a chroot, > see commit 3151527ee0. I'm not sure whether that was added before > or after unprivileged user namespaces were enabled. > > >>> A lot of effort went into making bind mounts >>> actually secure with reasonable performance, and I doubt that >>> something like this can provide anything close to that, at least >>> not without gigantic runtime overhead. Instead of making people >>> believe that it's now okay to use chroot for security, I would >>> very much prefer to keep the "never use this for security >>> purposes" warning in the chroot() manpage and encourage people >>> to use namespaces with bind mounts instead. >> There is merit to the argument that namespaces are better than >> chroot jails. Nonetheless, we're all aware of just how much >> legacy code we're going to have to deal with for the next >> forever, and some of that can benefit from this work. > Eh. For that, you could also make a shim that turns chroot into > namespace creation automatically Right. Why carry a tent when you can pull a 24' Airsteam trailer? :) > - either as a libc feature or > as a personality flag in the kernel. The biggest issue with this > would probably be dealing with multithreaded processes that call > chroot() while being multithreaded - in that case, a personality > flag would have the advantage of allowing the kernel to have a > variant of unshare() that synchronizes new user and mount > namespaces across all threads. > > That approach would probably be less of a maintenance and > performance burden and have less security issues popping up over > time compared to attempting to have two orthogonal filesystem > sandboxing implementations. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM 2016-07-29 21:10 ` Casey Schaufler @ 2016-07-29 21:50 ` Jann Horn 0 siblings, 0 replies; 33+ messages in thread From: Jann Horn @ 2016-07-29 21:50 UTC (permalink / raw) To: Casey Schaufler Cc: kernel-hardening, linux-security-module, keescook, spender, jmorris, casey.schaufler, michael.leibowitz, william.c.roberts, Elena Reshetova [-- Attachment #1: Type: text/plain, Size: 3091 bytes --] On Fri, Jul 29, 2016 at 02:10:31PM -0700, Casey Schaufler wrote: > On 7/29/2016 1:53 PM, Jann Horn wrote: > > On Fri, Jul 29, 2016 at 12:20:56PM -0700, Casey Schaufler wrote: > >> On 7/29/2016 11:53 AM, Jann Horn wrote: [...] > > And when you look at Linux 0.10, you'll see that already back > > then, sys_chroot() just updated current->root; sending signals > > to other processes, setting the system time and so on just did > > UID checks. > > > > > >>> and chroot "jails" break in a number > >>> of different ways. > >> All of which were introduced after the fact, and most of which > >> have been introduced in spite of the objections of the security > >> community. Even sockets, which are the biggest single breakage > >> (followed closely by the process namespace and SVIPC) came along > >> well after chroot and really should have taken the "root" into > >> account. > > Namespaces on Linux actually take chroots into account - you can't > > create a new namespace if you're unprivileged and inside a chroot, > > see commit 3151527ee0. I'm not sure whether that was added before > > or after unprivileged user namespaces were enabled. > > > > > >>> A lot of effort went into making bind mounts > >>> actually secure with reasonable performance, and I doubt that > >>> something like this can provide anything close to that, at least > >>> not without gigantic runtime overhead. Instead of making people > >>> believe that it's now okay to use chroot for security, I would > >>> very much prefer to keep the "never use this for security > >>> purposes" warning in the chroot() manpage and encourage people > >>> to use namespaces with bind mounts instead. > >> There is merit to the argument that namespaces are better than > >> chroot jails. Nonetheless, we're all aware of just how much > >> legacy code we're going to have to deal with for the next > >> forever, and some of that can benefit from this work. > > Eh. For that, you could also make a shim that turns chroot into > > namespace creation automatically > > Right. Why carry a tent when you can pull a 24' Airsteam trailer? :) Because that "tent" would be a lot of messy parts all over your house while the "Airsteam trailer" could maybe be parked outside and wouldn't be in the way every time you want to make coffee? And besides, since lots of people are going to build their own trailers, it might make sense to have your own so that you can show people what a proper trailer looks like. (And if multithreaded namespace creation is needed for this, it might be nice to have anyway, considering that people want to use namespaces from languages like Go, where they currently have to implement the unsharing in C before the runtime starts up - I think that was e.g. the case in subgraph's oz. This could probably be implemented similar to grsecurity's GRKERNSEC_SETXID. I think this isn't exactly a priority at the moment, but as people start using memory-safe languages with GC for low-level stuff like this, it might become more important - I don't know.) [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM 2016-07-29 18:53 ` [kernel-hardening] " Jann Horn 2016-07-29 19:20 ` Casey Schaufler @ 2016-07-30 6:10 ` Reshetova, Elena 1 sibling, 0 replies; 33+ messages in thread From: Reshetova, Elena @ 2016-07-30 6:10 UTC (permalink / raw) To: Jann Horn, kernel-hardening Cc: linux-security-module, keescook, spender, jmorris, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 10801 bytes --] First of all, thank you very much for the review! I am still learning with many things and your feedback is very much appreciated! >First off: On Linux, as far as I know, chroots were never meant to be a security feature, and chroot "jails" break in a number of different ways. A lot of effort went into making bind mounts actually secure with reasonable performance, and I doubt that >something like this can provide anything close to that, at least not without gigantic runtime overhead. Instead of making people believe that it's now okay to use chroot for security, I would very much prefer to keep the "never use this for security >purposes" warning in the chroot() manpage and encourage people to use namespaces with bind mounts instead. I am aware that chroot jail should not be used for security purposes, but reality is that it is *used* in many places at least with some intention of getting a better security. I don't believe there is any person nowadays who believes that chroot is secure, but because it is super light-weight (even compare with namespaces setup) and because of legacy concerns, it is still there. I don't also claim that this LSM make chroot *fully* secure (as you mentioned below some important features are still missing) and it would never be as strong as full namespace-based solution, but practical problem with namespaces is that it isn't that easy to setup them up properly and possibility of making configuration mistake is still quite high. About the performance overhead, I actually don't believe it would be that gigantic (especially when we get proper LSM stacking in place that eliminates the need to store internal list and do searches), but of course any LSM has performance impact, especially with the hooks on the filesystem. And don't take me wrong, I do personally like namespaces very much. Just usecases I am trying to address with this are very different. > I think that a somewhat more formal specification of what this is supposed to be good for might help. Yes, I think this make sense. It would clarify on when you should use this (in cases when you do need to use chroot only but want to make it a degree better). >As far as I can tell, you haven't done anything to restrict the access to unix domain sockets or ptrace, which are probably the main tricks people would actually use to break out of a chroot "jail". Instead, you add support for some weird edgecase things like >"if you are privileged and inside a chroot, you can renice processes outside the chroot". Supporting unix domain sockets is in plans, I was looking into all GRKERNSEC features and working them one by one. I first processed all features that can be easily moved into existing LSM hooks, and nice was one of them. Then I started adding new hooks for the rest of features. But at some point I just exceeded my threshold of confidence with all the changes and hooks I am making (unix domain sockets might need further new hooks), so I wanted to bring this out for review earlier than later. What about ptrace? I am assume that we have Yama in place for this :) > Note: this feature currently prevents rtkit daemon from > normal operation. >And it probably also prevents people from using chroot() for what it's actually intended to do. I would not go as far as that. How many cases apart from rtkit daemon who sets priorities for other processes you know that would suffer from this behaviour? Again, this was an easy feature that fits into existing hook and can be fully removed if people don't see a value in it. It is certainly not the main part of it. > +/** > + * is_process_chrooted - process is inside chroot > + * @task: the task_struct of the process to be checked > + * > + * Returns 1 if task is inside chroot. > + */ > +static int is_process_chrooted(struct task_struct *task) { > + int rc = 0; > + struct hardchroot_info *entry; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { >Wait, what? Is this really a function that has execution time linear in the number of running processes and that is called for every fchdir()? That seems slow. Did you benchmark this on a system with lots of processes? I haven't done any benchmarking on this yet, but remember that hardchroot_infos will be quite a short list, which equals number of processes that explicitly entered chroot (not their children, only first parent process to do it). In Ubuntu and Fedora for example, after the full boot, I only had 3 such processes in the list, so this gives the size of list I am expecting: 10-20 items absolutely max (normally under 10), not hundreds. So, list iteration is quite fast. So, it doesn't depend on the number of system processes, but it does depend on number of processes that entered chroot. > +/** > + * is_same_root - check if two tasks share the same root > + * @task1: the task_struct of the first task to be checked > + * @task2: the task_struct of the second task to be checked > + * > + * Returns 1 if tasks share the same root. > + */ > +static int is_same_root(struct task_struct *task1, struct task_struct > +*task2) { > + int rc = 0; > + struct hardchroot_info *entry; > + struct dentry *dentry1 = NULL; > + struct dentry *dentry2 = NULL; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &hardchroot_infos, node) { > + if (entry->invalid) > + continue; > + if (entry->task == task1) > + dentry1 = entry->dentry; > + if (entry->task == task2) > + dentry2 = entry->dentry; > + } > + if (dentry1 && (dentry1 == dentry2)) { > + rc = 1; > + pr_info("HCRT: pids %d and %d have the same root\n", > + task_pid_nr(task1), task_pid_nr(task2)); > + } >You're comparing dentries, not paths? So using the same directory through different bind mounts as root directories will still count as "same root" even though for chroot breakouts, it's like having different roots? Bind mounts are tricky in this case. From one side yes, it is yes like having two different roots, but then how to handle legitimate cases when people bind mount things into chroot to be used there? Blocking them would make bind mounts not supported inside chroot, which is not smth that should be done IMO. > +/** > + * hardchroot_path_chroot - validate chroot entry > + * @path contains the path structure. > + * > + * Returns 0 if chroot is allowed, -ve on error. > + */ > +static int hardchroot_path_chroot(const struct path *path) { > + int rc = 0; > + struct task_struct *myself = current; > + > + pr_info("HCRT, chroot: inode %lu and pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + > + get_task_struct(myself); > + if (is_process_chrooted(myself) && > + !is_inside_chroot(path->dentry, path->mnt)) { > + put_task_struct(myself); > + pr_info("HCRT, chroot denied: for inode %lu and pid %d\n", > + d_backing_inode(path->dentry)->i_ino, > + task_pid_nr(myself)); > + return -EACCES; > + } > + > + if (task_pid_nr(myself) > 1 && > + path->dentry != init_task.fs->root.dentry && > + path->dentry != myself->nsproxy->mnt_ns->root->mnt.mnt_root) { > + /* task is attempting to chroot, add it to the list */ > + rc = hardchroot_info_add(myself, path->dentry); > + pr_info("HCRT, chroot: adding %d to chrooted task list\n", > + task_pid_nr(myself)); > + } > + > + /* set the current working directory of all newly-chrooted > + * processes to the the root directory of the chroot > + */ > + set_fs_pwd(myself->fs, path); >That seems pretty broken, considering the possibility of open file descriptors pointing to the old root or so. Again, the main use case here is that we have a good process that enters chroot to minimize the damage in case it gets exploited some later time in life. A good process is supposed to close its file descriptors pointing to the old root before entering chroot, isn't it? >And again, chroot only works if the current process is privileged. >If it is, there are probably other ways to break out. You need to have privilege to *enter* chroot, but if you are good process, you will drop the privilege after you did your setup. > +/** > + * hardchroot_task_unshare - check if process is > + * allowed to unshare its namespaces > + * @unshare_flags flags > + * @new_fs contains the new fs_struct if created. > + * @new_fd contains the new files_struct if created. > + * @new_creds contains the new cred if created. > + * @new_nsproxy contains the new nsproxy if created. > + * > + * Returns 0 if unshare is allowed, -ve on error. > + */ > +static int hardchroot_task_unshare(unsigned long unshare_flags, > + const struct fs_struct *new_fs, > + const struct files_struct *new_fd, > + const struct cred *new_cred, > + const struct nsproxy *new_nsproxy) > +{ > + int rc = 0; > + struct task_struct *myself = current; > + const struct nsproxy *tnsproxy = new_nsproxy; > + > + pr_info("HCRT, unshare: unshare_flags %lu and pid %d\n", > + unshare_flags, task_pid_nr(myself)); > + if (new_fs) > + pr_info("HCRT, unshare: new_fs->root.dentry inode%lu\n", > + d_backing_inode(new_fs->root.dentry)->i_ino); > + > + if (!tnsproxy) > + tnsproxy = myself->nsproxy; > + > + if (new_fs && task_pid_nr(myself) > 1 && > + new_fs->root.dentry != init_task.fs->root.dentry && >Hm? You're accessing the fs_struct of init_task without any explicit locking? I am quite new to the kernel development and locking. I was reading a lot recently about locking to understand where it is needed and where not, but mistakes are obviously still coming. I guess I would need to do smth like: task_lock(&init_task) before and task_unlock(&init_task) after. > +/** > + * hardchroot_shm_shmat - check if shmat is allowed > + * @shp contains the shared memory structure to be modified. > + * @shmaddr contains the address to attach memory region to. > + * @shmflg contains the operational flags. > + * > + * Return 0 if allowed, -ve on error. > + */ > +int hardchroot_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr, > + int shmflg) > +{ > + int rc = 0; > + struct task_struct *p; > + struct task_struct *myself = current; > + u64 st; > + time_t ct; > + > + pr_info("HCRT, shmat: shp %d, shmflg %d, pid %d\n", > + shp->shm_perm.id, shmflg, > + task_pid_nr(myself)); > + > + if (likely(!is_process_chrooted(myself))) > + return rc; > + > + rcu_read_lock(); > + read_lock(&tasklist_lock); > + > + p = find_task_by_vpid(shp->shm_cprid); >Not exactly your fault, but this is broken when PID namespaces are involved. Yes, this has been bothering me also... Grsecurity has their own version of this function (find_task_by_vpid_unrestricted) instead that tried to do global search, I guess I should think on doing smth similar, but I am wondering if this can be done better. I will look into this for the next version. [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* [kernel-hardening] Re: [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks 2016-07-29 7:34 [kernel-hardening] [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks Elena Reshetova ` (4 preceding siblings ...) 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM Elena Reshetova @ 2016-08-03 6:36 ` James Morris 2016-08-05 7:53 ` [kernel-hardening] " Reshetova, Elena 5 siblings, 1 reply; 33+ messages in thread From: James Morris @ 2016-08-03 6:36 UTC (permalink / raw) To: Elena Reshetova Cc: kernel-hardening, linux-security-module, keescook, spender, casey.schaufler, michael.leibowitz, william.c.roberts On Fri, 29 Jul 2016, Elena Reshetova wrote: > fs/fhandle.c | 5 + > fs/fs_struct.c | 7 +- > fs/namespace.c | 15 +- > fs/open.c | 3 + > include/linux/lsm_hooks.h | 37 ++ > include/linux/security.h | 19 + > kernel/fork.c | 5 + > security/Kconfig | 1 + > security/Makefile | 2 + > security/hardchroot/Kconfig | 10 + > security/hardchroot/Makefile | 3 + > security/hardchroot/hardchroot_lsm.c | 654 +++++++++++++++++++++++++++++++++++ > security/security.c | 30 ++ > 13 files changed, 786 insertions(+), 5 deletions(-) > create mode 100644 security/hardchroot/Kconfig > create mode 100644 security/hardchroot/Makefile > create mode 100644 security/hardchroot/hardchroot_lsm.c Please cc: Al Viro on these changes. -- James Morris <jmorris@namei.org> ^ permalink raw reply [flat|nested] 33+ messages in thread
* [kernel-hardening] RE: [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks 2016-08-03 6:36 ` [kernel-hardening] Re: [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks James Morris @ 2016-08-05 7:53 ` Reshetova, Elena 0 siblings, 0 replies; 33+ messages in thread From: Reshetova, Elena @ 2016-08-05 7:53 UTC (permalink / raw) To: James Morris Cc: kernel-hardening, linux-security-module, keescook, spender, Schaufler, Casey, Leibowitz, Michael, Roberts, William C [-- Attachment #1: Type: text/plain, Size: 1116 bytes --] Subject: Re: [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks On Fri, 29 Jul 2016, Elena Reshetova wrote: > fs/fhandle.c | 5 + > fs/fs_struct.c | 7 +- > fs/namespace.c | 15 +- > fs/open.c | 3 + > include/linux/lsm_hooks.h | 37 ++ > include/linux/security.h | 19 + > kernel/fork.c | 5 + > security/Kconfig | 1 + > security/Makefile | 2 + > security/hardchroot/Kconfig | 10 + > security/hardchroot/Makefile | 3 + > security/hardchroot/hardchroot_lsm.c | 654 +++++++++++++++++++++++++++++++++++ > security/security.c | 30 ++ > 13 files changed, 786 insertions(+), 5 deletions(-) create mode > 100644 security/hardchroot/Kconfig create mode 100644 > security/hardchroot/Makefile create mode 100644 > security/hardchroot/hardchroot_lsm.c > Please cc: Al Viro on these changes. Sure, will do it on the next patch version. Have quite a few things to fix first. [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 7586 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2016-08-05 7:53 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-29 7:34 [kernel-hardening] [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks Elena Reshetova 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks Elena Reshetova 2016-07-29 18:12 ` Jann Horn 2016-07-31 10:55 ` Reshetova, Elena 2016-07-31 12:02 ` Jann Horn 2016-07-31 18:28 ` Reshetova, Elena 2016-07-31 21:23 ` Jann Horn 2016-08-01 8:38 ` Reshetova, Elena 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 2/5] task_unshare LSM hook Elena Reshetova 2016-07-29 17:58 ` Jann Horn 2016-07-29 18:17 ` Reshetova, Elena 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs " Elena Reshetova 2016-07-29 18:02 ` Jann Horn 2016-07-29 18:09 ` Reshetova, Elena 2016-07-29 18:15 ` Jann Horn 2016-07-29 18:19 ` Reshetova, Elena 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install() Elena Reshetova 2016-07-29 18:11 ` Jann Horn 2016-07-31 10:39 ` Reshetova, Elena 2016-07-31 11:29 ` Jann Horn 2016-08-01 9:26 ` Reshetova, Elena 2016-07-29 7:34 ` [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM Elena Reshetova 2016-07-29 11:44 ` [kernel-hardening] " Brad Spengler 2016-07-29 12:15 ` [kernel-hardening] " Reshetova, Elena 2016-07-29 12:25 ` Reshetova, Elena 2016-07-29 18:53 ` [kernel-hardening] " Jann Horn 2016-07-29 19:20 ` Casey Schaufler 2016-07-29 20:53 ` Jann Horn 2016-07-29 21:10 ` Casey Schaufler 2016-07-29 21:50 ` Jann Horn 2016-07-30 6:10 ` Reshetova, Elena 2016-08-03 6:36 ` [kernel-hardening] Re: [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks James Morris 2016-08-05 7:53 ` [kernel-hardening] " Reshetova, Elena
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.