On Monday 18 October 2010 12:28:12 Malte S. Stretz wrote:
> On Tuesday 12 October 2010 19:49:02 Malte S. Stretz wrote:
> > On Tuesday 12 October 2010 18:13:46 William A. Rowe Jr. wrote:
> > > On 10/12/2010 10:06 AM, Dirk-Willem van Gulik wrote:
> > > > On 12 Oct 2010, at 15:30, Malte S. Stretz wrote:
> > > >> I had a quick look at the Apache source and the solution was
> > > >> simple: Just drop headers which contain any character outside the
> > > >> range [a-zA-Z0-9-]. The patch against trunk is attached.
> > > >
> > > > This made me think of something we had a while ago; and after
> > > > checking the logs - big +1 from me!
> > >
> > > Agreed, with a caviat... we aught to be able to toggle this for the
> > > rare but significant legacy app that requires it... which implies a
> > > per-dir flag that can override just one CGI script out of an entire
> > > server.
> >
> > I think an option is not needed as there is a workaround. Eg. to
> > make an Accept_Encoding header work:
> >
> > SetEnvIfNoCase ^Accept.Encoding$ ^(.*)$ fix_header=$1
> > RequestHeader set Accept-Encoding %{fix_header}e env=fix_header
> >
> >[...]
>
> Attached is an updated patch which also updates the docs. It also
> includes the commit message I tried to commit it with (didn't realize
> that there are per-project commit flags).
>
> Is the documented workaround good enough or should something like an
> map- broken-headers environment variable be introduced?
Time flies by... :)
As it seems like an option is preferred to a workaround, here's are a
bunch of patches. The first implements an option (an environment variable
map-invalid-headers) to switch on the backwards compatibility. It got
delayed because I didn't write any documentation yet. I'll do so if it
gets accepted :)
The second patch transforms the #ifdef check against Auth headers into an
option, too (option map-authorization-headers). If it is accepted, I'll
write some documentation, highlighting why it is a bad idea, too. I added
it because I think an option is better than an obscure #ifdef which opens
up a rarely tested code path.
And finally, the third patch is a bit bigger, it contains a bunch of
refactoring, moving some stuff to helper routines, getting rid of unneeded
variables (which probably would have been optimized away anyway) and
adding some comments. It is probably easier to follow if you have a look
at the single commits at Github:
https://github.com/mss/httpd/compare/more-env-work
If you prefer the old way (a documented workaround instead of an option),
I rebased that code against trunk (two days old), so it applies again:
https://github.com/mss/httpd/compare/strict-env-work
Cheers,
Malte
diff --git a/server/util_script.c b/server/util_script.c
index f5e4ef1..f2335ee 100644
--- a/server/util_script.c
+++ b/server/util_script.c
@@ -54,7 +54,7 @@
APLOG_USE_MODULE(core);
-static char *http2env(apr_pool_t *a, const char *w)
+static char *http2env(apr_pool_t *a, const char *w, int sloppy)
{
char *res = (char *)apr_palloc(a, sizeof("HTTP_") + strlen(w));
char *cp = res;
@@ -67,11 +67,17 @@ static char *http2env(apr_pool_t *a, const char *w)
*cp++ = '_';
while ((c = *w++) != 0) {
- if (!apr_isalnum(c)) {
+ if (apr_isalnum(c)) {
+ *cp++ = apr_toupper(c);
+ }
+ else if (c == '-') {
+ *cp++ = '_';
+ }
+ else if (sloppy) {
*cp++ = '_';
}
else {
- *cp++ = apr_toupper(c);
+ return NULL;
}
}
*cp = 0;
@@ -128,6 +134,7 @@ AP_DECLARE(void) ap_add_common_vars(request_rec *r)
const char *host;
const apr_array_header_t *hdrs_arr = apr_table_elts(r->headers_in);
const apr_table_entry_t *hdrs = (const apr_table_entry_t *) hdrs_arr->elts;
+ int sloppy;
int i;
apr_port_t rport;
@@ -146,8 +153,14 @@ AP_DECLARE(void) ap_add_common_vars(request_rec *r)
/* First, add environment vars from headers... this is as per
* CGI specs, though other sorts of scripting interfaces see
* the same vars...
+ *
+ * A minimum of validation is applied and headers which blatantly
+ * violate the RFC are silently dropped. To map these as well,
+ * the environment variable map-invalid-headers can be set.
*/
+ sloppy = apr_table_get(r->subprocess_env, "map-invalid-headers") ? 1 : 0;
+
for (i = 0; i < hdrs_arr->nelts; ++i) {
if (!hdrs[i].key) {
continue;
@@ -175,8 +188,8 @@ AP_DECLARE(void) ap_add_common_vars(request_rec *r)
continue;
}
#endif
- else {
- apr_table_addn(e, http2env(r->pool, hdrs[i].key), hdrs[i].val);
+ else if ((env_temp = http2env(r->pool, hdrs[i].key, sloppy)) != NULL) {
+ apr_table_addn(e, env_temp, hdrs[i].val);
}
}
diff --git a/server/util_script.c b/server/util_script.c
index f2335ee..89506aa 100644
--- a/server/util_script.c
+++ b/server/util_script.c
@@ -178,16 +178,15 @@ AP_DECLARE(void) ap_add_common_vars(request_rec *r)
apr_table_addn(e, "CONTENT_LENGTH", hdrs[i].val);
}
/*
- * You really don't want to disable this check, since it leaves you
- * wide open to CGIs stealing passwords and people viewing them
- * in the environment with "ps -e". But, if you must...
+ * Disabling this check leaves you wide open to CGIs stealing
+ * passwords and people viewing them in the environment with
+ * "ps -e". But, if you must...
*/
-#ifndef SECURITY_HOLE_PASS_AUTHORIZATION
- else if (!strcasecmp(hdrs[i].key, "Authorization")
- || !strcasecmp(hdrs[i].key, "Proxy-Authorization")) {
+ else if ((!strcasecmp(hdrs[i].key, "Authorization")
+ || !strcasecmp(hdrs[i].key, "Proxy-Authorization"))
+ && !apr_table_get(r->subprocess_env, "map-authorization-headers")) {
continue;
}
-#endif
else if ((env_temp = http2env(r->pool, hdrs[i].key, sloppy)) != NULL) {
apr_table_addn(e, env_temp, hdrs[i].val);
}
diff --git a/server/util_script.c b/server/util_script.c
index 89506aa..9f52766 100644
--- a/server/util_script.c
+++ b/server/util_script.c
@@ -54,36 +54,6 @@
APLOG_USE_MODULE(core);
-static char *http2env(apr_pool_t *a, const char *w, int sloppy)
-{
- char *res = (char *)apr_palloc(a, sizeof("HTTP_") + strlen(w));
- char *cp = res;
- char c;
-
- *cp++ = 'H';
- *cp++ = 'T';
- *cp++ = 'T';
- *cp++ = 'P';
- *cp++ = '_';
-
- while ((c = *w++) != 0) {
- if (apr_isalnum(c)) {
- *cp++ = apr_toupper(c);
- }
- else if (c == '-') {
- *cp++ = '_';
- }
- else if (sloppy) {
- *cp++ = '_';
- }
- else {
- return NULL;
- }
- }
- *cp = 0;
-
- return res;
-}
AP_DECLARE(char **) ap_create_environment(apr_pool_t *p, apr_table_t *t)
{
@@ -106,6 +76,9 @@ AP_DECLARE(char **) ap_create_environment(apr_pool_t *p, apr_table_t *t)
continue;
}
env[j] = apr_pstrcat(p, elts[i].key, "=", elts[i].val, NULL);
+ /* Names of environment variables must contain only characters,
+ * numbers and underscores and must not begin with a number.
+ */
whack = env[j];
if (apr_isdigit(*whack)) {
*whack++ = '_';
@@ -123,20 +96,53 @@ AP_DECLARE(char **) ap_create_environment(apr_pool_t *p, apr_table_t *t)
return env;
}
+static void http2env(apr_pool_t *p, apr_table_t *e, const char *k,
+ const char *v, int s)
+{
+ char *nk = (char *)apr_palloc(p, sizeof("HTTP_") + strlen(k));
+ char *cp = nk;
+ char c;
+
+ *cp++ = 'H';
+ *cp++ = 'T';
+ *cp++ = 'T';
+ *cp++ = 'P';
+ *cp++ = '_';
+
+ while ((c = *k++) != 0) {
+ if (apr_isalnum(c)) {
+ *cp++ = apr_toupper(c);
+ }
+ else if ((c == '-') || s) {
+ *cp++ = '_';
+ }
+ else {
+ return;
+ }
+ }
+ *cp = 0;
+
+ apr_table_addn(e, nk, v);
+}
+
+static void env2env(apr_table_t *e, const char *k)
+{
+ char *v = getenv(k);
+ if (v) {
+ apr_table_addn(e, k, v);
+ }
+}
+
AP_DECLARE(void) ap_add_common_vars(request_rec *r)
{
apr_table_t *e;
server_rec *s = r->server;
conn_rec *c = r->connection;
- const char *rem_logname;
- const char *env_path;
const char *env_temp;
- const char *host;
const apr_array_header_t *hdrs_arr = apr_table_elts(r->headers_in);
const apr_table_entry_t *hdrs = (const apr_table_entry_t *) hdrs_arr->elts;
int sloppy;
int i;
- apr_port_t rport;
/* use a temporary apr_table_t which we'll overlap onto
* r->subprocess_env later
@@ -187,70 +193,42 @@ AP_DECLARE(void) ap_add_common_vars(request_rec *r)
&& !apr_table_get(r->subprocess_env, "map-authorization-headers")) {
continue;
}
- else if ((env_temp = http2env(r->pool, hdrs[i].key, sloppy)) != NULL) {
- apr_table_addn(e, env_temp, hdrs[i].val);
+ else {
+ http2env(r->pool, e, hdrs[i].key, hdrs[i].val, sloppy);
}
}
- env_path = apr_table_get(r->subprocess_env, "PATH");
- if (env_path == NULL) {
- env_path = getenv("PATH");
+ env_temp = apr_table_get(r->subprocess_env, "PATH");
+ if (env_temp == NULL) {
+ env_temp = getenv("PATH");
}
- if (env_path == NULL) {
- env_path = DEFAULT_PATH;
+ if (env_temp == NULL) {
+ env_temp = DEFAULT_PATH;
}
- apr_table_addn(e, "PATH", apr_pstrdup(r->pool, env_path));
+ apr_table_addn(e, "PATH", apr_pstrdup(r->pool, env_temp));
#if defined(WIN32)
- if (env_temp = getenv("SystemRoot")) {
- apr_table_addn(e, "SystemRoot", env_temp);
- }
- if (env_temp = getenv("COMSPEC")) {
- apr_table_addn(e, "COMSPEC", env_temp);
- }
- if (env_temp = getenv("PATHEXT")) {
- apr_table_addn(e, "PATHEXT", env_temp);
- }
- if (env_temp = getenv("WINDIR")) {
- apr_table_addn(e, "WINDIR", env_temp);
- }
+ env2env(e, "SystemRoot");
+ env2env(e, "COMSPEC");
+ env2env(e, "PATHEXT");
+ env2env(e, "WINDIR");
#elif defined(OS2)
- if ((env_temp = getenv("COMSPEC")) != NULL) {
- apr_table_addn(e, "COMSPEC", env_temp);
- }
- if ((env_temp = getenv("ETC")) != NULL) {
- apr_table_addn(e, "ETC", env_temp);
- }
- if ((env_temp = getenv("DPATH")) != NULL) {
- apr_table_addn(e, "DPATH", env_temp);
- }
- if ((env_temp = getenv("PERLLIB_PREFIX")) != NULL) {
- apr_table_addn(e, "PERLLIB_PREFIX", env_temp);
- }
+ env2env(e, "COMSPEC");
+ env2env(e, "ETC");
+ env2env(e, "DPATH");
+ env2env(e, "PERLLIB_PREFIX");
#elif defined(BEOS)
- if ((env_temp = getenv("LIBRARY_PATH")) != NULL) {
- apr_table_addn(e, "LIBRARY_PATH", env_temp);
- }
+ env2env(e, "LIBRARY_PATH");
#elif defined(DARWIN)
- if ((env_temp = getenv("DYLD_LIBRARY_PATH")) != NULL) {
- apr_table_addn(e, "DYLD_LIBRARY_PATH", env_temp);
- }
+ env2env(e, "DYLD_LIBRARY_PATH");
#elif defined(_AIX)
- if ((env_temp = getenv("LIBPATH")) != NULL) {
- apr_table_addn(e, "LIBPATH", env_temp);
- }
+ env2env(e, "LIBPATH");
#elif defined(__HPUX__)
/* HPUX PARISC 2.0W knows both, otherwise redundancy is harmless */
- if ((env_temp = getenv("SHLIB_PATH")) != NULL) {
- apr_table_addn(e, "SHLIB_PATH", env_temp);
- }
- if ((env_temp = getenv("LD_LIBRARY_PATH")) != NULL) {
- apr_table_addn(e, "LD_LIBRARY_PATH", env_temp);
- }
+ env2env(e, "SHLIB_PATH");
+ env2env(e, "LD_LIBRARY_PATH");
#else /* Some Unix */
- if ((env_temp = getenv("LD_LIBRARY_PATH")) != NULL) {
- apr_table_addn(e, "LD_LIBRARY_PATH", env_temp);
- }
+ env2env(e, "LD_LIBRARY_PATH");
#endif
apr_table_addn(e, "SERVER_SIGNATURE", ap_psignature("", r));
@@ -260,17 +238,17 @@ AP_DECLARE(void) ap_add_common_vars(request_rec *r)
apr_table_addn(e, "SERVER_ADDR", r->connection->local_ip); /* Apache */
apr_table_addn(e, "SERVER_PORT",
apr_psprintf(r->pool, "%u", ap_get_server_port(r)));
- host = ap_get_remote_host(c, r->per_dir_config, REMOTE_HOST, NULL);
- if (host) {
- apr_table_addn(e, "REMOTE_HOST", host);
+ env_temp = ap_get_remote_host(c, r->per_dir_config, REMOTE_HOST, NULL);
+ if (env_temp) {
+ apr_table_addn(e, "REMOTE_HOST", env_temp);
}
apr_table_addn(e, "REMOTE_ADDR", c->remote_ip);
apr_table_addn(e, "DOCUMENT_ROOT", ap_document_root(r)); /* Apache */
apr_table_addn(e, "SERVER_ADMIN", s->server_admin); /* Apache */
apr_table_addn(e, "SCRIPT_FILENAME", r->filename); /* Apache */
- rport = c->remote_addr->port;
- apr_table_addn(e, "REMOTE_PORT", apr_itoa(r->pool, rport));
+ env_temp = apr_itoa(r->pool, c->remote_addr->port);
+ apr_table_addn(e, "REMOTE_PORT", env_temp);
if (r->user) {
apr_table_addn(e, "REMOTE_USER", r->user);
@@ -289,9 +267,9 @@ AP_DECLARE(void) ap_add_common_vars(request_rec *r)
if (r->ap_auth_type) {
apr_table_addn(e, "AUTH_TYPE", r->ap_auth_type);
}
- rem_logname = ap_get_remote_logname(r);
- if (rem_logname) {
- apr_table_addn(e, "REMOTE_IDENT", apr_pstrdup(r->pool, rem_logname));
+ env_temp = ap_get_remote_logname(r);
+ if (env_temp) {
+ apr_table_addn(e, "REMOTE_IDENT", apr_pstrdup(r->pool, env_temp));
}
/* Apache custom error responses. If we have redirected set two new vars */
