From: Jim Jagielski Date: Tue, 20 Nov 2007 15:11:39 +0000 (+0000) Subject: mod_proxy canon-tion improvements/fixes. X-Git-Tag: 2.2.7~209 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=296439bfa99e3b6435298284174939c853b1a982;p=thirdparty%2Fapache%2Fhttpd.git mod_proxy canon-tion improvements/fixes. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x@596712 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index 69dc5db6c40..6860af1b1a0 100644 --- a/CHANGES +++ b/CHANGES @@ -1,8 +1,13 @@ -*- coding: utf-8 -*- Changes with Apache 2.2.7 + *) mod_proxy: Canonicalisation improvements. Add "nocanon" keyword to + ProxyPass, to suppress URI-canonicalisation in a reverse proxy. Also, + don't escape/unescape forward-proxied URLs. + PR 41798, 42592 [Nick Kew, Ruediger Pluem, Roy Fielding, Jim Jagielski] + *) mod_ssl: Prevent memory corruption of version string. - PR 43865 43334 [William Rowe, Joe Orton] + PR 43865, 43334 [William Rowe, Joe Orton] *) core: Avoid some unexpected connection closes by telling the client that the connection is not persistent if the MPM process handling diff --git a/STATUS b/STATUS index 8ff08053969..8475adab66b 100644 --- a/STATUS +++ b/STATUS @@ -79,36 +79,6 @@ RELEASE SHOWSTOPPERS: PATCHES ACCEPTED TO BACKPORT FROM TRUNK: [ start all new proposals below, under PATCHES PROPOSED. ] - * mod_proxy: add "nocanon" option to ProxyPass, to suppress - canonicalisation of reverse-proxied URLs. - PR 41798 PR 42592 - trunk: - http://svn.apache.org/viewvc?view=rev&revision=588791 - 2.2.x: - mod_proxy.c patch at http://people.apache.org/~niq/41798.patch - mod_proxy_http.c patch: trunk works after applying PR#42592 patch - Other files: trunk patch works. - 2.2.x composite patch: this together with PR42592 patch - http://people.apache.org/~niq/41798+42592.patch - +1: niq - -1: jim: patch should be self-contained. If a mix and match of - files, then please create a distinct 2.2 patch. - niq: we NEVER do that. When did you incorporate the CHANGES - update in a separate 2.2 patch? The complexity here is that - mod_proxy_http.c relies on 42592, and a separate fix to - that would invalidate the (very simple) patch to 42592. - jim: The issue is that the above "use trunk but use these external - patch" makes the backport hard to apply and test. Instead - of being able to just merge in r588791, you must just use - those that apply "cleanly" and handle mod_proxy.c as a - special case. This seems ripe for a specific 2.2 patch. Yes, - CHANGES always conflicts, but it is a documentation question, - not a code one. - rpluem says: Please add a minor bump for the changes to mod_proxy.h. - Then I am +1. - niq says: done - jim: +1 (needs doccos as well, 'natch) - PATCHES PROPOSED TO BACKPORT FROM TRUNK: [ New proposals should be added at the end of the list ] diff --git a/docs/manual/mod/mod_proxy.xml b/docs/manual/mod/mod_proxy.xml index 215912b24d5..cacc994ac79 100644 --- a/docs/manual/mod/mod_proxy.xml +++ b/docs/manual/mod/mod_proxy.xml @@ -548,7 +548,7 @@ expressions ProxyPass Maps remote servers into the local server URL-space -ProxyPass [path] !|url [key=value key=value ...]] +ProxyPass [path] !|url [key=value key=value ...]] [nocanon] server configvirtual host directory @@ -814,7 +814,14 @@ expressions </Proxy> - +

Normally, mod_proxy will canonicalise ProxyPassed URLs. + But this may be incompatible with some backends, particularly those + that make use of PATH_INFO. The optional nocanon + keyword suppresses this, and passes the URL path "raw" to the + backend. Note that may affect the security of your backend, as it + removes the normal limited protection against URL-based attacks + provided by the proxy.

+

When used inside a Location section, the first argument is omitted and the local directory is obtained from the status &= ~PROXY_WORKER_HOT_STANDBY; } - else if (*v == 'I' || *v == 'i') { - if (mode) - worker->status |= PROXY_WORKER_IGNORE_ERRORS; - else - worker->status &= ~PROXY_WORKER_IGNORE_ERRORS; - } + else if (*v == 'I' || *v == 'i') { + if (mode) + worker->status |= PROXY_WORKER_IGNORE_ERRORS; + else + worker->status &= ~PROXY_WORKER_IGNORE_ERRORS; + } else { return "Unknown status parameter option"; } @@ -440,7 +440,9 @@ static int proxy_trans(request_rec *r) int i, len; struct proxy_alias *ent = (struct proxy_alias *) conf->aliases->elts; ap_regmatch_t regm[AP_MAX_REG_MATCH]; + ap_regmatch_t reg1[AP_MAX_REG_MATCH]; char *found = NULL; + int mismatch = 0; if (r->proxyreq) { /* someone has already set up the proxy, it was possibly ourselves @@ -455,13 +457,22 @@ static int proxy_trans(request_rec *r) */ for (i = 0; i < conf->aliases->nelts; i++) { + unsigned int nocanon = ent[i].flags & PROXYPASS_NOCANON; + const char *use_uri = nocanon ? r->unparsed_uri : r->uri; if (ent[i].regex) { if (!ap_regexec(ent[i].regex, r->uri, AP_MAX_REG_MATCH, regm, 0)) { if ((ent[i].real[0] == '!') && (ent[i].real[1] == '\0')) { return DECLINED; } - found = ap_pregsub(r->pool, ent[i].real, r->uri, AP_MAX_REG_MATCH, - regm); + /* test that we haven't reduced the URI */ + if (nocanon && ap_regexec(ent[i].regex, r->unparsed_uri, + AP_MAX_REG_MATCH, reg1, 0)) { + mismatch = 1; + use_uri = r->uri; + } + found = ap_pregsub(r->pool, ent[i].real, use_uri, + AP_MAX_REG_MATCH, + (use_uri == r->uri) ? regm : reg1); /* Note: The strcmp() below catches cases where there * was no regex substitution. This is so cases like: * @@ -479,8 +490,8 @@ static int proxy_trans(request_rec *r) found = apr_pstrcat(r->pool, "proxy:", found, NULL); } else { - found = apr_pstrcat(r->pool, "proxy:", ent[i].real, r->uri, - NULL); + found = apr_pstrcat(r->pool, "proxy:", ent[i].real, + use_uri, NULL); } } } @@ -491,15 +502,31 @@ static int proxy_trans(request_rec *r) if ((ent[i].real[0] == '!') && (ent[i].real[1] == '\0')) { return DECLINED; } - + if (nocanon + && len != alias_match(r->unparsed_uri, ent[i].fake)) { + mismatch = 1; + use_uri = r->uri; + } found = apr_pstrcat(r->pool, "proxy:", ent[i].real, - r->uri + len, NULL); + use_uri + len, NULL); } } + if (mismatch) { + /* We made a reducing transformation, so we can't safely use + * unparsed_uri. Safe fallback is to ignore nocanon. + */ + ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, + "Unescaped URL path matched ProxyPass; ignoring unsafe nocanon"); + } + if (found) { r->filename = found; r->handler = "proxy-server"; r->proxyreq = PROXYREQ_REVERSE; + if (nocanon && !mismatch) { + /* mod_proxy_http needs to be told. Different module. */ + apr_table_setn(r->notes, "proxy-nocanon", "1"); + } return OK; } } @@ -1090,6 +1117,7 @@ static const char * const apr_table_entry_t *elts; int i; int use_regex = is_regex; + unsigned int flags = 0; while (*arg) { word = ap_getword_conf(cmd->pool, &arg); @@ -1103,8 +1131,12 @@ static const char * } f = word; } - else if (!r) + else if (!r) { r = word; + } + else if (!strcasecmp(word,"nocanon")) { + flags |= PROXYPASS_NOCANON; + } else { char *val = strchr(word, '='); if (!val) { @@ -1135,6 +1167,7 @@ static const char * new = apr_array_push(conf->aliases); new->fake = apr_pstrdup(cmd->pool, f); new->real = apr_pstrdup(cmd->pool, r); + new->flags = flags; if (use_regex) { new->regex = ap_pregcomp(cmd->pool, f, AP_REG_EXTENDED); if (new->regex == NULL) diff --git a/modules/proxy/mod_proxy.h b/modules/proxy/mod_proxy.h index 0f25caaff98..c0a29c9fcfb 100644 --- a/modules/proxy/mod_proxy.h +++ b/modules/proxy/mod_proxy.h @@ -109,10 +109,12 @@ struct proxy_remote { int use_regex; /* simple boolean. True if we have a regex pattern */ }; +#define PROXYPASS_NOCANON 0x01 struct proxy_alias { const char *real; const char *fake; ap_regex_t *regex; + unsigned int flags; }; struct dirconn_entry { diff --git a/modules/proxy/mod_proxy_http.c b/modules/proxy/mod_proxy_http.c index 7e6493b41cb..774852eed40 100644 --- a/modules/proxy/mod_proxy_http.c +++ b/modules/proxy/mod_proxy_http.c @@ -81,7 +81,26 @@ static int proxy_http_canon(request_rec *r, char *url) search = r->args; /* process path */ - path = ap_proxy_canonenc(r->pool, url, strlen(url), enc_path, 0, r->proxyreq); + /* In a reverse proxy, our URL has been processed, so canonicalise + * unless proxy-nocanon is set to say it's raw + * In a forward proxy, we have and MUST NOT MANGLE the original. + */ + switch (r->proxyreq) { + default: /* wtf are we doing here? */ + case PROXYREQ_REVERSE: + if (apr_table_get(r->notes, "proxy-nocanon")) { + path = url; /* this is the raw path */ + } + else { + path = ap_proxy_canonenc(r->pool, url, strlen(url), + enc_path, 0, r->proxyreq); + } + break; + case PROXYREQ_PROXY: + path = url; + break; + } + if (path == NULL) return HTTP_BAD_REQUEST;