Andrew Morton <[email protected]> writes:
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5/2.6.5-mm4/
>
> +compute-creds-race-fix.patch
> +compute-creds-race-fix-fix.patch
>
> Fix possible race in permission calculation across exec()
this is a small fix for the modified must_not_trace_exec() test.
I have tested neither the compute-creds-race-fix nor my patch.
It is on top of 2.6.5 + compute-creds-race-fix.patch +
compute-creds-race-fix-fix.patch.
Although, I'd rather not lump together unrelated tests without
renaming must_not_trace_exec(). Btw, can someone enlighten me what
this atomic_read() test is all about.
Regards, Olaf.
diff -urN a/security/commoncap.c b/security/commoncap.c
--- a/security/commoncap.c Mon Apr 12 10:38:17 2004
+++ b/security/commoncap.c Mon Apr 12 11:10:38 2004
@@ -118,9 +118,9 @@
static inline int must_not_trace_exec (struct task_struct *p)
{
return ((p->ptrace & PT_PTRACED) && !(p->ptrace & PT_PTRACE_CAP))
- || atomic_read(¤t->fs->count) > 1
- || atomic_read(¤t->files->count) > 1
- || atomic_read(¤t->sighand->count) > 1;
+ || atomic_read(&p->fs->count) > 1
+ || atomic_read(&p->files->count) > 1
+ || atomic_read(&p->sighand->count) > 1;
}
void cap_bprm_apply_creds (struct linux_binprm *bprm)
diff -urN a/security/dummy.c b/security/dummy.c
--- a/security/dummy.c Mon Apr 12 10:38:17 2004
+++ b/security/dummy.c Mon Apr 12 11:10:50 2004
@@ -174,9 +174,9 @@
static inline int must_not_trace_exec (struct task_struct *p)
{
return ((p->ptrace & PT_PTRACED) && !(p->ptrace & PT_PTRACE_CAP))
- || atomic_read(¤t->fs->count) > 1
- || atomic_read(¤t->files->count) > 1
- || atomic_read(¤t->sighand->count) > 1;
+ || atomic_read(&p->fs->count) > 1
+ || atomic_read(&p->files->count) > 1
+ || atomic_read(&p->sighand->count) > 1;
}
static void dummy_bprm_apply_creds (struct linux_binprm *bprm)
Olaf Dietsche wrote:
> Andrew Morton <[email protected]> writes:
>
>
> Although, I'd rather not lump together unrelated tests without
> renaming must_not_trace_exec(). Btw, can someone enlighten me what
> this atomic_read() test is all about.
Oops... your fix is obviously correct.
I assumed that the test was to check if the caller is a thread, but that sounds
odd -- wouldn't it stop being a thread after the exec anyway? Maybe that part
happens after compute_creds, so this prevents a race? Although I don't see how
it could be triggered if the thread never entered usermode before getting a new
fs/files/sighand.
Anyone?
>
> Regards, Olaf.
>
> diff -urN a/security/commoncap.c b/security/commoncap.c
> --- a/security/commoncap.c Mon Apr 12 10:38:17 2004
> +++ b/security/commoncap.c Mon Apr 12 11:10:38 2004
> @@ -118,9 +118,9 @@
> static inline int must_not_trace_exec (struct task_struct *p)
> {
> return ((p->ptrace & PT_PTRACED) && !(p->ptrace & PT_PTRACE_CAP))
> - || atomic_read(¤t->fs->count) > 1
> - || atomic_read(¤t->files->count) > 1
> - || atomic_read(¤t->sighand->count) > 1;
> + || atomic_read(&p->fs->count) > 1
> + || atomic_read(&p->files->count) > 1
> + || atomic_read(&p->sighand->count) > 1;
> }
> [...]
--Andy
* Andy Lutomirski ([email protected]) wrote:
> Olaf Dietsche wrote:
> > Although, I'd rather not lump together unrelated tests without
> > renaming must_not_trace_exec(). Btw, can someone enlighten me what
> > this atomic_read() test is all about.
>
> I assumed that the test was to check if the caller is a thread, but that
> sounds odd -- wouldn't it stop being a thread after the exec anyway?
> Maybe that part happens after compute_creds, so this prevents a race?
> Although I don't see how it could be triggered if the thread never
> entered usermode before getting a new fs/files/sighand.
There's no requirement for CLONE_THREAD when using at least CLONE_FS
and CLONE_FILES. And all of the latter are inherited across execve().
These tests are needed to keep a malicious program from controlling the
setuid program in ways other than ptrace.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net