[RFC,5/5] Hardchroot LSM

Commit Message

Reshetova, Elena July 29, 2016, 7:34 a.m. UTC

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

Comments

Brad Spengler July 29, 2016, 11:44 a.m. UTC | #1

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

Reshetova, Elena July 29, 2016, 12:15 p.m. UTC | #2

>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

Reshetova, Elena July 29, 2016, 12:25 p.m. UTC | #3

>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!

Jann Horn July 29, 2016, 6:53 p.m. UTC | #4

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.

Casey Schaufler July 29, 2016, 7:20 p.m. UTC | #5

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.

Jann Horn July 29, 2016, 8:53 p.m. UTC | #6

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.

Casey Schaufler July 29, 2016, 9:10 p.m. UTC | #7

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.

Jann Horn July 29, 2016, 9:50 p.m. UTC | #8

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

Reshetova, Elena July 30, 2016, 6:10 a.m. UTC | #9

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.

Patch

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 <[e-mail]>
+ *
+ * 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.