| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Mar 2009 21:31:09 +0000 (GMT)
From: Hugh Dickins <hugh@...itas.com>
To: David Howells <dhowells@...hat.com>
cc: jmalicki@...acarta.com, chrisw@...s-sol.org,
akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org
Subject: Re: [PATCH] CRED: Fix check_unsafe_exec()
On Tue, 10 Mar 2009, David Howells wrote:
> check_unsafe_exec() relies on the usage counts of fs_struct and files_struct to
> indicate the subscription count of cloned processes to these structures. This
> is not a viable method, however, as /proc can increment these counts when
> merely accessing the data.
>
> The effect of this is to occasionally prevent setuid executables from altering
> their security details correctly.
>
> To deal with this, subscription counters are added in addition to the usage
> counters to fs_struct and files_struct.
>
> This should hopefully fix:
>
> http://lkml.org/lkml/2009/2/25/491
> Date: Wed, 25 Feb 2009 18:11:54 -0500 (EST)
> From: Joe Malicki <jmalicki@...acarta.com>
> Subject: BUG: setuid sometimes doesn't.
>
> Very rarely, we experience a setuid program not properly getting
> the euid of its owner. This happens with (at least) both Linux 2.6.24.7
> and Linux 2.6.28.4, on multiple machines of at least two configurations
> (Dell 860 and Dell 2950 - cpuinfo attached).
> ...
>
> Reported-by: Joe Malicki <jmalicki@...acarta.com>
> Signed-off-by: David Howells <dhowells@...hat.com>
My current, certainly not to be trusted, belief is that this is
unnecessary overkill - as I've already suggested in private mail.
Surely we'd prefer to avoid the overhead of additional confusing
counts if they can be avoided?
We already have what I think is a satisfactory patch for the struct fs
part of it: /proc can easily manage root and pwd while holding the lock
instead of raising fs->count.
I don't understand why check_unsafe_exec() needs to check
current->files->count at all, since do_execve() has already
done an unshare_files() to get its own copy - and proceeds with
that one if the exec succeeds.
My belief is that the files->count check could/should have been
removed when that unshare_files() was put in. Please explain why
I'm wrong on that - I can quite accept that I'm muddled about it,
but please do explain it to me.
Hugh
> ---
>
> fs/exec.c | 4 ++--
> fs/file.c | 1 +
> include/linux/fdtable.h | 4 +++-
> include/linux/fs_struct.h | 7 ++++++-
> kernel/exit.c | 2 ++
> kernel/fork.c | 3 +++
> 6 files changed, 17 insertions(+), 4 deletions(-)
>
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 929b580..67d7a45 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1069,8 +1069,8 @@ void check_unsafe_exec(struct linux_binprm *bprm, struct files_struct *files)
> n_sighand++;
> }
>
> - if (atomic_read(&p->fs->count) > n_fs ||
> - atomic_read(&p->files->count) > n_files ||
> + if (atomic_read(&p->fs->subscribers) > n_fs ||
> + atomic_read(&p->files->subscribers) > n_files ||
> atomic_read(&p->sighand->count) > n_sighand)
> bprm->unsafe |= LSM_UNSAFE_SHARE;
>
> diff --git a/fs/file.c b/fs/file.c
> index f313314..6a33a7a 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -303,6 +303,7 @@ struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
> goto out;
>
> atomic_set(&newf->count, 1);
> + atomic_set(&newf->subscribers, 1);
>
> spin_lock_init(&newf->file_lock);
> newf->next_fd = 0;
> diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
> index 09d6c5b..12e54bc 100644
> --- a/include/linux/fdtable.h
> +++ b/include/linux/fdtable.h
> @@ -42,7 +42,9 @@ struct files_struct {
> /*
> * read mostly part
> */
> - atomic_t count;
> + atomic_t count; /* number of processes accessing this set */
> + atomic_t subscribers; /* number of cloned processes subscribed to
> + * this set */
> struct fdtable *fdt;
> struct fdtable fdtab;
> /*
> diff --git a/include/linux/fs_struct.h b/include/linux/fs_struct.h
> index a97c053..47679c1 100644
> --- a/include/linux/fs_struct.h
> +++ b/include/linux/fs_struct.h
> @@ -3,8 +3,13 @@
>
> #include <linux/path.h>
>
> +/*
> + * General filesystem access parameter block.
> + */
> struct fs_struct {
> - atomic_t count;
> + atomic_t count; /* number of processes accessing this block */
> + atomic_t subscribers; /* number of cloned processes subscribed to
> + * this block */
> rwlock_t lock;
> int umask;
> struct path root, pwd;
> diff --git a/kernel/exit.c b/kernel/exit.c
> index efd30cc..57b63bb 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -561,6 +561,7 @@ void exit_files(struct task_struct *tsk)
> task_lock(tsk);
> tsk->files = NULL;
> task_unlock(tsk);
> + atomic_dec(&files->subscribers);
> put_files_struct(files);
> }
> }
> @@ -583,6 +584,7 @@ void exit_fs(struct task_struct *tsk)
> task_lock(tsk);
> tsk->fs = NULL;
> task_unlock(tsk);
> + atomic_dec(&fs->subscribers);
> put_fs_struct(fs);
> }
> }
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 4854c2c..9d1a2a7 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -682,6 +682,7 @@ static struct fs_struct *__copy_fs_struct(struct fs_struct *old)
> /* We don't need to lock fs - think why ;-) */
> if (fs) {
> atomic_set(&fs->count, 1);
> + atomic_set(&fs->subscribers, 1);
> rwlock_init(&fs->lock);
> fs->umask = old->umask;
> read_lock(&old->lock);
> @@ -705,6 +706,7 @@ static int copy_fs(unsigned long clone_flags, struct task_struct *tsk)
> {
> if (clone_flags & CLONE_FS) {
> atomic_inc(¤t->fs->count);
> + atomic_inc(¤t->fs->subscribers);
> return 0;
> }
> tsk->fs = __copy_fs_struct(current->fs);
> @@ -727,6 +729,7 @@ static int copy_files(unsigned long clone_flags, struct task_struct * tsk)
>
> if (clone_flags & CLONE_FILES) {
> atomic_inc(&oldf->count);
> + atomic_inc(&oldf->subscribers);
> goto out;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists