Re: [PATCH] pid safety checks for 2.2.x

2007-07-05 Thread Jim Jagielski
On Jul 4, 2007, at 12:52 PM, Joe Orton wrote: On Thu, Jun 28, 2007 at 12:50:37PM -0400, Jim Jagielski wrote: On Jun 28, 2007, at 7:56 AM, Joe Orton wrote: So, final comments on this? If there's consensus that this is the approach to take I'll revert the pidtable stuff out of trunk, commit

Re: [PATCH] pid safety checks for 2.2.x

2007-07-04 Thread Joe Orton
On Thu, Jun 28, 2007 at 12:50:37PM -0400, Jim Jagielski wrote: On Jun 28, 2007, at 7:56 AM, Joe Orton wrote: So, final comments on this? If there's consensus that this is the approach to take I'll revert the pidtable stuff out of trunk, commit this there, and propose the backport. Don't

Re: [PATCH] pid safety checks for 2.2.x

2007-06-29 Thread Joe Orton
On Wed, Jun 27, 2007 at 04:42:38PM -0400, Jim Jagielski wrote: I might be missing this (just did a quick scan) but what about ap_reclaim_child_processes/reclaim_one_pid()? Here we trust the pid in the scoreboard and send signals. I'd said in the other thread that this wasn't an attack vector

Re: [PATCH] pid safety checks for 2.2.x

2007-06-28 Thread Joe Orton
On Wed, Jun 27, 2007 at 09:38:10PM +0200, Ruediger Pluem wrote: +/* Ensure the given pid is greater than zero; passing waitpid() a + * zero or negative pid has different semantics. */ Ok, it seems as I am trying to become the king of all nitpickers :-): Style of comment. Happy

Re: [PATCH] pid safety checks for 2.2.x

2007-06-28 Thread Plüm , Rüdiger , VF-Group
-Ursprüngliche Nachricht- Von: Joe Orton Gesendet: Donnerstag, 28. Juni 2007 13:56 An: dev@httpd.apache.org Betreff: Re: [PATCH] pid safety checks for 2.2.x On Wed, Jun 27, 2007 at 09:38:10PM +0200, Ruediger Pluem wrote: +/* Ensure the given pid is greater than zero

Re: [PATCH] pid safety checks for 2.2.x

2007-06-28 Thread Joe Orton
On Thu, Jun 28, 2007 at 02:42:35PM +0200, Plüm, Rüdiger, VF-Group wrote: The problem is that waitpid() does not distinguish between child already reaped (ignorable error) and child not in process group (something bad) so that will mean some unnecessary log spam in some cases. I

Re: [PATCH] pid safety checks for 2.2.x

2007-06-28 Thread Plüm , Rüdiger , VF-Group
-Ursprüngliche Nachricht- Von: Joe Orton Gesendet: Donnerstag, 28. Juni 2007 16:37 An: dev@httpd.apache.org Betreff: Re: [PATCH] pid safety checks for 2.2.x On Thu, Jun 28, 2007 at 02:42:35PM +0200, Plüm, Rüdiger, VF-Group wrote: The problem is that waitpid() does

Re: [PATCH] pid safety checks for 2.2.x

2007-06-28 Thread Jim Jagielski
On Jun 28, 2007, at 7:56 AM, Joe Orton wrote: So, final comments on this? If there's consensus that this is the approach to take I'll revert the pidtable stuff out of trunk, commit this there, and propose the backport. Don't forget the 1.3 branch...

[PATCH] pid safety checks for 2.2.x

2007-06-27 Thread Joe Orton
Here's the updated (and simpler) version of my patch which uses apr_proc_wait() to determine whether a pid is a valid child. Simplifies the MPM logic a bit since the pid != 0 check is moved into ap_mpm_safe_kill(). Tested for both prefork and worker (on Linux) to fix the vulnerability using

Re: [PATCH] pid safety checks for 2.2.x

2007-06-27 Thread Ruediger Pluem
On 06/27/2007 07:52 PM, Joe Orton wrote: Index: server/mpm_common.c === --- server/mpm_common.c (revision 549489) +++ server/mpm_common.c (working copy) @@ -305,6 +305,27 @@ cur_extra = next; } }

Re: [PATCH] pid safety checks for 2.2.x

2007-06-27 Thread Jim Jagielski
On Jun 27, 2007, at 3:38 PM, Ruediger Pluem wrote: Hm. Wouldn't it make sense to log this in the case waitret != APR_CHILD_DONE as in the PID table patches? This could give the admin a hint that something is rotten on his box. +1 on the logging... Looking forward to seeing the 1.3

Re: [PATCH] pid safety checks for 2.2.x

2007-06-27 Thread Jim Jagielski
On Jun 27, 2007, at 1:52 PM, Joe Orton wrote: Here's the updated (and simpler) version of my patch which uses apr_proc_wait() to determine whether a pid is a valid child. Simplifies the MPM logic a bit since the pid != 0 check is moved into ap_mpm_safe_kill(). Tested for both prefork and