Re: [patch 2.0] http body request/response/trace conformance
-1 This is a violation of HTTP:
+if (apr_table_get(r-subprocess_env, force-proxy-request-1.0)
+ || ((r-proto_num HTTP_VERSION(1,1))
+ !apr_table_get(r-subprocess_env,
force-proxy-request-1.1))) {
In all cases except when specifically configured otherwise,
the proxy must send HTTP/1.1 if it supports responses in HTTP/1.1.
Roy
Re: [patch 2.0] http body request/response/trace conformance
At 06:14 AM 7/15/2005, Roy T. Fielding wrote:
-1 This is a violation of HTTP:
+if (apr_table_get(r-subprocess_env, force-proxy-request-1.0)
+ || ((r-proto_num HTTP_VERSION(1,1))
+ !apr_table_get(r-subprocess_env,
force-proxy-request-1.1))) {
In all cases except when specifically configured otherwise,
the proxy must send HTTP/1.1 if it supports responses in HTTP/1.1.
Thanks for the clarification, Roy!
Bill
Re: [patch 2.0] http body request/response/trace conformance
At 03:03 PM 7/14/2005, William A. Rowe, Jr. wrote:
To simplify - Jeff Joe and I reviewed two of the patches, and they
are committed. Two patches are available for comment;
changelog;
*) Added TraceEnable [on|off|extended] per-server directive to alter
the behavior of the TRACE method. This addresses a flaw in proxy
conformance to RFC 2616 - previously the proxy server would accept
a TRACE request body although the RFC prohibited it. The default
remains 'TraceEnable on'. [William Rowe]
http://people.apache.org/~wrowe/httpd-2.0-trace.patch
and changelog;
*) SECURITY: CAN-2005-2088
proxy: Correctly handle the Transfer-Encoding and Content-Length
headers. Discard the request Content-Length whenever T-E: chunked
is used, always passing one of either C-L or T-E: chunked whenever
the request includes a request body. Resolves an entire class of
proxy HTTP Request Splitting/Spoofing attacks. [William Rowe]
The newest flavor based on my most recent commits from Roy and Jeff's
feedback is available at;
http://people.apache.org/~wrowe/httpd-2.0-proxy-request-2.patch
and 2.0 STATUS is updated accordingly. Votes/Comments please?
Although proxy-request.patch will evolve as this discussion
continues; Jeff caused me to look, again, at the code and
recognize another edge case already committed to trunk
(and also in the patch.) proxy-request.patch will ultimately
mirror what we agree to on trunk.
And FYI, revert r219061 (below) from 2.1 or 2.0 to see the
continued misbehavior of proxy without the proxy-request.patch.
Bill
--- httpd/httpd/branches/2.0.x/server/protocol.c (original)
+++ httpd/httpd/branches/2.0.x/server/protocol.c Thu Jul 14 09:51:55 2005
@@ -885,6 +885,15 @@
apr_brigade_destroy(tmp_bb);
return r;
}
+
+if (apr_table_get(r-headers_in, Transfer-Encoding)
+ apr_table_get(r-headers_in, Content-Length)) {
+/* 2616 section 4.4, point 3: if both Transfer-Encoding
+ * and Content-Length are received, the latter MUST be
+ * ignored; so unset it here to prevent any confusion
+ * later. */
+apr_table_unset(r-headers_in, Content-Length);
+}
}
else {
if (r-header_only) {
Re: [patch 2.0] http body request/response/trace conformance
+1
Re: [patch 2.0] http body request/response/trace conformance
To simplify - Jeff Joe and I reviewed two of the patches, and they
are committed. Two patches are available for comment;
http://people.apache.org/~wrowe/httpd-2.0-trace.patch
http://people.apache.org/~wrowe/httpd-2.0-proxy-request.patch
Although proxy-request.patch will evolve as this discussion
continues; Jeff caused me to look, again, at the code and
recognize another edge case already committed to trunk
(and also in the patch.) proxy-request.patch will ultimately
mirror what we agree to on trunk.
And FYI, revert r219061 (below) from 2.1 or 2.0 to see the
continued misbehavior of proxy without the proxy-request.patch.
Bill
--- httpd/httpd/branches/2.0.x/server/protocol.c (original)
+++ httpd/httpd/branches/2.0.x/server/protocol.c Thu Jul 14 09:51:55 2005
@@ -885,6 +885,15 @@
apr_brigade_destroy(tmp_bb);
return r;
}
+
+if (apr_table_get(r-headers_in, Transfer-Encoding)
+ apr_table_get(r-headers_in, Content-Length)) {
+/* 2616 section 4.4, point 3: if both Transfer-Encoding
+ * and Content-Length are received, the latter MUST be
+ * ignored; so unset it here to prevent any confusion
+ * later. */
+apr_table_unset(r-headers_in, Content-Length);
+}
}
else {
if (r-header_only) {
[patch 2.0] http body request/response/trace conformance
Attached is the rollup of everything that we have collected
to address c-l/t-e conformance in the httpd-2.1 trunk, which
should apply clean to httpd-2.0 branch. I'll commit in 24 hrs
by lazy consensus, but would rather see a few +1's first.
Jeff's patch (without the protocol.c patch) demonstrated to me
how broken 2.1 really was. I've refactored that code entirely.
After this refactoring (and still without the protocol.c patch)
the attached chunked.req works correctly against an array of
Apache versions. I totally agree with Jeff's plan, the only
way to address this is to backport the corrected 2.1 proxy
request handler.
The 'TraceEnable extended' option has really proved invaluable
for this testing; I would encourage anyone investigating these
issues to check it out.
Bill
p.s. I've found this mini-configlet very useful for my test array;
ProxyPass /13/ http://localhost:8013/
ProxyPassReverse /13/ http://localhost:8013/
ProxyPass /20/ http://localhost:8020/
ProxyPassReverse /20/ http://localhost:8020/
ProxyPass /21/ http://localhost:8021/
ProxyPassReverse /21/ http://localhost:8021/
ProxyPass /99/ http://localhost:8099/
ProxyPassReverse /99/ http://localhost:8099/
TraceEnable extendedIndex: server/core.c
===
--- server/core.c (revision 210154)
+++ server/core.c (working copy)
@@ -458,6 +458,8 @@
conf-redirect_limit = 0; /* 0 == unset */
conf-subreq_limit = 0;
+conf-trace_enable = AP_TRACE_UNSET;
+
return (void *)conf;
}
@@ -489,6 +491,10 @@
? virt-subreq_limit
: base-subreq_limit;
+conf-trace_enable = (virt-trace_enable != AP_TRACE_UNSET)
+ ? virt-trace_enable
+ : base-trace_enable;
+
return conf;
}
@@ -1587,7 +1593,7 @@
methnum = ap_method_number_of(method);
if (methnum == M_TRACE !tog) {
-return TRACE cannot be controlled by Limit;
+return TRACE cannot be controlled by Limit, see TraceEnable;
}
else if (methnum == M_INVALID) {
/* method has not been registered yet, but resorce restriction
@@ -3113,6 +3119,28 @@
return rv;
}
+static const char *set_trace_enable(cmd_parms *cmd, void *dummy,
+const char *arg1)
+{
+core_server_config *conf = ap_get_module_config(cmd-server-module_config,
+core_module);
+
+if (strcasecmp(arg1, on) == 0) {
+conf-trace_enable = AP_TRACE_ENABLE;
+}
+else if (strcasecmp(arg1, off) == 0) {
+conf-trace_enable = AP_TRACE_DISABLE;
+}
+else if (strcasecmp(arg1, extended) == 0) {
+conf-trace_enable = AP_TRACE_EXTENDED;
+}
+else {
+return TraceEnable must be one of 'on', 'off', or 'extended';
+}
+
+return NULL;
+}
+
/* Note --- ErrorDocument will now work from .htaccess files.
* The AllowOverride of Fileinfo allows webmasters to turn it off
*/
@@ -3338,6 +3366,8 @@
AP_INIT_TAKE1(EnableExceptionHook, ap_mpm_set_exception_hook, NULL,
RSRC_CONF,
Controls whether exception hook may be called after a crash),
#endif
+AP_INIT_TAKE1(TraceEnable, set_trace_enable, NULL, RSRC_CONF,
+ 'on' (default), 'off' or 'extended' to trace request body
content),
{ NULL }
};
Index: server/protocol.c
===
--- server/protocol.c (revision 210154)
+++ server/protocol.c (working copy)
@@ -885,6 +885,15 @@
apr_brigade_destroy(tmp_bb);
return r;
}
+
+if (apr_table_get(r-headers_in, Transfer-Encoding)
+ apr_table_get(r-headers_in, Content-Length)) {
+/* 2616 section 4.4, point 3: if both Transfer-Encoding
+ * and Content-Length are received, the latter MUST be
+ * ignored; so unset it here to prevent any confusion
+ * later. */
+apr_table_unset(r-headers_in, Content-Length);
+}
}
else {
if (r-header_only) {
Index: modules/http/http_protocol.c
===
--- modules/http/http_protocol.c(revision 210154)
+++ modules/http/http_protocol.c(working copy)
@@ -1321,6 +1321,9 @@
apr_int64_t mask;
apr_array_header_t *allow = apr_array_make(r-pool, 10, sizeof(char *));
apr_hash_index_t *hi = apr_hash_first(r-pool, methods_registry);
+/* For TRACE below */
+core_server_config *conf =
+ap_get_module_config(r-server-module_config, core_module);
mask = r-allowed_methods-method_mask;
@@ -1338,8 +1341,9 @@
}
}
-/* TRACE is always allowed */
-*(const char **)apr_array_push(allow) = TRACE;
+/* TRACE is tested on a per-server basis */
+if (conf-trace_enable != AP_TRACE_DISABLE)
+
