linux-kernel - Re: PATCH? fix unshare(NEWPID) && vfork()

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]
Date:	Tue, 20 Aug 2013 13:52:17 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andy Lutomirski <luto@...capital.net>,
	Brad Spengler <spender@...ecurity.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Colin Walters <walters@...hat.com>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: PATCH? fix unshare(NEWPID) && vfork()

Oleg Nesterov <oleg@...hat.com> writes:

> On 08/20, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@...hat.com> writes:
>>
>> > On 08/19, Andy Lutomirski wrote:
>> >>
>> >> On Mon, Aug 19, 2013 at 11:33 AM, Oleg Nesterov <oleg@...hat.com> wrote:
>> >> >
>> >> > So do you think this change is fine or not (ignoring the fact it needs
>> >> > cleanups) ?
>> >>
>> >> I think that removing the CLONE_VM check is fine (although there are
>> >> some other ones that should probably be removed as well), but I'm not
>> >> sure if that check needs replacing with something else.
>> >
>> > OK, thanks... but I still can't understand.
>> >
>> > The patch I sent is equivalent to the new one below. I just tried to
>> > unify it with another check in do_fork().
>>
>> The patch below also needs CLONE_SIGHAND.  You can't meaningfully share
>> signal handlers if you can't represent the pid in the siginfo.  pids and
>> signals are too interconnected.
>
> I don't really understand. If we allow to share ->mm (with this patch),
> why it is bad to share sighand_struct->action[] ? This only shares the
> pointers to the code which handles a signal.

Not the signal queues?   I guess it is only CLONE_THREAD that shares the
signal queues between tasks.

The practical problem I am worried about with signals is in
__send_signal and the lead up to send signal, pids and uids are stored
in the struct siginfo in the namespace of the destination task.  If that
destination task shares the signal queues we can not possibly store the
proper information.

I believe that sharing just the signal handlers between tasks is also a
problem because while in principle you could distinguish the signals.
In practice it will require at least an extra system call to do so.

> However I agree it probably makes sense to deny it "just in case",
> I do not think CLONE_SIGHAND can be useful in this case.

I agree.  The only CLONE_VM case I can imagine being useful is vfork().

> But then we should also deny CLONE_SIGHAND if CLONE_NEWUSER|CLONE_NEWPID
> (another check in do_fork()). Which makes me think again we should unify
> these 2 checks.

I agree CLONE_SIGHAND if CLONE_NEWUSER|CLONE_NEWPID.

As for unifying them.  I can see colocating the related checks to keep
everything together.  I don't know about unifying them.

I know I get a little dizzy when trying to think of all of the possible
orderings of unshare and clone that can result in problem cases.

Hmm.  Thinking a little more I am half sold on unification.  If we just
handle the pid namespace by itself.  And we handle the user namespace by
itself I think things make sense.

So I am thinking something like the diff below.  CLONE_SIGHAND as in
theory you can figure out which task you are in and sort it out,
although I don't expect that to happen in practice.

Eric



diff --git a/kernel/fork.c b/kernel/fork.c
index 66635c8..0356852 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1172,12 +1172,21 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 				current->signal->flags & SIGNAL_UNKILLABLE)
 		return ERR_PTR(-EINVAL);
 
+	/* Dissallow unshare(CLONE_NEWPID) ; clone(CLONE_NEWPID).
+	 * That can result in a possibly empty parent pid namespace
+	 * which makes no sense.
+	 */
+	if ((clone_flags & CLONE_NEWPID) &&
+	    task_active_pid_ns(current) != current->nsproxy->pid_ns)
+		return ERR_PTR(-EINVAL);
+
 	/*
 	 * If the new process will be in a different pid namespace
-	 * don't allow the creation of threads.
+	 * don't allow the problematic sharing.
 	 */
-	if ((clone_flags & (CLONE_VM|CLONE_NEWPID)) &&
-	    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
+	if ((clone_flags & (CLONE_THREAD|CLONE_SIGHAND|CLONE_PARENT)) &&
+	    ((clone_flags & CLONE_NEWPID) ||
+	     (task_active_pid_ns(current) != current->nsproxy->pid_ns))
 		return ERR_PTR(-EINVAL);
 
 	retval = security_task_create(clone_flags);
@@ -1578,8 +1587,8 @@ long do_fork(unsigned long clone_flags,
 	 * Do some preliminary argument and permissions checking before we
 	 * actually start allocating stuff
 	 */
-	if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
-		if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
+	if (clone_flags & (CLONE_NEWUSER)) {
+		if (clone_flags & (CLONE_THREAD | CLONE_PARENT | CLONE_SIGHAND))
 			return -EINVAL;
 	}
 
@@ -1816,12 +1825,12 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags)
 	 * If unsharing a user namespace must also unshare the thread.
 	 */
 	if (unshare_flags & CLONE_NEWUSER)
-		unshare_flags |= CLONE_THREAD | CLONE_FS;
+		unshare_flags |= CLONE_THREAD | CLONE_FS | CLONE_SIGHAND;
 	/*
 	 * If unsharing a pid namespace must also unshare the thread.
 	 */
 	if (unshare_flags & CLONE_NEWPID)
-		unshare_flags |= CLONE_THREAD;
+		unshare_flags |= CLONE_THREAD | CLONE_SIGHAND;
 	/*
 	 * If unsharing a thread from a thread group, must also unshare vm.
 	 */
--
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/