Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
--On Wednesday, December 8, 2004 8:53 PM + Joe Orton <[EMAIL PROTECTED]> wrote: I can't convince myself it would solve the general case, though: if both r-> and c->output_filters to happen point to the *same* filter, modifying the filter chain without knowledge of r-> (which is the problem) will still break if the filter must be inserted in the ->prev position? We'd likely have to switch r->output_filters (and perhaps c->output_filters) to use double-indirection (**) - so that we can change the underlying pointer and update all 'pointers' at the same time. -- justin
Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
On Wed, Dec 08, 2004 at 08:22:49AM -0800, Justin Erenkrantz wrote: > --On Wednesday, December 8, 2004 9:33 AM + Joe Orton > <[EMAIL PROTECTED]> wrote: > > >On Tue, Dec 07, 2004 at 05:14:40PM -0700, Brad Nicholes wrote: > >> OK, now that you have enabled upgrades for anything other than > >>OPTIONS, I see the problem. Even though there is a content-length > >>included in the header, you are saying that the header is being sent > >>encrypted but the content is not, correct? And the reason for this is > >>because there is more than one filter stack that needs to be modified? > > > >Yes. I think this fixes it, it's a bit of a hack though: > > Isn't this the issue Stas keeps bringing up about the filters? See STATUS > - in particular "the edge connection filter cannot be removed" - I believe > it's listed as a showstopper currently. > > Should we invest time to just fix this the 'right' way? > > IIRC, the only real way to fix it is to go back to a doubly-linked list of > some sort. But, there might be some other clever ways of dealing with this. Ah very interesting, yes, it's essentially the same issue. It looks like using a doubly-linked list would solve this case as well. I can't convince myself it would solve the general case, though: if both r-> and c->output_filters to happen point to the *same* filter, modifying the filter chain without knowledge of r-> (which is the problem) will still break if the filter must be inserted in the ->prev position? joe
Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
--On Wednesday, December 8, 2004 9:33 AM + Joe Orton <[EMAIL PROTECTED]> wrote: On Tue, Dec 07, 2004 at 05:14:40PM -0700, Brad Nicholes wrote: OK, now that you have enabled upgrades for anything other than OPTIONS, I see the problem. Even though there is a content-length included in the header, you are saying that the header is being sent encrypted but the content is not, correct? And the reason for this is because there is more than one filter stack that needs to be modified? Yes. I think this fixes it, it's a bit of a hack though: Isn't this the issue Stas keeps bringing up about the filters? See STATUS - in particular "the edge connection filter cannot be removed" - I believe it's listed as a showstopper currently. Should we invest time to just fix this the 'right' way? IIRC, the only real way to fix it is to go back to a doubly-linked list of some sort. But, there might be some other clever ways of dealing with this. *shrug* -- justin
Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
It may be a bit of a hack, but it seems reasonable to me. The best
part is that it works.
+1
Brad
>>> [EMAIL PROTECTED] Wednesday, December 08, 2004 2:33:48 AM >>>
On Tue, Dec 07, 2004 at 05:14:40PM -0700, Brad Nicholes wrote:
>OK, now that you have enabled upgrades for anything other than
> OPTIONS, I see the problem. Even though there is a content-length
> included in the header, you are saying that the header is being sent
> encrypted but the content is not, correct? And the reason for this
is
> because there is more than one filter stack that needs to be
modified?
Yes. I think this fixes it, it's a bit of a hack though:
Index: modules/ssl/ssl_engine_io.c
===
--- modules/ssl/ssl_engine_io.c (revision 59)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -1184,22 +1184,26 @@
apr_bucket *b;
SSL *ssl;
-/* Just remove the filter, if it doesn't work the first time, it
won't
- * work at all for this request.
- */
-ap_remove_output_filter(f);
+/* f->ctx is non-NULL after the first call to this filter: it's
+ * necessary to pass through directly to the connection
output_filters
+ * for the remainder of this request, since the SSL output filter
has
+ * not been added to r->output_filters for this request. */
+if (f->ctx) {
+return ap_pass_brigade(f->c->output_filters, bb);
+}
-/* No need to ensure that this is a server with optional SSL, the
filter
- * is only inserted if that is true.
- */
-
+/* No need to ensure that this is a server with optional SSL, the
+ * filter is only inserted if that is true. */
upgrade = apr_table_get(r->headers_in, "Upgrade");
if (upgrade == NULL
|| strcmp(ap_getword(r->pool, &upgrade, ','), "TLS/1.0")) {
/* "Upgrade: TLS/1.0, ..." header not found, don't do Upgrade
*/
+ap_remove_output_filter(f);
return ap_pass_brigade(f->next, bb);
}
+f->ctx = f; /* flag as non-NULL for subsequent passes */
+
apr_table_unset(r->headers_out, "Upgrade");
/* Send the interim 101 response. */
@@ -1245,7 +1249,6 @@
pass the brigade off to the connection based output filters so
that the
request can complete encrypted */
return ap_pass_brigade(f->c->output_filters, bb);
-
}
static apr_status_t ssl_io_filter_input(ap_filter_t *f,
Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
On Tue, Dec 07, 2004 at 05:14:40PM -0700, Brad Nicholes wrote:
>OK, now that you have enabled upgrades for anything other than
> OPTIONS, I see the problem. Even though there is a content-length
> included in the header, you are saying that the header is being sent
> encrypted but the content is not, correct? And the reason for this is
> because there is more than one filter stack that needs to be modified?
Yes. I think this fixes it, it's a bit of a hack though:
Index: modules/ssl/ssl_engine_io.c
===
--- modules/ssl/ssl_engine_io.c (revision 59)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -1184,22 +1184,26 @@
apr_bucket *b;
SSL *ssl;
-/* Just remove the filter, if it doesn't work the first time, it won't
- * work at all for this request.
- */
-ap_remove_output_filter(f);
+/* f->ctx is non-NULL after the first call to this filter: it's
+ * necessary to pass through directly to the connection output_filters
+ * for the remainder of this request, since the SSL output filter has
+ * not been added to r->output_filters for this request. */
+if (f->ctx) {
+return ap_pass_brigade(f->c->output_filters, bb);
+}
-/* No need to ensure that this is a server with optional SSL, the filter
- * is only inserted if that is true.
- */
-
+/* No need to ensure that this is a server with optional SSL, the
+ * filter is only inserted if that is true. */
upgrade = apr_table_get(r->headers_in, "Upgrade");
if (upgrade == NULL
|| strcmp(ap_getword(r->pool, &upgrade, ','), "TLS/1.0")) {
/* "Upgrade: TLS/1.0, ..." header not found, don't do Upgrade */
+ap_remove_output_filter(f);
return ap_pass_brigade(f->next, bb);
}
+f->ctx = f; /* flag as non-NULL for subsequent passes */
+
apr_table_unset(r->headers_out, "Upgrade");
/* Send the interim 101 response. */
@@ -1245,7 +1249,6 @@
pass the brigade off to the connection based output filters so that the
request can complete encrypted */
return ap_pass_brigade(f->c->output_filters, bb);
-
}
static apr_status_t ssl_io_filter_input(ap_filter_t *f,
Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
OK, now that you have enabled upgrades for anything other than OPTIONS, I see the problem. Even though there is a content-length included in the header, you are saying that the header is being sent encrypted but the content is not, correct? And the reason for this is because there is more than one filter stack that needs to be modified? Brad >>> [EMAIL PROTECTED] Tuesday, December 07, 2004 4:18:13 PM >>> On Tue, Dec 07, 2004 at 03:00:52PM -0700, Brad Nicholes wrote: >So what are you suggesting that the appropriate fix should be? Even > though the protocol.c patch was bogus, it sounds like might have exposed > something else. In fact I am still not seeing where the problem is. Try doing an upgrade on any request where the response includes an entity. I originally tried modifying ssl_io_filter_init and upwards to take an optional request_rec * and pass that to ap_add_output_filter but this broke everything horribly. Possibly I just missed some subtlety but if that's fatally flawed I have no better ideas. joe
Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
On Tue, Dec 07, 2004 at 03:00:52PM -0700, Brad Nicholes wrote: >So what are you suggesting that the appropriate fix should be? Even > though the protocol.c patch was bogus, it sounds like might have exposed > something else. In fact I am still not seeing where the problem is. Try doing an upgrade on any request where the response includes an entity. I originally tried modifying ssl_io_filter_init and upwards to take an optional request_rec * and pass that to ap_add_output_filter but this broke everything horribly. Possibly I just missed some subtlety but if that's fatally flawed I have no better ideas. joe
Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
So what are you suggesting that the appropriate fix should be? Even
though the protocol.c patch was bogus, it sounds like might have exposed
something else. In fact I am still not seeing where the problem is.
The protocol.c patch was just allowing a bad response to happen that
should never have happened in the first place. It seems like the
appropriate OPTIONS response should include C-L:0. Anything else that
actually has content would also contain the appropriate C-L: header.
Brad
>>> [EMAIL PROTECTED] Tuesday, December 07, 2004 12:51:15 PM >>>
On Tue, Dec 07, 2004 at 11:01:13AM -0700, Brad Nicholes wrote:
> >I tested the TLS upgrade stuff last week and it failed because the
> >zero-length chunk to terminate the OPTIONS response was not sent
> through
> >the mod_ssl output filter; is that the same problem you see?
>
> I don't think so. I can make everything work again by simply
allowing
> the "Content-Length: 0" header to be sent through. I'm not sure
what
> impact that header has on the rest of mod_ssl. Obviously by
removing
> it, it causes mod_ssl to *not* do something it was suppose to. My
guess
> is that if the zero-length chunk that terminates the OPTIONS response
is
> not being sent, it is probably a result of mod_ssl not seeing a
> content-length header.
I think we are seeing the same problem. Before, a zero-length OPTIONS
response would be sent with t-e: chunked and a single
zero-terminator-chunk. The "0\r\n\r\n" packet was being sent
unencrypted
and breaking the SSL connection. After you reverted the protocol.c
change, a zero-length OPTIONS response is sent with C-L: 0 and hence
terminates after the headers. Subsequent requests on the connection
get
the right filter stack.
So you have successfully hidden the mod_ssl bug for upgrades with
zero-length responses. The fact that mod_ssl (incorrectly) refuses to
upgrade anything other than an OPTIONS request makes the bug less
obvious since OPTIONS responses are rarely anything but zero-length,
but
I'll fix that.
For an upgrade on GET the strace now looks like:
send(3, "GET / HTTP/1.1\r\nHost"..., 173, 0) = 173
recv(3, "HTTP/1.1 101 Switchi"..., 4096, 0) = 85
read(4, "*J\204\17\342g l\327U\323/\271"..., 32) = 32
write(3, "\200|\1\3\1\0c\0\0\0\20\0\0009"..., 126) = 126
read(3, "\26\3\1\", 5) = 5
read(3, "a\20\363\37{\372\347\205\350\36"..., 48) = 48
read(3, " BTW, what are you using to test TLS Upgrade with?
A hacked up version of neon.
Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
On Tue, Dec 07, 2004 at 11:01:13AM -0700, Brad Nicholes wrote:
> >I tested the TLS upgrade stuff last week and it failed because the
> >zero-length chunk to terminate the OPTIONS response was not sent
> through
> >the mod_ssl output filter; is that the same problem you see?
>
> I don't think so. I can make everything work again by simply allowing
> the "Content-Length: 0" header to be sent through. I'm not sure what
> impact that header has on the rest of mod_ssl. Obviously by removing
> it, it causes mod_ssl to *not* do something it was suppose to. My guess
> is that if the zero-length chunk that terminates the OPTIONS response is
> not being sent, it is probably a result of mod_ssl not seeing a
> content-length header.
I think we are seeing the same problem. Before, a zero-length OPTIONS
response would be sent with t-e: chunked and a single
zero-terminator-chunk. The "0\r\n\r\n" packet was being sent unencrypted
and breaking the SSL connection. After you reverted the protocol.c
change, a zero-length OPTIONS response is sent with C-L: 0 and hence
terminates after the headers. Subsequent requests on the connection get
the right filter stack.
So you have successfully hidden the mod_ssl bug for upgrades with
zero-length responses. The fact that mod_ssl (incorrectly) refuses to
upgrade anything other than an OPTIONS request makes the bug less
obvious since OPTIONS responses are rarely anything but zero-length, but
I'll fix that.
For an upgrade on GET the strace now looks like:
send(3, "GET / HTTP/1.1\r\nHost"..., 173, 0) = 173
recv(3, "HTTP/1.1 101 Switchi"..., 4096, 0) = 85
read(4, "*J\204\17\342g l\327U\323/\271"..., 32) = 32
write(3, "\200|\1\3\1\0c\0\0\0\20\0\0009"..., 126) = 126
read(3, "\26\3\1\", 5) = 5
read(3, "a\20\363\37{\372\347\205\350\36"..., 48) = 48
read(3, " BTW, what are you using to test TLS Upgrade with?
A hacked up version of neon.
Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
This patch also looks bogus given the fact that the content-length
header for a r->header_only request is already being handled in
ap_http_header_filter(). Removing all Content-Length: 0 headers in the
ap_content_length_filter() seems like overkill.
Brad
>>> [EMAIL PROTECTED] Tuesday, December 07, 2004 10:14:28 AM >>>
It appears that the patch for bug 18757 which disallows a
content-length header for all requests with a content-length of 0 is
too
broad.
Index: D:/Projects/2.x/httpd-trunk/server/protocol.c
===
--- D:/Projects/2.x/httpd-trunk/server/protocol.c (revision
104639)
+++ D:/Projects/2.x/httpd-trunk/server/protocol.c (revision
104923)
@@ -1244,8 +1244,11 @@
*
* We can only set a C-L in the response header if we haven't
already
* sent any buckets on to the next output filter for this
request.
+ *
+ * Also check against cases of zero bytes sent, to avoid a bogus
+ * C-L on HEAD requests, or no-body GETs like 204s.
*/
-if (ctx->data_sent == 0 && eos) {
+if (ctx->data_sent == 0 && eos && r->bytes_sent > 0 ) {
ap_set_content_length(r, r->bytes_sent);
}
Property changes on: D:/Projects/2.x/httpd-trunk/server/protocol.c
___
Name: cvs2svn:cvs-rev
- 1.150
+ 1.151
The bug simply says that the content-length should be removed just for
HEAD requests. By removing it for all requests including an OPTIONS
requests, causes the SSL handshake to fail after a TLS upgrade
(somebody
with more knowledge of SSL would have to explain why). According to
the
bugzilla report, this patch didn't completely resolve the issue anyway.
I will be reverting the patch shortly unless somebody has a better
fix.
Brad
Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
>I tested the TLS upgrade stuff last week and it failed because the >zero-length chunk to terminate the OPTIONS response was not sent through >the mod_ssl output filter; is that the same problem you see? I don't think so. I can make everything work again by simply allowing the "Content-Length: 0" header to be sent through. I'm not sure what impact that header has on the rest of mod_ssl. Obviously by removing it, it causes mod_ssl to *not* do something it was suppose to. My guess is that if the zero-length chunk that terminates the OPTIONS response is not being sent, it is probably a result of mod_ssl not seeing a content-length header. BTW, what are you using to test TLS Upgrade with? Brad >>> [EMAIL PROTECTED] Tuesday, December 07, 2004 10:39:04 AM >>> On Tue, Dec 07, 2004 at 10:14:28AM -0700, Brad Nicholes wrote: > It appears that the patch for bug 18757 which disallows a > content-length header for all requests with a content-length of 0 is too > broad. ... > > The bug simply says that the content-length should be removed just for > HEAD requests. By removing it for all requests including an OPTIONS > requests, causes the SSL handshake to fail after a TLS upgrade (somebody > with more knowledge of SSL would have to explain why). According to the I tested the TLS upgrade stuff last week and it failed because the zero-length chunk to terminate the OPTIONS response was not sent through the mod_ssl output filter; is that the same problem you see? The problem was that r->connection->output_filters had been correctly adjusted to use the SSL output filter but r->output_filters had not, which looks purely like an issue in mod_ssl. joe > bugzilla report, this patch didn't completely resolve the issue anyway. > I will be reverting the patch shortly unless somebody has a better fix. > > Brad
Re: Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
On Tue, Dec 07, 2004 at 10:14:28AM -0700, Brad Nicholes wrote: > It appears that the patch for bug 18757 which disallows a > content-length header for all requests with a content-length of 0 is too > broad. ... > > The bug simply says that the content-length should be removed just for > HEAD requests. By removing it for all requests including an OPTIONS > requests, causes the SSL handshake to fail after a TLS upgrade (somebody > with more knowledge of SSL would have to explain why). According to the I tested the TLS upgrade stuff last week and it failed because the zero-length chunk to terminate the OPTIONS response was not sent through the mod_ssl output filter; is that the same problem you see? The problem was that r->connection->output_filters had been correctly adjusted to use the SSL output filter but r->output_filters had not, which looks purely like an issue in mod_ssl. joe > bugzilla report, this patch didn't completely resolve the issue anyway. > I will be reverting the patch shortly unless somebody has a better fix. > > Brad
Patch for bug 18757 breaks TLS upgrade - [Content-Length is removed from HEAD requests]
It appears that the patch for bug 18757 which disallows a
content-length header for all requests with a content-length of 0 is too
broad.
Index: D:/Projects/2.x/httpd-trunk/server/protocol.c
===
--- D:/Projects/2.x/httpd-trunk/server/protocol.c (revision
104639)
+++ D:/Projects/2.x/httpd-trunk/server/protocol.c (revision
104923)
@@ -1244,8 +1244,11 @@
*
* We can only set a C-L in the response header if we haven't
already
* sent any buckets on to the next output filter for this
request.
+ *
+ * Also check against cases of zero bytes sent, to avoid a bogus
+ * C-L on HEAD requests, or no-body GETs like 204s.
*/
-if (ctx->data_sent == 0 && eos) {
+if (ctx->data_sent == 0 && eos && r->bytes_sent > 0 ) {
ap_set_content_length(r, r->bytes_sent);
}
Property changes on: D:/Projects/2.x/httpd-trunk/server/protocol.c
___
Name: cvs2svn:cvs-rev
- 1.150
+ 1.151
The bug simply says that the content-length should be removed just for
HEAD requests. By removing it for all requests including an OPTIONS
requests, causes the SSL handshake to fail after a TLS upgrade (somebody
with more knowledge of SSL would have to explain why). According to the
bugzilla report, this patch didn't completely resolve the issue anyway.
I will be reverting the patch shortly unless somebody has a better fix.
Brad
