Re: [PATCH] pid safety checks for 2.2.x
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 this there, and propose the backport. Don't forget the 1.3 branch... I've been trying to get a patch together for 1.3.x but the portability stuff keeps biting me - accurately detecting presence of getpgid is harder with the 1.3 build system even on Linux (it's hidden without _GNU_SOURCE defined, but helpers/TestCompile still detects it... etc). So I'd say stick with the existing pid-table stuff for 1.3 - I tested it with mod_scribble and it prevents the exploits tested there. I agree... I also tried porting the pgid stuff back and it was not as easy or straightforward as I would have hoped. With 1.3, having a parent table is certainly more self-contained than possible with 2.x and doesn't involve any portability issues, afaikt.
Re: [PATCH] pid safety checks for 2.2.x
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 forget the 1.3 branch... I've been trying to get a patch together for 1.3.x but the portability stuff keeps biting me - accurately detecting presence of getpgid is harder with the 1.3 build system even on Linux (it's hidden without _GNU_SOURCE defined, but helpers/TestCompile still detects it... etc). So I'd say stick with the existing pid-table stuff for 1.3 - I tested it with mod_scribble and it prevents the exploits tested there. joe
Re: [PATCH] pid safety checks for 2.2.x
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 (and hence 2.0.x wasn't vulnerable), because it already goes through a waitpid() before a kill(). Having looked it again on your prompting there is a cute way to exploit it: using a pid of -1 will have waitpid() wait for any child, which can easily succeed with not done, and then passing a pid of -1 to kill is... kind of nasty, especially for root. So I fixed that also in the commit to the trunk. I haven't forgotten 1.3, and will submit 2.0/2.2 backports for review shortly! joe
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; 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 to oblige but I'm not sure what the nit is - just my poor grammar
or the slightly inappropriate reference to waitpid()?
+waitret = apr_proc_wait(proc, NULL, NULL, APR_NOWAIT);
+if (waitret == APR_CHILD_NOTDONE) {
+return kill(pid, sig) ? errno : APR_SUCCESS;
+}
+else {
+return APR_EINVAL;
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.
I guess this is worthwhile.
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.
The log spam is avoidable using getpgid() so I've resurrected that on
platforms where available: (which will be 99%)
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.
Index: server/mpm/prefork/prefork.c
===
--- server/mpm/prefork/prefork.c(revision 549489)
+++ server/mpm/prefork/prefork.c(working copy)
@@ -1127,7 +1127,7 @@
for (index = 0; index ap_daemons_limit; ++index) {
if (ap_scoreboard_image-servers[index][0].status != SERVER_DEAD) {
/* Ask each child to close its listeners. */
-kill(MPM_CHILD_PID(index), AP_SIG_GRACEFUL);
+ap_mpm_safe_kill(MPM_CHILD_PID(index), AP_SIG_GRACEFUL);
active_children++;
}
}
@@ -1165,12 +1165,10 @@
active_children = 0;
for (index = 0; index ap_daemons_limit; ++index) {
-if (MPM_CHILD_PID(index) != 0) {
-if (kill(MPM_CHILD_PID(index), 0) == 0) {
-active_children = 1;
-/* Having just one child is enough to stay around
*/
-break;
-}
+if (ap_mpm_safe_kill(MPM_CHILD_PID(index), 0) == APR_SUCCESS) {
+active_children = 1;
+/* Having just one child is enough to stay around */
+break;
}
}
} while (!shutdown_pending active_children
@@ -1222,7 +1220,7 @@
* piped loggers, etc. They almost certainly won't handle
* it gracefully.
*/
-kill(ap_scoreboard_image-parent[index].pid, AP_SIG_GRACEFUL);
+ap_mpm_safe_kill(ap_scoreboard_image-parent[index].pid,
AP_SIG_GRACEFUL);
}
}
}
Index: server/mpm/worker/worker.c
===
--- server/mpm/worker/worker.c (revision 549489)
+++ server/mpm/worker/worker.c (working copy)
@@ -1813,12 +1813,10 @@
active_children = 0;
for (index = 0; index ap_daemons_limit; ++index) {
-if (MPM_CHILD_PID(index) != 0) {
-if (kill(MPM_CHILD_PID(index), 0) == 0) {
-active_children = 1;
-/* Having just one child is enough to stay around
*/
-break;
-}
+if (ap_mpm_safe_kill(MPM_CHILD_PID(index), 0) == APR_SUCCESS) {
+active_children = 1;
+/* Having just one child is enough to stay around */
+break;
}
}
} while (!shutdown_pending active_children
Index: server/mpm/experimental/event/event.c
===
--- server/mpm/experimental/event/event.c (revision 549489)
+++ server/mpm/experimental/event/event.c (working copy)
@@ -1998,12 +1998,10 @@
active_children = 0;
for (index = 0; index ap_daemons_limit; ++index) {
-if (MPM_CHILD_PID(index) != 0) {
-if (kill(MPM_CHILD_PID(index), 0) == 0) {
-active_children = 1;
-/* Having just one child is enough to stay around
*/
-break;
-}
+if (ap_mpm_safe_kill(MPM_CHILD_PID(index), 0) == APR_SUCCESS) {
+active_children = 1;
+/* Having just one child is enough to stay around */
+break;
}
}
} while
Re: [PATCH] pid safety checks for 2.2.x
-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;
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 to oblige but I'm not sure what the nit is - just my
poor grammar
or the slightly inappropriate reference to waitpid()?
It is simpler and more formal :-). My point was that it should be
/*
* Ensure the given pid is greater than zero; passing waitpid() a
* zero or negative pid has different semantics.
*/
instead of
/* Ensure the given pid is greater than zero; passing waitpid() a
* zero or negative pid has different semantics. */
But now I have to admit that there is no rule in our style guide for multiline
comments (I thought there was) and the style I proposed just seem to be used
very often. So sorry for nitpicking on this.
+waitret = apr_proc_wait(proc, NULL, NULL, APR_NOWAIT);
+if (waitret == APR_CHILD_NOTDONE) {
+return kill(pid, sig) ? errno : APR_SUCCESS;
+}
+else {
+return APR_EINVAL;
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.
I guess this is worthwhile.
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 guess we are talking about ECHILD as the corresponding error value.
But under non malicious child conditions is there really the possibility
that we get here when the child was already reaped?
The log spam is avoidable using getpgid() so I've resurrected that on
platforms where available: (which will be 99%)
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.
I assume that your latest patch only differs from the previous one
in the implementation of ap_mpm_safe_kill and the additional line in
configure.in,
right?
Regards
Rüdiger
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 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 guess we are talking about ECHILD as the corresponding error value. But under non malicious child conditions is there really the possibility that we get here when the child was already reaped? Actually I'm not sure there is; but I'd rather err on the side of caution here, a bunch of new scary-looking log messages can be very confusing for users. 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. I assume that your latest patch only differs from the previous one in the implementation of ap_mpm_safe_kill and the additional line in configure.in, right? Correct! joe
Re: [PATCH] pid safety checks for 2.2.x
-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 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 guess we are talking about ECHILD as the corresponding error value. But under non malicious child conditions is there really the possibility that we get here when the child was already reaped? Actually I'm not sure there is; but I'd rather err on the side of caution here, a bunch of new scary-looking log messages can be very confusing for users. Ok. So better save than sorry. 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. I assume that your latest patch only differs from the previous one in the implementation of ap_mpm_safe_kill and the additional line in configure.in, right? Correct! So +1 from me on moving forward. Regards Rüdiger
Re: [PATCH] pid safety checks for 2.2.x
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
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 mod_scribble:
http://people.apache.org/~jorton/mod_scribble.c
Index: server/mpm/prefork/prefork.c
===
--- server/mpm/prefork/prefork.c(revision 549489)
+++ server/mpm/prefork/prefork.c(working copy)
@@ -1127,7 +1127,7 @@
for (index = 0; index ap_daemons_limit; ++index) {
if (ap_scoreboard_image-servers[index][0].status != SERVER_DEAD) {
/* Ask each child to close its listeners. */
-kill(MPM_CHILD_PID(index), AP_SIG_GRACEFUL);
+ap_mpm_safe_kill(MPM_CHILD_PID(index), AP_SIG_GRACEFUL);
active_children++;
}
}
@@ -1165,12 +1165,10 @@
active_children = 0;
for (index = 0; index ap_daemons_limit; ++index) {
-if (MPM_CHILD_PID(index) != 0) {
-if (kill(MPM_CHILD_PID(index), 0) == 0) {
-active_children = 1;
-/* Having just one child is enough to stay around
*/
-break;
-}
+if (ap_mpm_safe_kill(MPM_CHILD_PID(index), 0) == APR_SUCCESS) {
+active_children = 1;
+/* Having just one child is enough to stay around */
+break;
}
}
} while (!shutdown_pending active_children
@@ -1222,7 +1220,7 @@
* piped loggers, etc. They almost certainly won't handle
* it gracefully.
*/
-kill(ap_scoreboard_image-parent[index].pid, AP_SIG_GRACEFUL);
+ap_mpm_safe_kill(ap_scoreboard_image-parent[index].pid,
AP_SIG_GRACEFUL);
}
}
}
Index: server/mpm/worker/worker.c
===
--- server/mpm/worker/worker.c (revision 549489)
+++ server/mpm/worker/worker.c (working copy)
@@ -1813,12 +1813,10 @@
active_children = 0;
for (index = 0; index ap_daemons_limit; ++index) {
-if (MPM_CHILD_PID(index) != 0) {
-if (kill(MPM_CHILD_PID(index), 0) == 0) {
-active_children = 1;
-/* Having just one child is enough to stay around
*/
-break;
-}
+if (ap_mpm_safe_kill(MPM_CHILD_PID(index), 0) == APR_SUCCESS) {
+active_children = 1;
+/* Having just one child is enough to stay around */
+break;
}
}
} while (!shutdown_pending active_children
Index: server/mpm/experimental/event/event.c
===
--- server/mpm/experimental/event/event.c (revision 549489)
+++ server/mpm/experimental/event/event.c (working copy)
@@ -1998,12 +1998,10 @@
active_children = 0;
for (index = 0; index ap_daemons_limit; ++index) {
-if (MPM_CHILD_PID(index) != 0) {
-if (kill(MPM_CHILD_PID(index), 0) == 0) {
-active_children = 1;
-/* Having just one child is enough to stay around
*/
-break;
-}
+if (ap_mpm_safe_kill(MPM_CHILD_PID(index), 0) == APR_SUCCESS) {
+active_children = 1;
+/* Having just one child is enough to stay around */
+break;
}
}
} while (!shutdown_pending active_children
Index: server/mpm_common.c
===
--- server/mpm_common.c (revision 549489)
+++ server/mpm_common.c (working copy)
@@ -305,6 +305,27 @@
cur_extra = next;
}
}
+
+apr_status_t ap_mpm_safe_kill(pid_t pid, int sig)
+{
+apr_proc_t proc;
+apr_status_t waitret;
+
+/* Ensure the given pid is greater than zero; passing waitpid() a
+ * zero or negative pid has different semantics. */
+if (pid 1) {
+return APR_EINVAL;
+}
+
+proc.pid = pid;
+waitret = apr_proc_wait(proc, NULL, NULL, APR_NOWAIT);
+if (waitret == APR_CHILD_NOTDONE) {
+return kill(pid, sig) ? errno : APR_SUCCESS;
+}
+else {
+return APR_EINVAL;
+}
+}
#endif /* AP_MPM_WANT_RECLAIM_CHILD_PROCESSES */
#ifdef AP_MPM_WANT_WAIT_OR_TIMEOUT
Index: include/mpm_common.h
===
--- include/mpm_common.h
Re: [PATCH] pid safety checks for 2.2.x
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;
}
}
+
+apr_status_t ap_mpm_safe_kill(pid_t pid, int sig)
+{
+apr_proc_t proc;
+apr_status_t waitret;
+
+/* 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.
+if (pid 1) {
+return APR_EINVAL;
+}
+
+proc.pid = pid;
+waitret = apr_proc_wait(proc, NULL, NULL, APR_NOWAIT);
+if (waitret == APR_CHILD_NOTDONE) {
+return kill(pid, sig) ? errno : APR_SUCCESS;
+}
+else {
+return APR_EINVAL;
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.
Regards
Rüdiger
Re: [PATCH] pid safety checks for 2.2.x
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 patch...
Re: [PATCH] pid safety checks for 2.2.x
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 worker (on Linux) to fix the vulnerability using mod_scribble: 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.
