Re: [PATCH] Truncate HTTP response bodies to match clen
On 06/28/2009 01:50 PM, Henrik Nordstrom wrote: lör 2009-06-27 klockan 08:39 -0600 skrev Alex Rousskov: Do you think this change should go in? A strong + from me. Committed to trunk. Amos, please commit to 3.1 (using trunk) and 3.0 (using the patch I originally posted as it was based on 3p0-plus). For what it's worth Squid-2 was fixed long ago. But it took quite some iterations before all cases were covered.. When this condition is detected it not only discards excess data and closes the connection, it also throws out the current response from the cache as it's malformed. I agree that we should not cache the truncated response. Unfortunately, we cannot use HttpStateData::cacheableReply for that because it is called before we know about the body truncation. Will everything work as expected if we call entry-makePrivate() later in the process, when the extra body starts coming in? Thank you, Alex.
Re: [PATCH] Truncate HTTP response bodies to match clen
Alex Rousskov wrote: On 06/28/2009 01:50 PM, Henrik Nordstrom wrote: lör 2009-06-27 klockan 08:39 -0600 skrev Alex Rousskov: Do you think this change should go in? A strong + from me. Committed to trunk. Amos, please commit to 3.1 (using trunk) and 3.0 (using the patch I originally posted as it was based on 3p0-plus). For what it's worth Squid-2 was fixed long ago. But it took quite some iterations before all cases were covered.. When this condition is detected it not only discards excess data and closes the connection, it also throws out the current response from the cache as it's malformed. I agree that we should not cache the truncated response. Unfortunately, we cannot use HttpStateData::cacheableReply for that because it is called before we know about the body truncation. Will everything work as expected if we call entry-makePrivate() later in the process, when the extra body starts coming in? Thank you, Alex. Just in passing on one of the squid-users question research I noted a call that seemed to cause the RELEASE to happen for an object. If thats available it would seem to be the right thing. We are not making the URL no-cachable only the currently received object or variant. Amos -- Please be using Current Stable Squid 2.7.STABLE6 or 3.0.STABLE16 Current Beta Squid 3.1.0.9
Re: [PATCH] Truncate HTTP response bodies to match clen
fre 2009-06-26 klockan 12:05 -0600 skrev Alex Rousskov: TODO: simply truncating read content would not work for pipelined responses. We should preserve extra content for the next transaction on a pconn. Correct, and is a major reason NOT to do pipelining as it then becomes impossible to protect from the response splitting attack you just closed... Regards Henrik
Re: [PATCH] Truncate HTTP response bodies to match clen
Amos Jeffries has voted approve. Status is now: Approved For details, see: http://bundlebuggy.aaronbentley.com/project/squid/request/%3C4A450DFE.6040604%40measurement-factory.com%3E Project: Squid
Re: [PATCH] Truncate HTTP response bodies to match clen
Alex Rousskov wrote: On 06/27/2009 06:32 AM, Amos Jeffries wrote: Alex Rousskov wrote: Truncate too-long HTTP response bodies to match their Content-Length header. Sometimes a broken server sends more than Content-Length bytes in the response. For example, a 302 redirect message with Content-Length: 0 header may include an HTML body. Squid used to send everything it read to the client, even if it read more than the Content-Length bytes. That may have helped in some cases, but I think we should be more conservative when dealing with broken servers to combat message smuggling attacks and other bad side-effects for clients. We now do not forward more than the advertised content length and declare the connection with a broken server non-persistent. Chunked responses (that Squid should not receive and that must not have a Content-Length header) are not truncated because RFC 2616 says we MUST ignore their Content-Length header. TODO: simply truncating read content would not work for pipelined responses. We should preserve extra content for the next transaction on a pconn. --- The attached patch is against Squid 3.0 and has been tested in production. More testing is welcomed. I will port to trunk if needed if the change is accepted. Thank you, Alex. Trouble... I was of the understanding that Squid already was doing these truncation + non-pconn flagging :( I've written off many bugs in the last few years to this behavior... Is this some side case you found internal to Squid? or are the current Squid-3 actually not protecting people from message-appending? The problem was triggered at the production site running Squid 3p0-plus that was accelerating a broken server application. The application was including bodies in 302 redirects that had Content-Length set to zero. That, in turn, broke some clients. Neither the application nor the clients could be fixed, of course. Besides, it is probably an HTTP violation to forward trailing garbage anyway, unless we switch to some kind of tunneling mode and become a TCP proxy, but then you get problems with message smuggling and such. Do you think this change should go in? I do. I'm not the best for RFC compliance issues, but its a stuffing attack. And we should be protecting from those. Amos -- Please be using Current Stable Squid 2.7.STABLE6 or 3.0.STABLE16 Current Beta Squid 3.1.0.9
Re: [PATCH] Truncate HTTP response bodies to match clen
lör 2009-06-27 klockan 08:39 -0600 skrev Alex Rousskov: Do you think this change should go in? A strong + from me. For what it's worth Squid-2 was fixed long ago. But it took quite some iterations before all cases were covered.. When this condition is detected it not only discards excess data and closes the connection, it also throws out the current response from the cache as it's malformed. Regards Henrik
Re: [PATCH] Truncate HTTP response bodies to match clen
Besides, it is probably an HTTP violation to forward trailing garbage anyway, unless we switch to some kind of tunneling mode and become a TCP proxy, but then you get problems with message smuggling and such. Do you think this change should go in? Trunk and 3.1, I think so, 3.0 I'm uncertain. If the time allows, maybe protecting it with a fast-acl-backed selector (defaulting to 'apply to all') would be the most flexible solution, allowing (especially rproxy) administrators to fine-tune the behaviour. -- /kinkie
Re: [PATCH] Truncate HTTP response bodies to match clen
Alex Rousskov wrote: Truncate too-long HTTP response bodies to match their Content-Length header. Sometimes a broken server sends more than Content-Length bytes in the response. For example, a 302 redirect message with Content-Length: 0 header may include an HTML body. Squid used to send everything it read to the client, even if it read more than the Content-Length bytes. That may have helped in some cases, but I think we should be more conservative when dealing with broken servers to combat message smuggling attacks and other bad side-effects for clients. We now do not forward more than the advertised content length and declare the connection with a broken server non-persistent. Chunked responses (that Squid should not receive and that must not have a Content-Length header) are not truncated because RFC 2616 says we MUST ignore their Content-Length header. TODO: simply truncating read content would not work for pipelined responses. We should preserve extra content for the next transaction on a pconn. --- The attached patch is against Squid 3.0 and has been tested in production. More testing is welcomed. I will port to trunk if needed if the change is accepted. Thank you, Alex. Trouble... I was of the understanding that Squid already was doing these truncation + non-pconn flagging :( I've written off many bugs in the last few years to this behavior... Is this some side case you found internal to Squid? or are the current Squid-3 actually not protecting people from message-appending? Amos -- Please be using Current Stable Squid 2.7.STABLE6 or 3.0.STABLE16 Current Beta Squid 3.1.0.9
Re: [PATCH] Truncate HTTP response bodies to match clen
On 06/27/2009 06:32 AM, Amos Jeffries wrote: Alex Rousskov wrote: Truncate too-long HTTP response bodies to match their Content-Length header. Sometimes a broken server sends more than Content-Length bytes in the response. For example, a 302 redirect message with Content-Length: 0 header may include an HTML body. Squid used to send everything it read to the client, even if it read more than the Content-Length bytes. That may have helped in some cases, but I think we should be more conservative when dealing with broken servers to combat message smuggling attacks and other bad side-effects for clients. We now do not forward more than the advertised content length and declare the connection with a broken server non-persistent. Chunked responses (that Squid should not receive and that must not have a Content-Length header) are not truncated because RFC 2616 says we MUST ignore their Content-Length header. TODO: simply truncating read content would not work for pipelined responses. We should preserve extra content for the next transaction on a pconn. --- The attached patch is against Squid 3.0 and has been tested in production. More testing is welcomed. I will port to trunk if needed if the change is accepted. Thank you, Alex. Trouble... I was of the understanding that Squid already was doing these truncation + non-pconn flagging :( I've written off many bugs in the last few years to this behavior... Is this some side case you found internal to Squid? or are the current Squid-3 actually not protecting people from message-appending? The problem was triggered at the production site running Squid 3p0-plus that was accelerating a broken server application. The application was including bodies in 302 redirects that had Content-Length set to zero. That, in turn, broke some clients. Neither the application nor the clients could be fixed, of course. Besides, it is probably an HTTP violation to forward trailing garbage anyway, unless we switch to some kind of tunneling mode and become a TCP proxy, but then you get problems with message smuggling and such. Do you think this change should go in? Thank you, Alex.
[PATCH] Truncate HTTP response bodies to match clen
Truncate too-long HTTP response bodies to match their Content-Length header.
Sometimes a broken server sends more than Content-Length bytes in the
response. For example, a 302 redirect message with Content-Length: 0
header may include an HTML body. Squid used to send everything it read
to the client, even if it read more than the Content-Length bytes. That
may have helped in some cases, but I think we should be more
conservative when dealing with broken servers to combat message
smuggling attacks and other bad side-effects for clients.
We now do not forward more than the advertised content length and
declare the connection with a broken server non-persistent.
Chunked responses (that Squid should not receive and that must not have
a Content-Length header) are not truncated because RFC 2616 says we MUST
ignore their Content-Length header.
TODO: simply truncating read content would not work for pipelined
responses. We should preserve extra content for the next transaction on
a pconn.
---
The attached patch is against Squid 3.0 and has been tested in
production. More testing is welcomed. I will port to trunk if needed if
the change is accepted.
Thank you,
Alex.
Truncate too-long HTTP response bodies to match their Content-Length header.
Sometimes a broken server sends more than Content-Length bytes in the
response. For example, a 302 redirect message with Content-Length: 0 header
may include an HTML body. Squid used to send everything it read to the
client, even if it read more than the Content-Length bytes. That may have
helped in some cases, but I think we should be more conservative when dealing
with broken servers to combat message smuggling attacks and other bad
side-effects for clients.
We now do not forward more than the advertised content length and declare the
connection with a broken server non-persistent.
Chunked responses (that Squid should not receive and that must not have a
Content-Length header) are not truncated because RFC 2616 says we MUST ignore
their Content-Length header.
TODO: simply truncating read content would not work for pipelined responses.
We should preserve extra content for the next transaction on a pconn.
=== modified file 'src/MemBuf.cc'
--- src/MemBuf.cc 2006-09-20 14:13:38 +
+++ src/MemBuf.cc 2009-05-26 16:20:20 +
@@ -220,6 +220,15 @@
PROF_stop(MemBuf_consume);
}
+// removes last tailSize bytes
+void MemBuf::truncate(mb_size_t tailSize)
+{
+const mb_size_t cSize = contentSize();
+assert(0 = tailSize tailSize = cSize);
+assert(!stolen); /* not frozen */
+size -= tailSize;
+}
+
// calls memcpy, appends exactly size bytes, extends buffer if needed
void MemBuf::append(const char *newContent, mb_size_t sz)
{
=== modified file 'src/MemBuf.h'
--- src/MemBuf.h 2008-04-06 00:35:11 +
+++ src/MemBuf.h 2009-05-26 15:45:58 +
@@ -73,6 +73,7 @@
void consume(mb_size_t sz); // removes sz bytes, moving content left
void append(const char *c, mb_size_t sz); // grows if needed and possible
void appended(mb_size_t sz); // updates content size after external append
+void truncate(mb_size_t sz); // removes sz last bytes
// XXX: convert global memBuf*() functions into methods
=== modified file 'src/http.cc'
--- src/http.cc 2009-05-01 15:50:41 +
+++ src/http.cc 2009-06-10 04:56:17 +
@@ -68,7 +68,7 @@
HttpHeader * hdr_out, int we_do_ranges, http_state_flags);
HttpStateData::HttpStateData(FwdState *theFwdState) : ServerStateData(theFwdState),
-header_bytes_read(0), reply_bytes_read(0)
+header_bytes_read(0), reply_bytes_read(0), body_bytes_truncated(0)
{
debugs(11,5,HERE HttpStateData this created);
ignoreCacheControl = false;
@@ -923,6 +923,9 @@
if (body_bytes_read vrep-content_length)
return INCOMPLETE_MSG;
+
+if (body_bytes_truncated 0) // already read more than needed
+return COMPLETE_NONPERSISTENT_MSG; // disable pconns
}
/* If there is no message body or we got it all, we can be persistent */
@@ -1099,6 +1102,33 @@
return false; // quit on error
}
+/** truncate what we read if we read too much so that writeReplyBody()
+writes no more than what we should have read */
+void
+HttpStateData::truncateVirginBody()
+{
+assert(flags.headers_parsed);
+
+HttpReply *vrep = virginReply();
+int64_t clen = -1;
+if (!vrep-expectingBody(request-method, clen) || clen 0)
+return; // no body or a body of unknown size, including chunked
+
+const int64_t body_bytes_read = reply_bytes_read - header_bytes_read;
+if (body_bytes_read - body_bytes_truncated = clen)
+return; // we did not read too much or already took care of the extras
+
+if (const int64_t extras = body_bytes_read - body_bytes_truncated - clen) {
+// server sent more that the advertised content length
+debugs(11,5, HERE body_bytes_read= body_bytes_read
+
Re: [PATCH] Truncate HTTP response bodies to match clen
Bundle Buggy has detected this merge request. For details, see: http://bundlebuggy.aaronbentley.com/project/squid/request/%3C4A450DFE.6040604%40measurement-factory.com%3E Project: Squid
