Re: svn commit: r1534015 - /httpd/httpd/trunk/server/main.c
On Wed, Oct 23, 2013 at 1:27 AM, William A. Rowe Jr. wmr...@gmail.comwrote:
On Oct 22, 2013 5:14 PM, Yann Ylavic ylavic@gmail.com wrote:
Shouldn't this be safe from terminal controls, eg :
const char *name = process-short_name;
if (!name ||
!*name ||
ap_has_cntrl(name)) {
name = httpd;
}
?
No. You are thinking of untrusted user input. The Admin started this
process under the given name. Describe how this can be devolved to a
vulnerability?
No particular vulnerability (in sane circumstances, ie. the Admin is not
given an evil name), and not an httpd vulnerability anyway, but the usual
ap_log_error(...STARTUP...) does escape control chars, which make this code
the only place where some given data is put direclty to the terminal. I
can probably live with it...
Regards.
Re: stop copying footers to r-headers_in?
Should I continue this way? Simply to propose patches?
On Sat, Oct 19, 2013 at 4:13 PM, Eric Covener cove...@gmail.com wrote:
Currently, when the body is consumed by a handler, a side effect is
reading footers that might be present and copying them to
r-headers_in.
This presents a series of problems.
* things that inspect r-headers_in expect it to be fluffed up much
earlier than midway through the handler phase
* if the handler looks at headers before reading the body, they could
differ from what's logged
* if the handler looks at headers after reading the body, mod_headers
was out of the loop if configured.
I am thinking:
now:
1) add r-footers_in and use it in 2.2 and up by default
Do that mean no API/ABI change ?
2) add a directive to copy them up to r-headers_in (for those broken
by the change)
soon:
3) add a hook to parse footers
later:
4) try to teach mod_headers to do something useful with that hook, but
not with existing directives.
5) teach mod_log_config to log from footers_in
Not done (yet), maybe I could try or am I completely bogged down with the
design?
Maybe it worth also :
6) teach ap_expr/ssl_lookup to interpolate from footers
7) teach mod_rewrite (hooked) how to play with footers
8) teach mod_proxy_http to forward the footers
A proposition for 8) follows.
Regards.
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c(revision 1534970)
+++ modules/proxy/proxy_util.c(working copy)
@@ -3114,11 +3114,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
char **old_te_val)
{
conn_rec *c = r-connection;
-int counter;
char *buf;
-const apr_array_header_t *headers_in_array;
-const apr_table_entry_t *headers_in;
-apr_table_t *headers_in_copy;
apr_bucket *e;
int do_100_continue;
conn_rec *origin = p_conn-connection;
@@ -3278,19 +3274,42 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
}
proxy_run_fixups(r);
+return ap_proxy_fill_hdrbrgd(p, header_brigade, r-headers_in, r,
+ old_cl_val, old_te_val);
+}
+
+PROXY_DECLARE(int) ap_proxy_fill_hdrbrgd(apr_pool_t *p,
+ apr_bucket_brigade
*header_brigade,
+ const apr_table_t *header_table,
+ request_rec *r, char **old_cl_val,
+ char **old_te_val)
+{
+const apr_array_header_t *headers_in_array;
+const apr_table_entry_t *headers_in;
+apr_table_t *headers_in_copy;
+apr_bucket *e;
+int counter;
+char *buf;
+
+/* easy path first */
+if (apr_is_empty_table(header_table)) {
+return OK;
+}
+
/*
- * Make a copy of the headers_in table before clearing the connection
+ * Make a copy of the header_table before clearing the connection
* headers as we need the connection headers later in the http output
* filter to prepare the correct response headers.
*
* Note: We need to take r-pool for apr_table_copy as the key / value
- * pairs in r-headers_in have been created out of r-pool and
- * p might be (and actually is) a longer living pool.
+ * pairs in header_table (eg. r-headers/footers_in) have been created
+ * out of r-pool and p might be (and actually is) a longer living
pool.
* This would trigger the bad pool ancestry abort in apr_table_copy if
* apr is compiled with APR_POOL_DEBUG.
*/
-headers_in_copy = apr_table_copy(r-pool, r-headers_in);
+headers_in_copy = apr_table_copy(r-pool, header_table);
ap_proxy_clear_connection(r, headers_in_copy);
+
/* send request headers */
headers_in_array = apr_table_elts(headers_in_copy);
headers_in = (const apr_table_entry_t *) headers_in_array-elts;
@@ -3317,7 +3336,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
* If we have used it then MAYBE: RFC2616 says we MAY propagate it.
* So let's make it configurable by env.
*/
-if (!strcasecmp(headers_in[counter].key,Proxy-Authorization)) {
+if (!strcasecmp(headers_in[counter].key, Proxy-Authorization)) {
if (r-user != NULL) { /* we've authenticated */
if (!apr_table_get(r-subprocess_env, Proxy-Chain-Auth))
{
continue;
@@ -3328,17 +3347,21 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
/* Skip Transfer-Encoding and Content-Length for now.
*/
if (!strcasecmp(headers_in[counter].key, Transfer-Encoding)) {
-*old_te_val = headers_in[counter].val;
+if (old_te_val) {
+*old_te_val = headers_in[counter].val;
+}
continue;
}
if (!strcasecmp(headers_in[counter].key, Content-Length)) {
-*old_cl_val =
Re: [PATCH 55593] Add SSLServerInfoFile directive
On 21.10.2013 06:09, Trevor Perrin wrote: I looked at your patch. Besides lack of passphrase-handling, it breaks compatibility with existing config files (which assume certs/keys are matched by type, not order). I don't think that random order of multiple SSLCertificateFile and SSLCertificateKeyFile directives is a feature which has ever been promised in our docs. In those rare cases where people are currently configuring more than one cert per vhost, I would be quite surprised to see a config where the order of the SSLCertificateKeyFile directive does not match the one of the SSLCertificateFile directives. That would work, but someone would have to rewrite all the passphrase-handling code, Correct, and an overhaul of ssl_engine_pphrase.c is actually long due, so we shouldn't introduce more band-aid-style workarounds. and users would have to switch to a new set of commands for working with certs / keys. No, that's not what I had in mind. SSLCertificateFile and SSLCertificateKeyFile can stay (not the least to support existing configs), but SSLOpenSSLConfCmd would offer an additional way of configuring certs and keys, e.g. for those cases where more per-cert tweaking is desired. Seems like a lot of work. For example, how would the generic SSLConfCmd commands get hooked-up with passphrase handling for the key files? Based on Steve's recent work on OpenSSL-master (PrivateKey SSL_CONF command), we might reuse the ssl_pphrase_Handle_CB() callback and set it for use with SSL_CTX_set_default_passwd_cb(). This needs further examination, though. Redesigning and reimplementing all of mod_ssl's cert / key handling around SSLConfCmd is a bigger task than I can handle. I intend to work on that, and have already done some experiments with a modified/trimmed down version of ssl_pphrase_Handle(), which only keeps the tPrivateKey hash in the global SSLModConfigRec. But I still wonder if a ServerInfoFile directive would be worthwhile, in the meantime. I don't see a justification for doing that at this time, also when considering that we're currently coding against OpenSSL 1.0.2, which isn't released yet. Kaspar
Re: [PATCH 55593] Add SSLServerInfoFile directive
On 22.10.2013 22:04, Dr Stephen Henson wrote: Only bit I'm not completely sure about is the use of the SSL_CONF_CTX structure in modssl_ctx_t. It's done that way to avoid having to keep creating and destroying the SSL_CONF_CTX for each directive but a quick test showed it was creating several other SSL_CONF_CTX structures which were never used. Right now, the SSL_CONF_CTX_* handling is in ssl_init_ctx_protocol, which is called once for each vhost (and each vhost has its own modssl_ctx_t), so the change you applied with r1534754 doesn't really change much as far as the SSL_CONF_CTX structure handling is concerned, I think. To prevent unnecessary SSL_CONF_CTX structures from being created, it should be sufficient to enclose that block with an if (mctx-ssl_ctx_param-nelts 0) condition, IINM. Kaspar
Re: [PATCH 55593] Add SSLServerInfoFile directive
On 23/10/2013 15:30, Kaspar Brand wrote: On 22.10.2013 22:04, Dr Stephen Henson wrote: Only bit I'm not completely sure about is the use of the SSL_CONF_CTX structure in modssl_ctx_t. It's done that way to avoid having to keep creating and destroying the SSL_CONF_CTX for each directive but a quick test showed it was creating several other SSL_CONF_CTX structures which were never used. Right now, the SSL_CONF_CTX_* handling is in ssl_init_ctx_protocol, which is called once for each vhost (and each vhost has its own modssl_ctx_t), so the change you applied with r1534754 doesn't really change much as far as the SSL_CONF_CTX structure handling is concerned, I think. To prevent unnecessary SSL_CONF_CTX structures from being created, it should be sufficient to enclose that block with an if (mctx-ssl_ctx_param-nelts 0) condition, IINM. Well the handling remains in ssl_init_ctx_protocol but now an SSL_CONF_CTX with appropriate flags is created in moddssl_ctx_init. That is done because a valid SSL_CONF_CTX is needed to call SSL_CONF_cmd_value_type in ssl_cmd_SSLOpenSSLConfCmd. So my thought was (if unnecessary SSL_CONF_CTX creation is a problem) change the modssl_ctx_init to just set mctx-ssl_ctx_config to NULL and instead create a new SSL_CONF_CTX in ssl_cmd_SSLOpenSSLConfCmd if mctx-ssl_ctx_config is NULL. Steve. -- Dr Stephen Henson. OpenSSL Software Foundation, Inc. 1829 Mount Ephraim Road Adamstown, MD 21710 +1 877-673-6775 shen...@opensslfoundation.com
Re: [PATCH 55593] Add SSLServerInfoFile directive
On 23.10.2013 16:48, Dr Stephen Henson wrote: Well the handling remains in ssl_init_ctx_protocol but now an SSL_CONF_CTX with appropriate flags is created in moddssl_ctx_init. That is done because a valid SSL_CONF_CTX is needed to call SSL_CONF_cmd_value_type in ssl_cmd_SSLOpenSSLConfCmd. So my thought was (if unnecessary SSL_CONF_CTX creation is a problem) change the modssl_ctx_init to just set mctx-ssl_ctx_config to NULL and instead create a new SSL_CONF_CTX in ssl_cmd_SSLOpenSSLConfCmd if mctx-ssl_ctx_config is NULL. Ah, ok, I was missing the point of SSL_CONF_CTX now being necessary in ssl_cmd_SSLOpenSSLConfCmd. Creating it on first use then seems like a reasonable approach. Perhaps it would also make sense to free SSL_CONF_CTX in ssl_init_ctx_protocol after having called SSL_CONF_CTX_finish (as was done before r1534754)? Kaspar
