[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160729185317.GF11621@pc.thejh.net>
Date: Fri, 29 Jul 2016 20:53:17 +0200
From: Jann Horn <jann@...jh.net>
To: kernel-hardening@...ts.openwall.com
Cc: linux-security-module@...r.kernel.org, keescook@...omium.org,
spender@...ecurity.net, jmorris@...ei.org,
casey.schaufler@...el.com, michael.leibowitz@...el.com,
william.c.roberts@...el.com,
Elena Reshetova <elena.reshetova@...el.com>
Subject: Re: [RFC] [PATCH 5/5] Hardchroot LSM
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.
Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
