On 06/21/2007 06:51 PM, Joe Orton wrote:
>
> Secondly: I think this approach is unnecessarily complex. I think it's
> sufficient to simply check whether the target process is in the right
> process group before sending a signal, i.e. getpgid(pid) == getpgrp().
> This ensures that the parent will only kill things it created.
>
> It is reasonable to assume that the parent's process group holds exactly
> the set of processes which is safe to kill - prefork relies on that
> being so when handling SIGHUP already.
>
> Patch below is PoC.
So I assume the patches for the other MPM's will follow.
BTW: Do we have getpgrp / getpgid on all these systems?
>
> Thirdly: an implementation problem which I think needs addressing
> regardless:
>
> + if (ap_in_pid_table(MPM_CHILD_PID(index))) {
> + if (kill(MPM_CHILD_PID(index), 0) == 0) {
>
> and similar has a potential race; both calls could result in the pid
> being read back from the shm segment, and hence the malicious child
> could change from a "good" pid to a "bad" one in between. I don't think
> it's guaranteed that even using a temporary variable is safe; an
> out-of-line function call passed MPM_CHILD_PID(index) is best. (as in
> the ap_mpm_safe_kill() function in my patch)
Thanks for this good catch. At least I missed that.
>
> Here's the patch (against 2.2.x):
>
> Index: server/mpm/prefork/mpm.h
> ===================================================================
> --- server/mpm/prefork/mpm.h (revision 549489)
> +++ server/mpm/prefork/mpm.h (working copy)
> @@ -53,6 +53,7 @@
> #define AP_MPM_USES_POD 1
> #define MPM_CHILD_PID(i) (ap_scoreboard_image->parent[i].pid)
> #define MPM_NOTE_CHILD_KILLED(i) (MPM_CHILD_PID(i) = 0)
> +#define MPM_VALID_PID(p) (getpgid(p) == getpgrp())
> #define MPM_ACCEPT_FUNC unixd_accept
>
> extern int ap_threads_per_child;
> Index: server/mpm/prefork/prefork.c
> ===================================================================
> --- server/mpm/prefork/prefork.c (revision 549489)
> +++ server/mpm/prefork/prefork.c (working copy)
Maybe nitpicking, but I think you missed one kill at about line 310 in
reap_children.
Regards
Rüdiger
