[v2,2/8] exec: turn self_exec_id into self_privunit
diff mbox

Message ID 20160930132046.GA12047@redhat.com
State New
Headers show

Commit Message

Oleg Nesterov Sept. 30, 2016, 1:20 p.m. UTC
On 09/23, Jann Horn wrote:
>
> One reason for doing this is that it prevents an attacker from sending an
> arbitrary signal to a parent process after performing 2^32-1 execve()
> calls.

I think we should simply kill self/parent_exec_id's. I am going to send
the patch below after re-check/testing.

Oleg.



--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Oleg Nesterov Sept. 30, 2016, 1:44 p.m. UTC | #1
forgot to mention...

On 09/30, Oleg Nesterov wrote:
>
> On 09/23, Jann Horn wrote:
> >
> > One reason for doing this is that it prevents an attacker from sending an
> > arbitrary signal to a parent process after performing 2^32-1 execve()
> > calls.

No, sets ->exit_signal = SIGCHLD. So the only problem is that the parent
can do clone(SIGKILL), then do execve() 2^32-1 times, then it can be killed
by SIGKILL from the exiting child.

Honestly, I do not think this is security problem.

> I think we should simply kill self/parent_exec_id's. I am going to send
> the patch below after re-check/testing.

Yes, I think this makes sense anyway.

Oleg.

> --- x/include/linux/sched.h
> +++ x/include/linux/sched.h
> @@ -1677,9 +1677,6 @@ struct task_struct {
>  #endif
>  	struct seccomp seccomp;
>  
> -/* Thread group tracking */
> -   	u32 parent_exec_id;
> -   	u32 self_exec_id;
>  /* Protection of (de-)allocation: mm, files, fs, tty, keyrings, mems_allowed,
>   * mempolicy */
>  	spinlock_t alloc_lock;
> --- x/fs/exec.c
> +++ x/fs/exec.c
> @@ -1163,6 +1163,14 @@ static int de_thread(struct task_struct 
>  no_thread_group:
>  	/* we have changed execution domain */
>  	tsk->exit_signal = SIGCHLD;
> +	if (!list_empty(&father->children)) {
> +		struct task_struct *child;
> +
> +		read_lock(&tasklist_lock);
> +		list_for_each_entry(child, &father->children, sibling)
> +			child->exit_signal = SIGCHLD;
> +		read_unlock(&tasklist_lock);
> +	}
>  
>  	exit_itimers(sig);
>  	flush_itimer_signals();
> @@ -1306,9 +1314,6 @@ void setup_new_exec(struct linux_binprm 
>  			set_dumpable(current->mm, suid_dumpable);
>  	}
>  
> -	/* An exec changes our domain. We are no longer part of the thread
> -	   group */
> -	current->self_exec_id++;
>  	flush_signal_handlers(current, 0);
>  	do_close_on_exec(current->files);
>  }
> --- x/kernel/fork.c
> +++ x/kernel/fork.c
> @@ -1573,13 +1573,10 @@ static struct task_struct *copy_process(
>  	write_lock_irq(&tasklist_lock);
>  
>  	/* CLONE_PARENT re-uses the old parent */
> -	if (clone_flags & (CLONE_PARENT|CLONE_THREAD)) {
> +	if (clone_flags & (CLONE_PARENT|CLONE_THREAD))
>  		p->real_parent = current->real_parent;
> -		p->parent_exec_id = current->parent_exec_id;
> -	} else {
> +	else
>  		p->real_parent = current;
> -		p->parent_exec_id = current->self_exec_id;
> -	}
>  
>  	spin_lock(&current->sighand->siglock);
>  
> --- x/kernel/signal.c
> +++ x/kernel/signal.c
> @@ -1585,15 +1585,6 @@ bool do_notify_parent(struct task_struct
>  	BUG_ON(!tsk->ptrace &&
>  	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
>  
> -	if (sig != SIGCHLD) {
> -		/*
> -		 * This is only possible if parent == real_parent.
> -		 * Check if it has changed security domain.
> -		 */
> -		if (tsk->parent_exec_id != tsk->parent->self_exec_id)
> -			sig = SIGCHLD;
> -	}
> -
>  	info.si_signo = sig;
>  	info.si_errno = 0;
>  	/*

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook Sept. 30, 2016, 6:30 p.m. UTC | #2
On Fri, Sep 30, 2016 at 6:44 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> forgot to mention...
>
> On 09/30, Oleg Nesterov wrote:
>>
>> On 09/23, Jann Horn wrote:
>> >
>> > One reason for doing this is that it prevents an attacker from sending an
>> > arbitrary signal to a parent process after performing 2^32-1 execve()
>> > calls.
>
> No, sets ->exit_signal = SIGCHLD. So the only problem is that the parent
> can do clone(SIGKILL), then do execve() 2^32-1 times, then it can be killed
> by SIGKILL from the exiting child.
>
> Honestly, I do not think this is security problem.

It's a corner case, to be sure. But even sending a SIGKILL across
privilege boundaries should not be allowed to happen.

>
>> I think we should simply kill self/parent_exec_id's. I am going to send
>> the patch below after re-check/testing.
>
> Yes, I think this makes sense anyway.

Hrm, I also thought this was used for more than just signal checking,
but I don't see anything else right now. Maybe I was remembering
earlier versions of Jann's patches...

-Kees
Jann Horn Sept. 30, 2016, 6:59 p.m. UTC | #3
On Fri, Sep 30, 2016 at 11:30:23AM -0700, Kees Cook wrote:
> On Fri, Sep 30, 2016 at 6:44 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 09/30, Oleg Nesterov wrote:
> >> I think we should simply kill self/parent_exec_id's. I am going to send
> >> the patch below after re-check/testing.
> >
> > Yes, I think this makes sense anyway.
> 
> Hrm, I also thought this was used for more than just signal checking,
> but I don't see anything else right now. Maybe I was remembering
> earlier versions of Jann's patches...

Maybe you're thinking of grsecurity's exec_id (which I used as the basis
for my first implementation of the unique ID before Andy suggested the
LUID approach)?
Kees Cook Sept. 30, 2016, 7:05 p.m. UTC | #4
On Fri, Sep 30, 2016 at 11:59 AM, Jann Horn <jann@thejh.net> wrote:
> On Fri, Sep 30, 2016 at 11:30:23AM -0700, Kees Cook wrote:
>> On Fri, Sep 30, 2016 at 6:44 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > On 09/30, Oleg Nesterov wrote:
>> >> I think we should simply kill self/parent_exec_id's. I am going to send
>> >> the patch below after re-check/testing.
>> >
>> > Yes, I think this makes sense anyway.
>>
>> Hrm, I also thought this was used for more than just signal checking,
>> but I don't see anything else right now. Maybe I was remembering
>> earlier versions of Jann's patches...
>
> Maybe you're thinking of grsecurity's exec_id (which I used as the basis
> for my first implementation of the unique ID before Andy suggested the
> LUID approach)?

Ah, yes, so I am. Looks like it's part of CONFIG_GRKERNSEC_PROC_MEMMAP.

-Kees
Oleg Nesterov Oct. 3, 2016, 4:37 p.m. UTC | #5
On 09/30, Kees Cook wrote:
>
> On Fri, Sep 30, 2016 at 6:44 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > forgot to mention...
> >
> > On 09/30, Oleg Nesterov wrote:
> >>
> >> On 09/23, Jann Horn wrote:
> >> >
> >> > One reason for doing this is that it prevents an attacker from sending an
> >> > arbitrary signal to a parent process after performing 2^32-1 execve()
> >> > calls.
> >
> > No, sets ->exit_signal = SIGCHLD. So the only problem is that the parent
> > can do clone(SIGKILL), then do execve() 2^32-1 times, then it can be killed
> > by SIGKILL from the exiting child.
> >
> > Honestly, I do not think this is security problem.
>
> It's a corner case, to be sure. But even sending a SIGKILL across
> privilege boundaries should not be allowed to happen.

Agreed, and actually I need to take my words back, of course this is not
nice security-wise.

So lets kill these counters. At least they should not live in task_struct.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

--- x/include/linux/sched.h
+++ x/include/linux/sched.h
@@ -1677,9 +1677,6 @@  struct task_struct {
 #endif
 	struct seccomp seccomp;
 
-/* Thread group tracking */
-   	u32 parent_exec_id;
-   	u32 self_exec_id;
 /* Protection of (de-)allocation: mm, files, fs, tty, keyrings, mems_allowed,
  * mempolicy */
 	spinlock_t alloc_lock;
--- x/fs/exec.c
+++ x/fs/exec.c
@@ -1163,6 +1163,14 @@  static int de_thread(struct task_struct 
 no_thread_group:
 	/* we have changed execution domain */
 	tsk->exit_signal = SIGCHLD;
+	if (!list_empty(&father->children)) {
+		struct task_struct *child;
+
+		read_lock(&tasklist_lock);
+		list_for_each_entry(child, &father->children, sibling)
+			child->exit_signal = SIGCHLD;
+		read_unlock(&tasklist_lock);
+	}
 
 	exit_itimers(sig);
 	flush_itimer_signals();
@@ -1306,9 +1314,6 @@  void setup_new_exec(struct linux_binprm 
 			set_dumpable(current->mm, suid_dumpable);
 	}
 
-	/* An exec changes our domain. We are no longer part of the thread
-	   group */
-	current->self_exec_id++;
 	flush_signal_handlers(current, 0);
 	do_close_on_exec(current->files);
 }
--- x/kernel/fork.c
+++ x/kernel/fork.c
@@ -1573,13 +1573,10 @@  static struct task_struct *copy_process(
 	write_lock_irq(&tasklist_lock);
 
 	/* CLONE_PARENT re-uses the old parent */
-	if (clone_flags & (CLONE_PARENT|CLONE_THREAD)) {
+	if (clone_flags & (CLONE_PARENT|CLONE_THREAD))
 		p->real_parent = current->real_parent;
-		p->parent_exec_id = current->parent_exec_id;
-	} else {
+	else
 		p->real_parent = current;
-		p->parent_exec_id = current->self_exec_id;
-	}
 
 	spin_lock(&current->sighand->siglock);
 
--- x/kernel/signal.c
+++ x/kernel/signal.c
@@ -1585,15 +1585,6 @@  bool do_notify_parent(struct task_struct
 	BUG_ON(!tsk->ptrace &&
 	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
 
-	if (sig != SIGCHLD) {
-		/*
-		 * This is only possible if parent == real_parent.
-		 * Check if it has changed security domain.
-		 */
-		if (tsk->parent_exec_id != tsk->parent->self_exec_id)
-			sig = SIGCHLD;
-	}
-
 	info.si_signo = sig;
 	info.si_errno = 0;
 	/*