You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2007/06/27 19:52:37 UTC
[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 (revision 549489)
+++ include/mpm_common.h (working copy)
@@ -145,6 +145,17 @@
#endif
/**
+ * Safely signal an MPM child process, if the process is in the
+ * current process group. Otherwise fail.
+ * @param pid the process id of a child process to signal
+ * @param sig the signal number to send
+ * @return APR_SUCCESS if signal is sent, otherwise an error as per kill(3)
+ */
+#ifdef AP_MPM_WANT_RECLAIM_CHILD_PROCESSES
+apr_status_t ap_mpm_safe_kill(pid_t pid, int sig);
+#endif
+
+/**
* Determine if any child process has died. If no child process died, then
* this process sleeps for the amount of time specified by the MPM defined
* macro SCOREBOARD_MAINTENANCE_INTERVAL.
Re: [PATCH] pid safety checks for 2.2.x
Posted by Jim Jagielski <ji...@jaguNET.com>.
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
Posted by Plüm,
Rüdiger,
VF-Group <ru...@vodafone.com>.
> -----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
Posted by Joe Orton <jo...@redhat.com>.
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
Posted by Jim Jagielski <ji...@jaguNET.com>.
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
Posted by Joe Orton <jo...@redhat.com>.
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
Posted by Jim Jagielski <ji...@jaguNET.com>.
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...
Re: [PATCH] pid safety checks for 2.2.x
Posted by Plüm,
Rüdiger,
VF-Group <ru...@vodafone.com>.
> -----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
Posted by Joe Orton <jo...@redhat.com>.
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 (!shutdown_pending && active_children &&
Index: server/mpm_common.c
===================================================================
--- server/mpm_common.c (revision 549489)
+++ server/mpm_common.c (working copy)
@@ -305,6 +305,64 @@
cur_extra = next;
}
}
+
+/* Before sending the signal to the pid this function verifies that
+ * the pid is a member of the current process group; either using
+ * apr_proc_wait(), where waitpid() guarantees to fail for non-child
+ * processes; or by using getpgid() directly, if available. */
+apr_status_t ap_mpm_safe_kill(pid_t pid, int sig)
+{
+#ifndef HAVE_GETPGID
+ apr_proc_t proc;
+ apr_status_t rv;
+ apr_exit_why_e why;
+ int status;
+
+ if (pid < 1) {
+ return APR_EINVAL;
+ }
+
+ proc.pid = pid;
+ rv = apr_proc_wait(&proc, &status, &why, APR_NOWAIT);
+ if (rv == APR_CHILD_DONE) {
+#ifdef AP_MPM_WANT_PROCESS_CHILD_STATUS
+ /* The child already died - log the termination status if
+ * necessary: */
+ ap_process_child_status(&proc, why, status);
+#endif
+ return APR_EINVAL;
+ }
+ else if (rv != APR_CHILD_NOTDONE) {
+ /* The child is already dead and reaped, or was a bogus pid -
+ * log this either way. */
+ ap_log_error(APLOG_MARK, APLOG_NOTICE, rv, ap_server_conf,
+ "cannot send signal %d to pid %ld (non-child or "
+ "already dead)", sig, (long)pid);
+ return APR_EINVAL;
+ }
+#else
+ int pg;
+
+ if (pid < 1) {
+ return APR_EINVAL;
+ }
+
+ pg = getpgid(pid);
+ if (pg == -1) {
+ /* Process already dead... */
+ return errno;
+ }
+
+ if (pg != getpgrp()) {
+ ap_log_error(APLOG_MARK, APLOG_NOTICE, 0, ap_server_conf,
+ "refusing to send signal %d to pid %ld outside "
+ "process group", sig, (long)pid);
+ return APR_EINVAL;
+ }
+#endif
+
+ return kill(pid, sig) ? errno : APR_SUCCESS;
+}
#endif /* AP_MPM_WANT_RECLAIM_CHILD_PROCESSES */
#ifdef AP_MPM_WANT_WAIT_OR_TIMEOUT
Index: include/mpm_common.h
===================================================================
--- include/mpm_common.h (revision 549489)
+++ include/mpm_common.h (working copy)
@@ -145,6 +145,17 @@
#endif
/**
+ * Safely signal an MPM child process, if the process is in the
+ * current process group. Otherwise fail.
+ * @param pid the process id of a child process to signal
+ * @param sig the signal number to send
+ * @return APR_SUCCESS if signal is sent, otherwise an error as per kill(3)
+ */
+#ifdef AP_MPM_WANT_RECLAIM_CHILD_PROCESSES
+apr_status_t ap_mpm_safe_kill(pid_t pid, int sig);
+#endif
+
+/**
* Determine if any child process has died. If no child process died, then
* this process sleeps for the amount of time specified by the MPM defined
* macro SCOREBOARD_MAINTENANCE_INTERVAL.
Index: configure.in
===================================================================
--- configure.in (revision 549489)
+++ configure.in (working copy)
@@ -392,6 +392,7 @@
bindprocessor \
prctl \
timegm \
+getpgid
)
dnl confirm that a void pointer is large enough to store a long integer
Re: [PATCH] pid safety checks for 2.2.x
Posted by Ruediger Pluem <rp...@apache.org>.
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
Posted by Joe Orton <jo...@redhat.com>.
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
Posted by Jim Jagielski <ji...@jaguNET.com>.
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.