linux-kernel - Re: [patch 2/6] posix-cpu-timers: Fix permission check regression

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190905173148.GE18251@lenoir>
Date:   Thu, 5 Sep 2019 19:31:48 +0200
From:   Frederic Weisbecker <frederic@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Frederic Weisbecker <fweisbec@...il.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Ingo Molnar <mingo@...nel.org>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [patch 2/6] posix-cpu-timers: Fix permission check regression

On Thu, Sep 05, 2019 at 02:03:41PM +0200, Thomas Gleixner wrote:
> The recent consolidation of the three permission checks introduced a subtle
> regression. For timer_create() with a process wide timer it returns the
> current task if the lookup through the PID which is encoded into the
> clockid results in returning current.
> 
> That's broken because it does not validate whether the current task is the
> group leader.
> 
> That was caused by the two different variants of permission checks:
> 
>   - posix_cpu_timer_get() allowed access to the process wide clock when the
>     looked up task is current. That's not an issue because the process wide
>     clock is in the shared sighand.
> 
>   - posix_cpu_timer_create() made sure that the looked up task is the group
>     leader.
> 
> Restore the previous state.
> 
> Note, that these permission checks are more than questionable, but that's
> subject to follow up changes.
> 
> Fixes: 6ae40e3fdcd3 ("posix-cpu-timers: Provide task validation functions")
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  kernel/time/posix-cpu-timers.c |   40 +++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -47,25 +47,42 @@ void update_rlimit_cpu(struct task_struc
>  /*
>   * Functions for validating access to tasks.
>   */
> -static struct task_struct *lookup_task(const pid_t pid, bool thread)
> +static struct task_struct *lookup_task(const pid_t pid, bool thread,
> +				       bool gettime)
>  {
>  	struct task_struct *p;
>  
> +	/*
> +	 * If the encoded PID is 0, then the timer is targeted at current
> +	 * or the process to which current belongs.
> +	 */
>  	if (!pid)
>  		return thread ? current : current->group_leader;
>  
>  	p = find_task_by_vpid(pid);
> -	if (!p || p == current)
> +	if (!p)
>  		return p;
> +
>  	if (thread)
>  		return same_thread_group(p, current) ? p : NULL;
> -	if (p == current)
> -		return p;
> +
> +	if (gettime) {
> +		/*
> +		 * For clock_gettime() the task does not need to be the
> +		 * actual group leader. tsk->sighand gives access to the
> +		 * group's clock.
> +		 */

I'm a bit confused with the explanation. Why is it fine to do so with clock
and not with timer? tsk->sighand gives access to the group's timer as
well.

> +		return (p == current || thread_group_leader(p)) ? p : NULL;
> +	}
> +
> +	/*
> +	 * For processes require that p is group leader.
> +	 */
>  	return has_group_leader_pid(p) ? p : NULL;
>  }