Re: PID table changes (was Re: svn commit: r547987 - in /httpd/httpd/trunk)
On Jun 21, 2007, at 1:18 PM, Colm MacCarthaigh wrote: On Thu, Jun 21, 2007 at 05:51:34PM +0100, Joe Orton wrote: On Sat, Jun 16, 2007 at 09:29:25PM -, Jim Jagielski 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. I actually thought avoiding this was a design goal, to prevent someone from killing piped loggers and cgi processes ? Yes, that's the case.
Re: svn commit: r550519 - in /httpd/httpd/trunk: docs/manual/mod/mod_proxy.html.en docs/manual/mod/mod_proxy.xml modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h modules/proxy/mod_proxy_balancer.c
Ruediger Pluem wrote: Ok, partly playing a bit of devils advocate below :-). On 06/25/2007 04:42 PM, [EMAIL PROTECTED] wrote: Author: jfclere Date: Mon Jun 25 07:42:25 2007 New Revision: 550519 URL: http://svn.apache.org/viewvc?view=revrev=550519 Log: Add sticky_path to solve PR41897. Modified: httpd/httpd/trunk/docs/manual/mod/mod_proxy.html.en httpd/httpd/trunk/docs/manual/mod/mod_proxy.xml httpd/httpd/trunk/modules/proxy/mod_proxy.c httpd/httpd/trunk/modules/proxy/mod_proxy.h httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.h?view=diffrev=550519r1=550518r2=550519 == --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original) +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Mon Jun 25 07:42:25 2007 @@ -365,6 +365,7 @@ apr_thread_mutex_t *mutex; /* Thread lock for updating lb params */ #endif void*context; /* general purpose storage */ +const char *sticky_path; /* URL sticky session identifier */ }; I am missing a minor bump, since this is part of a public API. Ok, furthermore I think we need to adjust the proxy_status_hook to actually display the string the user configured and not only the path for the cookie. The same is true for the balancer manager (display wise). Missing to split the string at the | in the balancer manager when you enter new data for sticky does not really worry me because setting a new sticky for the balancer (like all other balancer parameters) does not work anyway. We should really disable this. How? Keep the balancer-manager page as it is but disable that part. (form with fields read only and submit disabled). Cheers Jean-Frederic Just to avoid confusion: Changing parameters for a worker via the balancer manager works *well* (all this is stored in shared memory). Changing parameters for a balancer itself does *not* work as the balancer configuration is not stored in shared memory and thus the change only happens in that process that processed the according GET request to the balancer manager. Doing the right thing with the note session-sticky and the environment variable BALANCER_SESSION_STICKY seems to be a bit more tricky to me. At the point of time we are currently setting these we have already forgotten what we used for the route (sticky / sticky_path). OTOH we currently set session-sticky and BALANCER_SESSION_STICKY only in the case that we really find a worker. We do not set them if all workers for this route / all workers in general are in error state. So moving this inside find_session_route would change this behaviour. But unseting the note and the env variable in the case of all workers being in error state seems to be the perfect trigger in a few month for the question: Why the hell are we doing this?. So a change in behavior might be the better option here. As said: Devils advocate :-). Regards Rüdiger
Re: PID table changes (was Re: svn commit: r547987 - in /httpd/httpd/trunk)
On Jun 21, 2007, at 12:51 PM, Joe Orton wrote: Firstly my sincere apologies to Jim for bringing this up after such considerable work was put in already - I've had a busy month with a week out for a holiday :( 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. If: 1. The required getpgid/getpgrp functions are available on all current systems, and... 2. it provides as much protection as the more... exacting... check, then I have no real push one way or another...
Re: PID table changes (was Re: svn commit: r547987 - in /httpd/httpd/trunk)
On Jun 21, 2007, at 6:20 PM, William A. Rowe, Jr. wrote: Joe Orton wrote: On Thu, Jun 21, 2007 at 06:18:59PM +0100, Colm MacCarthaigh wrote: On Thu, Jun 21, 2007 at 05:51:34PM +0100, Joe Orton wrote: On Sat, Jun 16, 2007 at 09:29:25PM -, Jim Jagielski 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. I actually thought avoiding this was a design goal, to prevent someone from killing piped loggers and cgi processes ? What's the security threat there? Given that the attacker can arrange for arbitrary execution of code in any unprivileged child, preventing logging or CGI script execution is possible anyway. Two different cases. It will be trivial to kill the CGI scripts launched from the process (not from cgid) for other children in, say, a worker config. The cgid and logging processes spawned by the parents aren't vulnerable as such (and the logging processes are spawned with the httpd's launching uid anyways, not nobody, IIRC.) We only care about constraining the fallout of a malicious in process script to that specific process, and the loggers, if we go beyond testing the pid group, won't have that issue from the child process. So I agree with testing the pid groups, I also believe it is worthwhile to sanity test any critical data from the shared-scoreboard against the parent processes' private reference table. So what's the word... should we back out all the pid table stuff (from both 1.3 and 2.x) and wait for Joe to provide his pgrp changes (including any required configure magic to detect function existance) or what? It's kind of embarrassing that this is taking so long :)
Re: PID table changes (was Re: svn commit: r547987 - in /httpd/httpd/trunk)
Jim Jagielski wrote: So what's the word... should we back out all the pid table stuff (from both 1.3 and 2.x) and wait for Joe to provide his pgrp changes (including any required configure magic to detect function existance) or what? As you wrote... If: 1. The required getpgid/getpgrp functions are available on all current systems, and... 2. it provides as much protection as the more... exacting... check, then I doubt we will find process group tree portability across all of our posix-like OS's - and it does not provide protection against one of our children assigning a child-id scoreboard slot to a piped logger, the cgid process, etc. So I think we -don't- want the existing code to be backed out. Simplified, perhaps, but the basic logic is sound. And if the presence of getpgid/getpgrp functions lets us add a second sanity check; all the better! Bill
Re: PID table changes (was Re: svn commit: r547987 - in /httpd/httpd/trunk)
My summary: I've still not seen any argument why it presents a security risk for a malicious child to be able to kill a piped logger or other non-MPM-spawned process, so: 1) for 2.2.x and 1.3.x apr_proc_wait()/waitpid() can be used instead of getpgid(pid) == getpgrp() to determine whether the pid-to-kill is a child of the parent - this doesn't present a portability risk. I will produce a new patch for that tomorrow. 2) for 2.0.x there is no security issue to fix joe
Re: svn commit: r550519 - in /httpd/httpd/trunk: docs/manual/mod/mod_proxy.html.en docs/manual/mod/mod_proxy.xml modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h modules/proxy/mod_proxy_balancer.c
On 06/26/2007 06:46 PM, jean-frederic clere wrote:
Ruediger Pluem wrote:
Ok, furthermore I think we need to adjust the proxy_status_hook to
actually display the string the user configured and not only the path
for the cookie. The same is true for the balancer manager (display wise).
Missing to split the string at the | in the balancer manager when you
enter new data for sticky does not really worry me because setting
a new sticky for the balancer (like all other balancer parameters)
does not work anyway. We should really disable this.
How? Keep the balancer-manager page as it is but disable that part.
(form with fields read only and submit disabled).
I am thinking more about removing it completely like in the attached patch,
because
1. All information is also displayed without the form.
2. If it turns out that we need the form for the balancer again, because we
adjusted
the balancer data structures in a way that changes via the balancer-manager
work,
we can fetch it back from svn. IMHO no need to keep dead code.
Regards
Rüdiger
Index: modules/proxy/mod_proxy_balancer.c
===
--- modules/proxy/mod_proxy_balancer.c (Revision 550713)
+++ modules/proxy/mod_proxy_balancer.c (Arbeitskopie)
@@ -649,33 +649,6 @@
}
}
/* First set the params */
-if (bsel) {
-const char *val;
-if ((val = apr_table_get(params, ss))) {
-if (strlen(val))
-bsel-sticky = apr_pstrdup(conf-pool, val);
-else
-bsel-sticky = NULL;
-}
-if ((val = apr_table_get(params, tm))) {
-int ival = atoi(val);
-if (ival = 0)
-bsel-timeout = apr_time_from_sec(ival);
-}
-if ((val = apr_table_get(params, fa))) {
-int ival = atoi(val);
-if (ival = 0)
-bsel-max_attempts = ival;
-bsel-max_attempts_set = 1;
-}
-if ((val = apr_table_get(params, lm))) {
-proxy_balancer_method *provider;
-provider = ap_lookup_provider(PROXY_LBMETHOD, val, 0);
-if (provider) {
-bsel-lbmethod = provider;
-}
-}
-}
if (wsel) {
const char *val;
if ((val = apr_table_get(params, lf))) {
@@ -755,10 +728,7 @@
for (i = 0; i conf-balancers-nelts; i++) {
ap_rputs(hr /\nh3LoadBalancer Status for , r);
-ap_rvputs(r, a href=\, r-uri, ?b=,
- balancer-name + sizeof(balancer://) - 1,
- \, NULL);
-ap_rvputs(r, balancer-name, /a/h3\n\n, NULL);
+ap_rvputs(r, balancer-name, /h3\n\n, NULL);
ap_rputs(\n\ntable border=\0\ style=\text-align: left;\tr
thStickySession/ththTimeout/ththFailoverAttempts/ththMethod/th
/tr\ntr, r);
@@ -843,41 +813,6 @@
\\n/form\n, NULL);
ap_rputs(hr /\n, r);
}
-else if (bsel) {
-ap_rputs(h3Edit balancer settings for , r);
-ap_rvputs(r, bsel-name, /h3\n, NULL);
-ap_rvputs(r, form method=\GET\ action=\, NULL);
-ap_rvputs(r, r-uri, \\ndl, NULL);
-ap_rputs(tabletrtdStickySession Identifier:/tdtdinput name=\ss\ type=text , r);
-if (bsel-sticky)
-ap_rvputs(r, value=\, bsel-sticky, \, NULL);
-ap_rputs(/tdtr\ntrtdTimeout:/tdtdinput name=\tm\ type=text , r);
-ap_rprintf(r, value=\% APR_TIME_T_FMT \/td/tr\n,
- apr_time_sec(bsel-timeout));
-ap_rputs(trtdFailover Attempts:/tdtdinput name=\fa\ type=text , r);
-ap_rprintf(r, value=\%d\/td/tr\n,
- bsel-max_attempts);
-ap_rputs(trtdLB Method:/tdtdselect name=\lm\, r);
-{
-apr_array_header_t *methods;
-ap_list_provider_names_t *method;
-int i;
-methods = ap_list_provider_names(r-pool, PROXY_LBMETHOD, 0);
-method = (ap_list_provider_names_t *)methods-elts;
-for (i = 0; i methods-nelts; i++) {
-ap_rprintf(r, option value=\%s\ %s%s/option, method-provider_name,
- (!strcasecmp(bsel-lbmethod-name, method-provider_name)) ? selected : ,
- method-provider_name);
-method++;
-}
-}
-ap_rputs(/select/td/tr\n, r);
-ap_rputs(trtd colspan=2input type=submit value=\Submit\/td/tr\n, r);
-ap_rvputs(r, /table\ninput type=hidden name=\b\ , NULL);
-ap_rvputs(r, value=\, bsel-name + sizeof(balancer://) - 1,
- \\n/form\n, NULL);
-ap_rputs(hr /\n, r);
-}
ap_rputs(ap_psignature(,r), r);
ap_rputs(/body/html\n, r);
}
Re: PID table changes (was Re: svn commit: r547987 - in /httpd/httpd/trunk)
On 06/26/2007 08:37 PM, Joe Orton wrote: My summary: I've still not seen any argument why it presents a security risk for a malicious child to be able to kill a piped logger or other non-MPM-spawned process, so: What about signals other than SIGKILL and SIGTERM? We also send SIGUSR1 in some cases. Can this signal create any harm that could not be created otherwise by the malicious child when sent to 1. A piped logger program (could be 3rd party). 2. A CGI script started with suexec. Regarding the piped logger: I would guess that a malicious child can disable logging for itself by closing the fd of the piped logger. IMHO this is even harder to detect for the admin than a killed logger. Regarding other processes I think the malicious child can send any signal to them anyway as long as they are running with the same user id as the child. IMHO the advantage of the PID table is that it opens the possibility for further sanity checks of the scoreboard, especially for cross checking how many childs we really have. OTOH if I think about it more closely it is questionable if the added overhead is really worth it, because a malicious child at least can create a fork bomb without the help of the scoreboard. Regards Rüdiger
