[kernel-hardening] [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks

[kernel-hardening] [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks

[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

[kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks

[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

[kernel-hardening] [RFC] [PATCH 2/5] task_unshare LSM hook

[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

[kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs LSM hook

[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

[kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install()

[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

[kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM

[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, &currentroot);
+	if (path_is_under(&path, &currentroot))
+		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(&currentroot);
+	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

[kernel-hardening] Re: [RFC] [PATCH 5/5] Hardchroot LSM

[Brad Spengler]

[-- 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, &currentroot);
> +	if (path_is_under(&path, &currentroot))
> +		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(&currentroot);
> +	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 --]

[kernel-hardening] RE: [RFC] [PATCH 5/5] Hardchroot LSM

[Reshetova, Elena]

[-- 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, &currentroot);
> +	if (path_is_under(&path, &currentroot))
> +		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(&currentroot);
> +	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 --]

[kernel-hardening] RE: [RFC] [PATCH 5/5] Hardchroot LSM

[Reshetova, Elena]

[-- 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 --]

Re: [kernel-hardening] [RFC] [PATCH 2/5] task_unshare LSM hook

[Jann Horn]

[-- 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 --]

Re: [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs LSM hook

[Jann Horn]

[-- 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 --]

RE: [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs LSM hook

[Reshetova, Elena]

[-- 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 --]

Re: [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install()

[Jann Horn]

[-- 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 --]

Re: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks

[Jann Horn]

[-- 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 --]

Re: [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs LSM hook

[Jann Horn]

[-- 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 --]

RE: [kernel-hardening] [RFC] [PATCH 2/5] task_unshare LSM hook

[Reshetova, Elena]

[-- 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 --]

RE: [kernel-hardening] [RFC] [PATCH 3/5] sb_unsharefs LSM hook

[Reshetova, Elena]

[-- 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 --]

Re: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM

[Jann Horn]

[-- 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 --]

Re: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM

[Casey Schaufler]

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.

Re: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM

[Jann Horn]

[-- 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 --]

Re: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM

[Casey Schaufler]

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.

Re: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM

[Jann Horn]

[-- 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 --]

RE: [kernel-hardening] [RFC] [PATCH 5/5] Hardchroot LSM

[Reshetova, Elena]

[-- 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 --]

RE: [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install()

[Reshetova, Elena]

[-- 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 --]

RE: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks

[Reshetova, Elena]

[-- 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 --]

Re: [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install()

[Jann Horn]

[-- 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 --]

Re: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks

[Jann Horn]

[-- 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 --]

RE: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks

[Reshetova, Elena]

[-- 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 --]

Re: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks

[Jann Horn]

[-- 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 --]

RE: [kernel-hardening] [RFC] [PATCH 1/5] path_fchdir and path_fhandle LSM hooks

[Reshetova, Elena]

[-- 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 --]

RE: [kernel-hardening] [RFC] [PATCH 4/5] invoke path_chroot() LSM hook on mntns_install()

[Reshetova, Elena]

[-- 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 --]

[kernel-hardening] Re: [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks

[James Morris]

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>

[kernel-hardening] RE: [RFC] [PATCH 0/5] Hardchroot LSM + additional hooks

[Reshetova, Elena]

[-- 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 --]