linux-kernel - Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit

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: <20090226215945.GA12520@redhat.com>
Date:	Thu, 26 Feb 2009 22:59:46 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Roland McGrath <roland@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Chris Evans <scarybeasts@...il.com>,
	David Howells <dhowells@...hat.com>,
	Don Howard <dhoward@...hat.com>,
	Eugene Teo <eugene@...hat.com>,
	Michael Kerrisk <mtk.manpages@...glemail.com>,
	Tavis Ormandy <taviso@....lonestar.org>,
	Vitaly Mayatskikh <vmayatsk@...hat.com>, stable@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal
	interaction

On 02/25, Oleg Nesterov wrote:
>
> On 02/25, Linus Torvalds wrote:
> >
> > > As I think I said before, I don't really know what the actual use case is
> > > for CLONE_PARENT without CLONE_THREAD.  So it's easy to approve changing
> > > its behavior, but I do vaguely worry about who expected what behavior before.
> >
> > I think changing it is wrong.
>
> Perhaps. As I said, I don't know what is the expected behaviour. And in fact
> I can't think of the "obviously good" behaviour.
>
> > I can easily see somebody using CLONE_PARENT to get the correct getppid
> > semantics in the thread, and then setting the signal to zero to not make
> > the parent see the thread go away.
>
> ->exit_signal == 0 doesn't mean the thread silently goes away, it becomes
> a zombie (even if ->parent ignores SIGCHLD). We don't send the signal, but
> that is all.
>
> And if ->parent execs, we reset ->exit_signal to SIGCHLD anyway.

Really, how CLONE_PARENT + exit_signal==0 can be useful?

I still think this patch does the right change. Not because it fixes the
security problem, as I said I do not think the ability to send SIGKILL to
->parent is wrong from the security pov, even if child does setuid/etc,
the problem is that CLONE_PARENT can fool ->xxx_exec_id logic and we can
send SIGKILL after parent/child exec.

But because the current behaviour is just silly. Imho.

But of course, if this change can break the user-space applications, then
it should not be applied.

> > And quite
> > frankly, it would be good to try to see if there are other alternatives.
>
> Agreed. I thought about checking ->xxx_exec_id's in copy_process(),
> but doesn't look very nice...

I meant something like the patch below. But I don't like it.

Anybody has other ideas?

Oleg.

--- kernel/fork.c
+++ kernel/fork.c
@@ -1218,9 +1218,15 @@ static struct task_struct *copy_process(
 		set_task_cpu(p, smp_processor_id());
 
 	/* 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;
-	else
+		p->self_exec_id = p->parent_exec_id =
+				p->real_parent->self_exec_id;
+
+		if (current->parent_exec_id != current->real_parent->self_exec_id ||
+		    current->self_exec_id  != current->parent_exec_id)
+			p->exit_signal = SIGCHLD;
+	} else
 		p->real_parent = current;
 
 	spin_lock(&current->sighand->siglock);

--
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/