Re: 2.1.5 available for testing
One more thought on this thread; wouldn't it be nice to
communicate to mod_cache not to trust this flaky response
or request TE+CL combination? Unsetting the keep alive on
both the proxy and client adds some degree of saftey, but
marking this non-cachable would eliminate any likely hood
of cache poisoning.
I don't have the code, perhaps someone can point out an easy
answer for 2.0/2.1 or offer a patch to mod_cache to make that
toggleable. In 1.3, this patch is trivial.
Just a thought,
Bill
At 05:45 AM 6/23/2005, Jeff Trawick wrote:
On 6/23/05, jean-frederic clere [EMAIL PROTECTED] wrote:
William A. Rowe, Jr. wrote:
++1 To Joe's comments.
Jeff's fix is technically right, but scares the nibbles out
of me. If, for example, an exploit is able to inject the
T-E on top of the legit C-L, I really suspect we should not
trust the origin server at all.
One important situation to fear is a buggy server or proxy+server that
we may not be able to talk to at all if we are extremely strict.
[As you implied w.r.t. Apache 1.3] The smuggling fear is really if we
allow keepalive on this connection to the origin server again. We
could be confused by making the wrong choice from {CL, TE} and
misunderstand the next response. We can set backend-close and
origin-keepalive to prevent that.
If we don't allow keepalive, then it is down to whether or not this
single request can be parsed correctly if our choice of {CL, TE} makes
sense.
For origin servers (as opposed to clients) make this choice
between ignore C-L, or fail, configurable?
try very hard to make a reasonable choice with no configuration :)
(speaking to the choir, of course)
My observation is that there are far more varied clients in
the world than servers, each with unique faults. But the
RFC 2616 is clear...
Messages MUST NOT include both a Content-Length header field and a
non-identity transfer-coding. If the message does include a non-
identity transfer-coding, the Content-Length MUST be ignored.
When a Content-Length is given in a message where a message-body is
allowed, its field value MUST exactly match the number of OCTETs in
the message-body. HTTP/1.1 user agents MUST notify the user when an
invalid length is received and detected.
...and server authors can be expected to be less buggy than clients.
Permissive in what we accept, strict in what we send implies some
strictness in what we trust to pass on to the client.
We're removing the protocol breakage in what we pass on to the client.
At this point, we either send a valid response or it is if the server
dropped the connection before sending the full response.
(I hear what you're screaming. I think the minimal-intervention path
should be preferred if we can justify it.)
So +.5 to Jeff's patch, and let's discuss if the proxy response should
then be trusted at all with T-E and C-L, in httpd-2.x where we support
keepalives.
Once the patch applied we lose the information that the request was
incorrect.
That means we won't be able to choose in proxy between sending C-L (and
dechunk)
and T-E.
I don't follow here. How does the backend choice of {TE, CL} affect
what we send the client?
Re: 2.1.5 available for testing
William A. Rowe, Jr. wrote:
++1 To Joe's comments.
Jeff's fix is technically right, but scares the nibbles out
of me. If, for example, an exploit is able to inject the
T-E on top of the legit C-L, I really suspect we should not
trust the origin server at all.
For origin servers (as opposed to clients) make this choice
between ignore C-L, or fail, configurable?
My observation is that there are far more varied clients in
the world than servers, each with unique faults. But the
RFC 2616 is clear...
Messages MUST NOT include both a Content-Length header field and a
non-identity transfer-coding. If the message does include a non-
identity transfer-coding, the Content-Length MUST be ignored.
When a Content-Length is given in a message where a message-body is
allowed, its field value MUST exactly match the number of OCTETs in
the message-body. HTTP/1.1 user agents MUST notify the user when an
invalid length is received and detected.
...and server authors can be expected to be less buggy than clients.
Permissive in what we accept, strict in what we send implies some
strictness in what we trust to pass on to the client.
So +.5 to Jeff's patch, and let's discuss if the proxy response should
then be trusted at all with T-E and C-L, in httpd-2.x where we support
keepalives.
Once the patch applied we lose the information that the request was incorrect.
That means we won't be able to choose in proxy between sending C-L (and dechunk)
and T-E.
[FYI +1 in httpd-1.3, that proxy does not use keepalives.]
Bill
Bill
At 06:40 PM 6/22/2005, Jeff Trawick wrote:
On 6/22/05, Paul Querna [EMAIL PROTECTED] wrote:
Joe Orton wrote:
On Wed, Jun 22, 2005 at 03:02:50PM -0500, William Rowe wrote:
Prior to either patch we totally mishandled such requests. So the
only question which remains is; which behavior do we prefer?
As the RFC states this is not acceptable, my gut says reject ANY
request with both C-L and T-E of non-identity.
I don't see any reason to reject anything, 2616 dictates precisely how
to handle requests which are malformed in this way. I do think the
check against identity is actually redundant, though; not least
because the 2616 errata remove all references to the word.
I think correct behaviour is to just follow the letter of Section 4.4,
point 3, as below:
If a message is received with both a
Transfer-Encoding header field and a Content-Length header field,
the latter MUST be ignored.
(and it's also going to be better to check for T-E before C-L since
99.99% of requests received are not going to have a T-E header so it'll
fall through slightly quicker)
+1 to the posted patch.
+1 here as well
What about proxy reading from origin server?
Origin server sends this to Apache acting as proxy:
HTTP/1.1 200 OK\r\n
Content-Length: 99\r\n
Transfer-Encoding: chunked\r\n
\r\n
0A\r\n
aa\r\n
00\r\n
\r\n
Client receives this:
HTTP/1.1 200 OK
Date: Wed, 22 Jun 2005 23:12:31 GMT
Content-Length: 99
Content-Type: text/plain; charset=ISO-8859-1
aa(END)
Add this patch:
Index: modules/proxy/mod_proxy_http.c
===
--- modules/proxy/mod_proxy_http.c (revision 191655)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1128,7 +1128,17 @@
r-headers_out,
save_table);
}
-
+
+/* can't have both Content-Length and Transfer-Encoding */
+if (apr_table_get(r-headers_out, Transfer-Encoding)
+ apr_table_get(r-headers_out, 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_out, Content-Length);
+}
+
/* strip connection listed hop-by-hop headers from response */
backend-close +=
ap_proxy_liststr(apr_table_get(r-headers_out,
Connection),
Client now gets this:
HTTP/1.1 200 OK
Date: Wed, 22 Jun 2005 23:22:19 GMT
Transfer-Encoding: chunked
Content-Type: text/plain; charset=ISO-8859-1
a
aa
0
Re: 2.1.5 available for testing
At 02:34 AM 6/23/2005, jean-frederic clere wrote: Once the patch applied we lose the information that the request was incorrect. That means we won't be able to choose in proxy between sending C-L (and dechunk) and T-E. s/request/response/ The point was, if one were to exploit the origin server to inject a fake T-E, and the C-L is legit, we can't catch it. Suggesting (to me) that it's better to insist on strictly conformant responses from origin servers, which are an entirely different beast than clients. But since there are (undoubtedly) bad origin servers out there, we would likely need to make this a configuration choice, not an absolute rule. After all, they are using Apache to front end their buggy/vulnerable backend servers out there :) Bill
Re: 2.1.5 available for testing
On 6/23/05, jean-frederic clere [EMAIL PROTECTED] wrote:
William A. Rowe, Jr. wrote:
++1 To Joe's comments.
Jeff's fix is technically right, but scares the nibbles out
of me. If, for example, an exploit is able to inject the
T-E on top of the legit C-L, I really suspect we should not
trust the origin server at all.
One important situation to fear is a buggy server or proxy+server that
we may not be able to talk to at all if we are extremely strict.
[As you implied w.r.t. Apache 1.3] The smuggling fear is really if we
allow keepalive on this connection to the origin server again. We
could be confused by making the wrong choice from {CL, TE} and
misunderstand the next response. We can set backend-close and
origin-keepalive to prevent that.
If we don't allow keepalive, then it is down to whether or not this
single request can be parsed correctly if our choice of {CL, TE} makes
sense.
For origin servers (as opposed to clients) make this choice
between ignore C-L, or fail, configurable?
try very hard to make a reasonable choice with no configuration :)
(speaking to the choir, of course)
My observation is that there are far more varied clients in
the world than servers, each with unique faults. But the
RFC 2616 is clear...
Messages MUST NOT include both a Content-Length header field and a
non-identity transfer-coding. If the message does include a non-
identity transfer-coding, the Content-Length MUST be ignored.
When a Content-Length is given in a message where a message-body is
allowed, its field value MUST exactly match the number of OCTETs in
the message-body. HTTP/1.1 user agents MUST notify the user when an
invalid length is received and detected.
...and server authors can be expected to be less buggy than clients.
Permissive in what we accept, strict in what we send implies some
strictness in what we trust to pass on to the client.
We're removing the protocol breakage in what we pass on to the client.
At this point, we either send a valid response or it is if the server
dropped the connection before sending the full response.
(I hear what you're screaming. I think the minimal-intervention path
should be preferred if we can justify it.)
So +.5 to Jeff's patch, and let's discuss if the proxy response should
then be trusted at all with T-E and C-L, in httpd-2.x where we support
keepalives.
Once the patch applied we lose the information that the request was
incorrect.
That means we won't be able to choose in proxy between sending C-L (and
dechunk)
and T-E.
I don't follow here. How does the backend choice of {TE, CL} affect
what we send the client?
Re: 2.1.5 available for testing
Jeff Trawick wrote:
On 6/23/05, jean-frederic clere [EMAIL PROTECTED] wrote:
William A. Rowe, Jr. wrote:
++1 To Joe's comments.
Jeff's fix is technically right, but scares the nibbles out
of me. If, for example, an exploit is able to inject the
T-E on top of the legit C-L, I really suspect we should not
trust the origin server at all.
One important situation to fear is a buggy server or proxy+server that
we may not be able to talk to at all if we are extremely strict.
[As you implied w.r.t. Apache 1.3] The smuggling fear is really if we
allow keepalive on this connection to the origin server again. We
could be confused by making the wrong choice from {CL, TE} and
misunderstand the next response. We can set backend-close and
origin-keepalive to prevent that.
If we don't allow keepalive, then it is down to whether or not this
single request can be parsed correctly if our choice of {CL, TE} makes
sense.
For origin servers (as opposed to clients) make this choice
between ignore C-L, or fail, configurable?
try very hard to make a reasonable choice with no configuration :)
(speaking to the choir, of course)
My observation is that there are far more varied clients in
the world than servers, each with unique faults. But the
RFC 2616 is clear...
Messages MUST NOT include both a Content-Length header field and a
non-identity transfer-coding. If the message does include a non-
identity transfer-coding, the Content-Length MUST be ignored.
When a Content-Length is given in a message where a message-body is
allowed, its field value MUST exactly match the number of OCTETs in
the message-body. HTTP/1.1 user agents MUST notify the user when an
invalid length is received and detected.
...and server authors can be expected to be less buggy than clients.
Permissive in what we accept, strict in what we send implies some
strictness in what we trust to pass on to the client.
We're removing the protocol breakage in what we pass on to the client.
At this point, we either send a valid response or it is if the server
dropped the connection before sending the full response.
(I hear what you're screaming. I think the minimal-intervention path
should be preferred if we can justify it.)
So +.5 to Jeff's patch, and let's discuss if the proxy response should
then be trusted at all with T-E and C-L, in httpd-2.x where we support
keepalives.
Once the patch applied we lose the information that the request was incorrect.
That means we won't be able to choose in proxy between sending C-L (and dechunk)
and T-E.
I don't follow here. How does the backend choice of {TE, CL} affect
what we send the client?
If we are acting as a proxy, what we send to the next proxy is changed by the
patch, isn't it?
Re: 2.1.5 available for testing
On 6/23/05, jean-frederic clere [EMAIL PROTECTED] wrote:
Jeff Trawick wrote:
On 6/23/05, jean-frederic clere [EMAIL PROTECTED] wrote:
William A. Rowe, Jr. wrote:
++1 To Joe's comments.
Jeff's fix is technically right, but scares the nibbles out
of me. If, for example, an exploit is able to inject the
T-E on top of the legit C-L, I really suspect we should not
trust the origin server at all.
One important situation to fear is a buggy server or proxy+server that
we may not be able to talk to at all if we are extremely strict.
[As you implied w.r.t. Apache 1.3] The smuggling fear is really if we
allow keepalive on this connection to the origin server again. We
could be confused by making the wrong choice from {CL, TE} and
misunderstand the next response. We can set backend-close and
origin-keepalive to prevent that.
If we don't allow keepalive, then it is down to whether or not this
single request can be parsed correctly if our choice of {CL, TE} makes
sense.
For origin servers (as opposed to clients) make this choice
between ignore C-L, or fail, configurable?
try very hard to make a reasonable choice with no configuration :)
(speaking to the choir, of course)
My observation is that there are far more varied clients in
the world than servers, each with unique faults. But the
RFC 2616 is clear...
Messages MUST NOT include both a Content-Length header field and a
non-identity transfer-coding. If the message does include a non-
identity transfer-coding, the Content-Length MUST be ignored.
When a Content-Length is given in a message where a message-body is
allowed, its field value MUST exactly match the number of OCTETs in
the message-body. HTTP/1.1 user agents MUST notify the user when an
invalid length is received and detected.
...and server authors can be expected to be less buggy than clients.
Permissive in what we accept, strict in what we send implies some
strictness in what we trust to pass on to the client.
We're removing the protocol breakage in what we pass on to the client.
At this point, we either send a valid response or it is if the server
dropped the connection before sending the full response.
(I hear what you're screaming. I think the minimal-intervention path
should be preferred if we can justify it.)
So +.5 to Jeff's patch, and let's discuss if the proxy response should
then be trusted at all with T-E and C-L, in httpd-2.x where we support
keepalives.
Once the patch applied we lose the information that the request was
incorrect.
That means we won't be able to choose in proxy between sending C-L (and
dechunk)
and T-E.
I don't follow here. How does the backend choice of {TE, CL} affect
what we send the client?
If we are acting as a proxy, what we send to the next proxy is changed by the
patch, isn't it?
This second patch is a bit confusing out of context because of the use
by that function of r-headers_OUT for information we have READ from
the origin server. It doesn't affect what we send to the next proxy
as far as I can tell.
The original patch could affect what we send to the next proxy.
Re: 2.1.5 available for testing
At 05:45 AM 6/23/2005, Jeff Trawick wrote:
On 6/23/05, jean-frederic clere [EMAIL PROTECTED] wrote:
William A. Rowe, Jr. wrote:
++1 To Joe's comments.
Jeff's fix is technically right, but scares the nibbles out
of me. If, for example, an exploit is able to inject the
T-E on top of the legit C-L, I really suspect we should not
trust the origin server at all.
If we don't allow keepalive, then it is down to whether or not this
single request can be parsed correctly if our choice of {CL, TE} makes
sense.
So close the proxy connection if C-L and T-E are returned from the
origin server? That would upgrade my +.5 to +1 - I totally agree.
For origin servers (as opposed to clients) make this choice
between ignore C-L, or fail, configurable?
try very hard to make a reasonable choice with no configuration :)
(speaking to the choir, of course)
Yup - that was my concern too.
Once the patch applied we lose the information that the request was
incorrect.
That means we won't be able to choose in proxy between sending C-L (and
dechunk)
and T-E.
I don't follow here. How does the backend choice of {TE, CL} affect
what we send the client?
I really didn't understand that either. The RFC really doesn't offer
any choice of how to interpret T-E and C-L (it states, ignore (drop)
the C-L header. And we will pass the response to the client however
we agreed.
Bill
Re: 2.1.5 available for testing
On 6/23/05, William A. Rowe, Jr. [EMAIL PROTECTED] wrote:
At 05:45 AM 6/23/2005, Jeff Trawick wrote:
On 6/23/05, jean-frederic clere [EMAIL PROTECTED] wrote:
William A. Rowe, Jr. wrote:
++1 To Joe's comments.
Jeff's fix is technically right, but scares the nibbles out
of me. If, for example, an exploit is able to inject the
T-E on top of the legit C-L, I really suspect we should not
trust the origin server at all.
If we don't allow keepalive, then it is down to whether or not this
single request can be parsed correctly if our choice of {CL, TE} makes
sense.
So close the proxy connection if C-L and T-E are returned from the
origin server? That would upgrade my +.5 to +1 - I totally agree.
Cool... I'm working on a code change and a test for this...
Re: 2.1.5 available for testing
On 6/23/05, Jeff Trawick [EMAIL PROTECTED] wrote:
On 6/23/05, William A. Rowe, Jr. [EMAIL PROTECTED] wrote:
At 05:45 AM 6/23/2005, Jeff Trawick wrote:
On 6/23/05, jean-frederic clere [EMAIL PROTECTED] wrote:
William A. Rowe, Jr. wrote:
++1 To Joe's comments.
Jeff's fix is technically right, but scares the nibbles out
of me. If, for example, an exploit is able to inject the
T-E on top of the legit C-L, I really suspect we should not
trust the origin server at all.
If we don't allow keepalive, then it is down to whether or not this
single request can be parsed correctly if our choice of {CL, TE} makes
sense.
So close the proxy connection if C-L and T-E are returned from the
origin server? That would upgrade my +.5 to +1 - I totally agree.
Cool... I'm working on a code change and a test for this...
Even with plenty of caffeine I'm lost on how to get 2.1-dev proxy to
try to keep backend connection. Some sort of configuration is now
required? Backing down to 2.0.x for now.
Re: 2.1.5 available for testing
At 02:12 AM 6/20/2005, Paul Querna wrote: William A. Rowe, Jr. wrote: Also, possibly across platforms is a fault in ssl_engine_init, where the host-protocol is still NULL, and we are trying to strcmp it to 'https'. I spent part of my weekend trying to grok what change has broken this, but strcmp to NULL is popping a segfault. Not worthy of rejecting 2.1.5 on it's own, this is still a minor irritation. FYI - mod_ssl was loaded without SSL being defined, so no ssl host actually exists. In all cases the protocol should be set, and never be null. Look at ap_setup_listeners() in server/listen.c. All linux cases as usual :-? ap_setup_listeners is only called in the parent. On mpm_winnt, this happens in the open_logs hook of the parent. get_listeners_from_parent in the mpm_winnt creates those per-child listen records. Memory is not inherited between parent and child so the patch to modify this broke win32. This code seems a bit too convoluted for me to delve into right now, and I'm not quite clear what problem the patch was ment to solve in the first place (?) Bill
Re: 2.1.5 available for testing
William A. Rowe, Jr. wrote: At 02:12 AM 6/20/2005, Paul Querna wrote: William A. Rowe, Jr. wrote: Also, possibly across platforms is a fault in ssl_engine_init, where the host-protocol is still NULL, and we are trying to strcmp it to 'https'. I spent part of my weekend trying to grok what change has broken this, but strcmp to NULL is popping a segfault. Not worthy of rejecting 2.1.5 on it's own, this is still a minor irritation. FYI - mod_ssl was loaded without SSL being defined, so no ssl host actually exists. In all cases the protocol should be set, and never be null. Look at ap_setup_listeners() in server/listen.c. All linux cases as usual :-? Ugh. FreeBSD actually, and all the *nix MPMs do this correctly :-P ap_setup_listeners is only called in the parent. On mpm_winnt, this happens in the open_logs hook of the parent. I wish we had more 'rules' for MPMs. Right now its hit and miss. In this case, I missed mpm_winnt... get_listeners_from_parent in the mpm_winnt creates those per-child listen records. Memory is not inherited between parent and child so the patch to modify this broke win32. Okay, I will look at breaking this out to a separate function, and then calling it from the correct places in mpm_winnt. This code seems a bit too convoluted for me to delve into right now, and I'm not quite clear what problem the patch was ment to solve in the first place (?) The point is to inherit a Protocol in a Virtual Host from the Listening Socket, if one wasn't explicitly set. -Paul
Re: 2.1.5 available for testing
At 01:57 AM 6/20/2005, jean-frederic clere wrote: I still need some more time to check POST with 2 different content-lengths and GET with content-length. Well, GET with c-l is permitted. 2 C-L headers would be rejected due to the '##, ##' format, where the ', ' is non-numeric. After your patch, in 2.1 we now accept (discard) the C-L headers (even multiples) if we also have T-E: chunked. Prior to your patch we ment to reject the request with C-L and T-E if Paul's patch was entirely correct. Prior to either patch we totally mishandled such requests. So the only question which remains is; which behavior do we prefer? As the RFC states this is not acceptable, my gut says reject ANY request with both C-L and T-E of non-identity. Quoting 4.4 of RFC 2616; For compatibility with HTTP/1.0 applications, HTTP/1.1 requests containing a message-body MUST include a valid Content-Length header field unless the server is known to be HTTP/1.1 compliant. If a request contains a message-body and a Content-Length is not given, the server SHOULD respond with 400 (bad request) if it cannot determine the length of the message, or with 411 (length required) if it wishes to insist on receiving a valid Content-Length. All HTTP/1.1 applications that receive entities MUST accept the chunked transfer-coding (section 3.6), thus allowing this mechanism to be used for messages when the message length cannot be determined in advance. Messages MUST NOT include both a Content-Length header field and a non-identity transfer-coding. If the message does include a non- identity transfer-coding, the Content-Length MUST be ignored. So I can see where we would ignore C-L, I'm mostly concerned that we never proxy or pass on a C-L passed by a client through our proxy to another UA for non-identity encodings. But rejecting outright seems safer :) Bill
Re: 2.1.5 available for testing
Joe Orton wrote: On Wed, Jun 22, 2005 at 03:02:50PM -0500, William Rowe wrote: Prior to either patch we totally mishandled such requests. So the only question which remains is; which behavior do we prefer? As the RFC states this is not acceptable, my gut says reject ANY request with both C-L and T-E of non-identity. I don't see any reason to reject anything, 2616 dictates precisely how to handle requests which are malformed in this way. I do think the check against identity is actually redundant, though; not least because the 2616 errata remove all references to the word. I think correct behaviour is to just follow the letter of Section 4.4, point 3, as below: If a message is received with both a Transfer-Encoding header field and a Content-Length header field, the latter MUST be ignored. (and it's also going to be better to check for T-E before C-L since 99.99% of requests received are not going to have a T-E header so it'll fall through slightly quicker) +1 to the posted patch. -Paul
Re: 2.1.5 available for testing
On 6/22/05, Paul Querna [EMAIL PROTECTED] wrote:
Joe Orton wrote:
On Wed, Jun 22, 2005 at 03:02:50PM -0500, William Rowe wrote:
Prior to either patch we totally mishandled such requests. So the
only question which remains is; which behavior do we prefer?
As the RFC states this is not acceptable, my gut says reject ANY
request with both C-L and T-E of non-identity.
I don't see any reason to reject anything, 2616 dictates precisely how
to handle requests which are malformed in this way. I do think the
check against identity is actually redundant, though; not least
because the 2616 errata remove all references to the word.
I think correct behaviour is to just follow the letter of Section 4.4,
point 3, as below:
If a message is received with both a
Transfer-Encoding header field and a Content-Length header field,
the latter MUST be ignored.
(and it's also going to be better to check for T-E before C-L since
99.99% of requests received are not going to have a T-E header so it'll
fall through slightly quicker)
+1 to the posted patch.
+1 here as well
What about proxy reading from origin server?
Origin server sends this to Apache acting as proxy:
HTTP/1.1 200 OK\r\n
Content-Length: 99\r\n
Transfer-Encoding: chunked\r\n
\r\n
0A\r\n
aa\r\n
00\r\n
\r\n
Client receives this:
HTTP/1.1 200 OK
Date: Wed, 22 Jun 2005 23:12:31 GMT
Content-Length: 99
Content-Type: text/plain; charset=ISO-8859-1
aa(END)
Add this patch:
Index: modules/proxy/mod_proxy_http.c
===
--- modules/proxy/mod_proxy_http.c (revision 191655)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1128,7 +1128,17 @@
r-headers_out,
save_table);
}
-
+
+/* can't have both Content-Length and Transfer-Encoding */
+if (apr_table_get(r-headers_out, Transfer-Encoding)
+ apr_table_get(r-headers_out, 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_out, Content-Length);
+}
+
/* strip connection listed hop-by-hop headers from response */
backend-close +=
ap_proxy_liststr(apr_table_get(r-headers_out,
Connection),
Client now gets this:
HTTP/1.1 200 OK
Date: Wed, 22 Jun 2005 23:22:19 GMT
Transfer-Encoding: chunked
Content-Type: text/plain; charset=ISO-8859-1
a
aa
0
Re: 2.1.5 available for testing
++1 To Joe's comments.
Jeff's fix is technically right, but scares the nibbles out
of me. If, for example, an exploit is able to inject the
T-E on top of the legit C-L, I really suspect we should not
trust the origin server at all.
For origin servers (as opposed to clients) make this choice
between ignore C-L, or fail, configurable?
My observation is that there are far more varied clients in
the world than servers, each with unique faults. But the
RFC 2616 is clear...
Messages MUST NOT include both a Content-Length header field and a
non-identity transfer-coding. If the message does include a non-
identity transfer-coding, the Content-Length MUST be ignored.
When a Content-Length is given in a message where a message-body is
allowed, its field value MUST exactly match the number of OCTETs in
the message-body. HTTP/1.1 user agents MUST notify the user when an
invalid length is received and detected.
...and server authors can be expected to be less buggy than clients.
Permissive in what we accept, strict in what we send implies some
strictness in what we trust to pass on to the client.
So +.5 to Jeff's patch, and let's discuss if the proxy response should
then be trusted at all with T-E and C-L, in httpd-2.x where we support
keepalives.
[FYI +1 in httpd-1.3, that proxy does not use keepalives.]
Bill
Bill
At 06:40 PM 6/22/2005, Jeff Trawick wrote:
On 6/22/05, Paul Querna [EMAIL PROTECTED] wrote:
Joe Orton wrote:
On Wed, Jun 22, 2005 at 03:02:50PM -0500, William Rowe wrote:
Prior to either patch we totally mishandled such requests. So the
only question which remains is; which behavior do we prefer?
As the RFC states this is not acceptable, my gut says reject ANY
request with both C-L and T-E of non-identity.
I don't see any reason to reject anything, 2616 dictates precisely how
to handle requests which are malformed in this way. I do think the
check against identity is actually redundant, though; not least
because the 2616 errata remove all references to the word.
I think correct behaviour is to just follow the letter of Section 4.4,
point 3, as below:
If a message is received with both a
Transfer-Encoding header field and a Content-Length header field,
the latter MUST be ignored.
(and it's also going to be better to check for T-E before C-L since
99.99% of requests received are not going to have a T-E header so it'll
fall through slightly quicker)
+1 to the posted patch.
+1 here as well
What about proxy reading from origin server?
Origin server sends this to Apache acting as proxy:
HTTP/1.1 200 OK\r\n
Content-Length: 99\r\n
Transfer-Encoding: chunked\r\n
\r\n
0A\r\n
aa\r\n
00\r\n
\r\n
Client receives this:
HTTP/1.1 200 OK
Date: Wed, 22 Jun 2005 23:12:31 GMT
Content-Length: 99
Content-Type: text/plain; charset=ISO-8859-1
aa(END)
Add this patch:
Index: modules/proxy/mod_proxy_http.c
===
--- modules/proxy/mod_proxy_http.c (revision 191655)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1128,7 +1128,17 @@
r-headers_out,
save_table);
}
-
+
+/* can't have both Content-Length and Transfer-Encoding */
+if (apr_table_get(r-headers_out, Transfer-Encoding)
+ apr_table_get(r-headers_out, 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_out, Content-Length);
+}
+
/* strip connection listed hop-by-hop headers from response */
backend-close +=
ap_proxy_liststr(apr_table_get(r-headers_out,
Connection),
Client now gets this:
HTTP/1.1 200 OK
Date: Wed, 22 Jun 2005 23:22:19 GMT
Transfer-Encoding: chunked
Content-Type: text/plain; charset=ISO-8859-1
a
aa
0
Re: 2.1.5 available for testing
William A. Rowe, Jr. wrote: At 03:07 PM 6/17/2005, William A. Rowe, Jr. wrote: -1 on Win32, caddr_t isn't sufficiently portable (fix committed). Correction, -1 on all platforms. jfclere just committed a significant patch to the T-E override of the C-L passed to us, as part of the Watchfire vulnerability fixes. It seems very irresponsible to release any flavor (alpha, beta, nadda) with a known vuln, when we've committed a fix already. I still need some more time to check POST with 2 different content-lengths and GET with content-length. Also, possibly across platforms is a fault in ssl_engine_init, where the host-protocol is still NULL, and we are trying to strcmp it to 'https'. I spent part of my weekend trying to grok what change has broken this, but strcmp to NULL is popping a segfault. Not worthy of rejecting 2.1.5 on it's own, this is still a minor irritation. FYI - mod_ssl was loaded without SSL being defined, so no ssl host actually exists. Bill
Re: 2.1.5 available for testing
William A. Rowe, Jr. wrote: Also, possibly across platforms is a fault in ssl_engine_init, where the host-protocol is still NULL, and we are trying to strcmp it to 'https'. I spent part of my weekend trying to grok what change has broken this, but strcmp to NULL is popping a segfault. Not worthy of rejecting 2.1.5 on it's own, this is still a minor irritation. FYI - mod_ssl was loaded without SSL being defined, so no ssl host actually exists. In all cases the protocol should be set, and never be null. Look at ap_setup_listeners() in server/listen.c. What configuration are you using that causes it to be NULL in cases? Thanks, -Paul
Re: 2.1.5 available for testing
jean-frederic clere wrote: William A. Rowe, Jr. wrote: At 03:07 PM 6/17/2005, William A. Rowe, Jr. wrote: -1 on Win32, caddr_t isn't sufficiently portable (fix committed). Correction, -1 on all platforms. jfclere just committed a significant patch to the T-E override of the C-L passed to us, as part of the Watchfire vulnerability fixes. It seems very irresponsible to release any flavor (alpha, beta, nadda) with a known vuln, when we've committed a fix already. I still need some more time to check POST with 2 different content-lengths HTTP_BAD_REQUEST for this one. and GET with content-length. I think that is not forbidden in the rfc... But what about returning HTTP_BAD_REQUEST if Content-Length is not 0? Also, possibly across platforms is a fault in ssl_engine_init, where the host-protocol is still NULL, and we are trying to strcmp it to 'https'. I spent part of my weekend trying to grok what change has broken this, but strcmp to NULL is popping a segfault. Not worthy of rejecting 2.1.5 on it's own, this is still a minor irritation. FYI - mod_ssl was loaded without SSL being defined, so no ssl host actually exists. Bill
Re: 2.1.5 available for testing
Not sure what is causing he protocol not to be set either, but I hit the same thing when testing mod_ssl on NetWare. Brad [EMAIL PROTECTED] Monday, June 20, 2005 1:12:23 AM William A. Rowe, Jr. wrote: Also, possibly across platforms is a fault in ssl_engine_init, where the host-protocol is still NULL, and we are trying to strcmp it to 'https'. I spent part of my weekend trying to grok what change has broken this, but strcmp to NULL is popping a segfault. Not worthy of rejecting 2.1.5 on it's own, this is still a minor irritation. FYI - mod_ssl was loaded without SSL being defined, so no ssl host actually exists. In all cases the protocol should be set, and never be null. Look at ap_setup_listeners() in server/listen.c. What configuration are you using that causes it to be NULL in cases? Thanks, -Paul
Re: 2.1.5 available for testing
On Fri, Jun 17, 2005 at 12:40:50AM -0700, Paul Querna wrote: Please test and vote on releasing 2.1.5 as -alpha. +1 for alpha, httpd-test'ed on a bunch of Linuxes here, looks good. Thanks Paul! Also +1 on basing the 2.1.x/2.2.x stabilisation branch on this per previous discussion. The C-L vs T-E thing is not very critical and the handling of that in this release is not really much worse than all previous 2.0.x releases. joe
Re: 2.1.5 available for testing
On 20-Jun-05, at 8:55 AM, jean-frederic clere wrote: and GET with content-length. I think that is not forbidden in the rfc... But what about returning HTTP_BAD_REQUEST if Content-Length is not 0? I don't believe rfc2616 forbids GET requests to have entity-bodies, so it could be argued that rejecting such a request a priori is non-conformant. Obviously, if the handler for the request does not expect an entity-body, which would be the case in all the handlers I've seen, the request ought to be rejected. On the other hand, from a pragmatic point of view, proxying GETs with entity-bodies to unknown downstream servers may cause them to misbehave. So there is a good argument for mod_proxy to reject such requests unless specifically instructed otherwise.
Re: 2.1.5 available for testing
William A. Rowe, Jr. wrote: At 08:55 AM 6/20/2005, jean-frederic clere wrote: jean-frederic clere wrote: and GET with content-length. I think that is not forbidden in the rfc... But what about returning HTTP_BAD_REQUEST if Content-Length is not 0? See section 4.3 of RFC 2616. Content-Length: 0 signals a message body of 0 bytes, not the absence of a message body. It shouldn't be treated differently from Content-Length: n. OK, HTTP_BAD_REQUEST if C-L is present in GET? Bill
Re: 2.1.5 available for testing
At 11:13 AM 6/20/2005, jean-frederic clere wrote: William A. Rowe, Jr. wrote: At 08:55 AM 6/20/2005, jean-frederic clere wrote: jean-frederic clere wrote: and GET with content-length. I think that is not forbidden in the rfc... But what about returning HTTP_BAD_REQUEST if Content-Length is not 0? See section 4.3 of RFC 2616. Content-Length: 0 signals a message body of 0 bytes, not the absence of a message body. It shouldn't be treated differently from Content-Length: n. OK, HTTP_BAD_REQUEST if C-L is present in GET? I don't think so; 9.3 does not exclude it. Any extended XML resource description could require it. The only method which prohibits message bodies is TRACE. I believe this is one case where we need to absolutely follow the letter of RFC2616, and expect the backend to do the same. I would not object to some additional flag in the Proxy block which restricts message bodies in various ways, as an optional feature as opposed to being 'stricter than the RFC'. The changes to T-E and C-L will undoubtedly break some home brew request submission code. At least when things break, let's have the RFC to fall back on, that we 'did the right thing' with respect to request bodies. Bill
Re: 2.1.5 available for testing
At 11:18 AM 6/20/2005, jean-frederic clere wrote: Correct, I have a new additional patch that prevent multiples C-L headers. It isn't needed for reasons I pointed out on the dev list. Content-Encoding: x followed by Content-Encoding: y becomes Content-Encoding: x, y - this is rejected later in the process due to the non-numeric text , . But the patch stops the processing of the bad request earlier. from RFC 2616 4.2 Message Headers Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one field-name: field-value pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded. E.g. Accept[-*]: Allow: Cache-Control: Connection: Content-Encoding: Content-Language: Expect: If[-None]-Match: Pragma: Proxy-Authenticate: Range: (?the bytes=1#() target so I'm uncertain) TE:(literal TE, meaning acceptable response T-E choices) Trailer: Transfer-Encoding: Upgrade: Vary: Via: Warning: WWW-Authenticate: Every other header is equally guilty if we send multiples. Obviously some extension headers not defined in RFC2616 also follow some #() grammer. Cookies come to mind. Bill
Re: 2.1.5 available for testing
On Jun 17, 2005, at 12:40 AM, Paul Querna wrote: Please test and vote on releasing 2.1.5 as -alpha. I know it's already been vetoed, but here's my run: Darwin MonaLisa 8.1.0 Darwin Kernel Version 8.1.0: Tue May 10 18:16:08 PDT 2005; root:xnu-792.1.5.obj~4/RELEASE_PPC Power Macintosh powerpc 2.1.4: Failed TestStat Wstat Total Fail Failed List of Failed --- t/apache/passbrigade.t 1147 6.14% 55-57 111-114 t/apache/rwrite.t 1147 6.14% 54-57 112-114 t/http11/chunked.t 303 10.00% 9 14 19 t/modules/include.t 861 1.16% 51 t/modules/info.t 11 100.00% 1 t/protocol/echo.t 255 65280 8 16 200.00% 1-8 t/ssl/proxy.t 1723 1.74% 59 61 115 3 tests and 1 subtest skipped. Failed 7/63 test scripts, 88.89% okay. 30/2790 subtests failed, 98.92% okay. 2.1.5: Failed TestStat Wstat Total Fail Failed List of Failed --- t/apache/passbrigade.t 1148 7.02% 54-57 111-114 t/apache/rwrite.t 1148 7.02% 54-57 111-114 t/http11/chunked.t 303 10.00% 9 14 19 t/modules/info.t 11 100.00% 1 t/protocol/echo.t 255 65280 8 16 200.00% 1-8 3 tests and 1 subtest skipped. Failed 5/63 test scripts, 92.06% okay. 28/2790 subtests failed, 99.00% okay. So, we're doing a little better with the new drop. I have done no analysis on the failures but may do so as time permits. S. -- [EMAIL PROTECTED] http://www.temme.net/sander/ PGP FP: 51B4 8727 466A 0BC3 69F4 B7B8 B2BE BC40 1529 24AF smime.p7s Description: S/MIME cryptographic signature
Re: 2.1.5 available for testing
At 03:07 PM 6/17/2005, William A. Rowe, Jr. wrote: -1 on Win32, caddr_t isn't sufficiently portable (fix committed). Correction, -1 on all platforms. jfclere just committed a significant patch to the T-E override of the C-L passed to us, as part of the Watchfire vulnerability fixes. It seems very irresponsible to release any flavor (alpha, beta, nadda) with a known vuln, when we've committed a fix already. Also, possibly across platforms is a fault in ssl_engine_init, where the host-protocol is still NULL, and we are trying to strcmp it to 'https'. I spent part of my weekend trying to grok what change has broken this, but strcmp to NULL is popping a segfault. Not worthy of rejecting 2.1.5 on it's own, this is still a minor irritation. FYI - mod_ssl was loaded without SSL being defined, so no ssl host actually exists. Bill
2.1.5 available for testing
Please test and vote on releasing 2.1.5 as -alpha. Available at: http://httpd.apache.org/dev/dist/ (might take up to 2 hours for the files to appear, due to the rsync delay) MD5 (httpd-2.1.5-alpha.tar.Z) = f9dea893723fe7ddb8e9c5a4225a276b MD5 (httpd-2.1.5-alpha.tar.bz2) = 8ec926f27c2768c7cbd24f42fa22108a MD5 (httpd-2.1.5-alpha.tar.gz) = 7fdc3d61deb72b3815e137bdae282f0e Thanks, Paul
Re: 2.1.5 available for testing
Paul Querna wrote: Please test and vote on releasing 2.1.5 as -alpha. +1 from me, Tested on FreeBSD 6.0-CURRENT and NetBSD 2.0. -Paul
Re: 2.1.5 available for testing
fredag 17 juni 2005 09.40 skrev Paul Querna: Please test and vote on releasing 2.1.5 as -alpha. Available at: http://httpd.apache.org/dev/dist/ (might take up to 2 hours for the files to appear, due to the rsync delay) MD5 (httpd-2.1.5-alpha.tar.Z) = f9dea893723fe7ddb8e9c5a4225a276b MD5 (httpd-2.1.5-alpha.tar.bz2) = 8ec926f27c2768c7cbd24f42fa22108a MD5 (httpd-2.1.5-alpha.tar.gz) = 7fdc3d61deb72b3815e137bdae282f0e Thanks, Paul +1 on Mandriva LE2005 -- Regards // Oden Eriksson Mandriva: http://www.mandriva.com NUX: http://nux.se
Re: 2.1.5 available for testing
+1 netware [EMAIL PROTECTED] Friday, June 17, 2005 1:40:50 AM Please test and vote on releasing 2.1.5 as -alpha. Available at: http://httpd.apache.org/dev/dist/ (might take up to 2 hours for the files to appear, due to the rsync delay) MD5 (httpd-2.1.5-alpha.tar.Z) = f9dea893723fe7ddb8e9c5a4225a276b MD5 (httpd-2.1.5-alpha.tar.bz2) = 8ec926f27c2768c7cbd24f42fa22108a MD5 (httpd-2.1.5-alpha.tar.gz) = 7fdc3d61deb72b3815e137bdae282f0e Thanks, Paul
Re: 2.1.5 available for testing
-1 on Win32, caddr_t isn't sufficiently portable (fix committed). Also, LDAP TLS is altogether broken, my gut says disable it, but this may not be an issue in the previous flavor of apr-util. I'm testing HEAD - which is why my apr-util result might vary. And stumbled across an old install of apache.exe which might have tripped me up (instead of using httpd.exe). So I'll have to unwind exactly what's going on with apr_ldap. I'm in the process of a broad range of 1.3 / 2.0 / 2.1 proxy header introspection, so I'll have a conclusive +/-1 on RFC2616 misbehavior shortly. In light of the Watchfire analysis, it's irresponsible of us to release anything until such issues are fully addressed, and I don't care if you call it beta, alpha, or nadda. At the very least, jfclere just fixed a major snafu in the T-E/C-L tests, which renders 2.1.5 borked, in my mind. Bill At 02:40 AM 6/17/2005, Paul Querna wrote: Please test and vote on releasing 2.1.5 as -alpha. Available at: http://httpd.apache.org/dev/dist/ (might take up to 2 hours for the files to appear, due to the rsync delay) MD5 (httpd-2.1.5-alpha.tar.Z) = f9dea893723fe7ddb8e9c5a4225a276b MD5 (httpd-2.1.5-alpha.tar.bz2) = 8ec926f27c2768c7cbd24f42fa22108a MD5 (httpd-2.1.5-alpha.tar.gz) = 7fdc3d61deb72b3815e137bdae282f0e Thanks, Paul
