Bug 47087 - Incorrect request body handling with Expect: 100-continue if the client does not receive a transmitted 300 or 400 response prior to sending its body
Summary: Incorrect request body handling with Expect: 100-continue if the client does ...
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: Core (show other bugs)
Version: 2.2.11
Hardware: All All
: P2 normal with 3 votes (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: FixedInTrunk, PatchAvailable
Depends on:
Blocks:
 
Reported: 2009-04-23 16:08 UTC by Steven Bush
Modified: 2014-02-17 13:54 UTC (History)
8 users (show)



Attachments
Experimental patch. (1.04 KB, patch)
2009-11-02 07:36 UTC, Nick Kew
Details | Diff
Amended experimental patch (2.53 KB, patch)
2009-11-02 08:13 UTC, Nick Kew
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steven Bush 2009-04-23 16:08:33 UTC
when responding to a HTTP request that has the following headers:

Expect: 100-continue
Transfer-Encoding: chunked

if the response is anything other than simply "100 Continue" (e.g. if the response is 302 redirect or 401 authorization required), the server interprets any body elements in the request as being part of a new request instead of discarding the chunks.

The scenario is as follows:
The client transmits:

POST /RedirectedLocation HTTP/1.1
Host: localhost:9999
User-Agent: MyAgent/1.1
Content-Type: multipart/form-data; boundary=Boundary1240524002
Transfer-Encoding: chunked
Expect: 100-continue
CRLFCRLF

The server immediately responds with something like:
HTTP/1.1 302 Found
Date: Thu, 23 Apr 2009 22:00:05 GMT
Server: Apache
Location: /newLocation
Content-Length: 257
Keep-Alive: timeout=10, max=100
Connection: Keep-Alive
Content-Type: text/html; charset=iso-8859-1

<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>302 Found</title>
</head><body>
<h1>Found</h1>
<p>The document has moved <a href="/newLocation">here</a>.</p>
<hr>
<address>Apache Server at localhost Port 9999</address>
</body></html>

However, in this scenario due to network lag, this response is not received by the client before its timeout period expires waiting for the 100 continue or for another response.  Therefore, having not received the response, the client begins to send the body:


49
--Boundary1240524002
Content-Disposition: form-data; name="FileName"

At this point, the client receives the 302 response and stops transmitting by sending this:

0

However, the Apache server responds with the following:
NOTE: there is no HTTP header information associated with the rest of the response and it appears that the server is treating the chunk size "49" as an unknown HTTP method

<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>501 Method Not Implemented</title>
</head><body>
<h1>Method Not Implemented</h1>
<p>49 to /index.htm not supported.<br />
</p>
<hr>
<address>Apache Server at myserver Port 9999</address>
</body></html>

It looks like RFC2616 section 8.2.3 Use of the 100 (Continue) Status (http://www.ietf.org/rfc/rfc2616.txt), is indeterminate in this matter.  However, I believe the current behavior is incorrect given that HTML data is being sent across the wire without an associated HTTP header that would indicate its content-length or chunked encoding.

I would propose that the correct behavior would be to either wait for and discard the rest of the response or close the connection.  This seems to be in line with what is said about this in the RFC (relevant section delimited by >>>><<<<):

Upon receiving a request which includes an Expect request-header field with the "100-continue" expectation, an origin server MUST either respond with 100 (Continue) status and continue to read from the input stream, or respond with a final status code. The origin server MUST NOT wait for the request body before sending the 100 (Continue) response.  >>>>>>If it responds with a final status code, it MAY close the transport connection or it MAY continue to read and discard the rest of the request.<<<<<<  It MUST NOT perform the requested method if it returns a final status code.
Comment 1 Stuart A. Malone 2009-08-27 09:22:42 UTC
I also ran into this bug when trying to write a simple WebDAV client using Microsoft's .NET Framework. I have a very simple C# test program that demonstrates the problem, and have Wireshark network traces that demonstrate that the error is in Apache rather than .NET.

You can read the discussion and find links to the network traces on the MSDN forums at:

<http://social.msdn.microsoft.com/Forums/en-US/ncl/thread/5c576a2d-2f13-485c-8ada-b4c3ee127d3c/>

I just re-ran the test against Apache 2.2.13 on Mac OS X Snow Leopard, and the problem is still there.

It appears that Apache is violating this paragraph from RFC 2616:

      - Upon receiving a request which includes an Expect request-header
        field with the "100-continue" expectation, an origin server MUST
        either respond with 100 (Continue) status and continue to read
        from the input stream, or respond with a final status code. The
        origin server MUST NOT wait for the request body before sending
        the 100 (Continue) response. If it responds with a final status
        code, it MAY close the transport connection or it MAY continue
        to read and discard the rest of the request.  It MUST NOT
        perform the requested method if it returns a final status code.
Comment 2 Nick Kew 2009-08-29 22:03:10 UTC
Hmm.

In the absence of something that's syntactically an HTTP/1.0 or higher request, the server falls back to assuming HTTP/0.9, which is where the bare HTML page comes from.  But since it's not a GET, I guess a 400 response would make more sense.

Re: the recent comment, that makes sense to me.  Unless we close the connection, we should retain its state so we can interpret subsequent data as not-a-request.  But that still leaves ambiguity if the next data look like a syntactically valid request.

I guess closing the connection is the only safe thing to do here, unless someone has a better idea.  Thinking.
Comment 3 Steven Bush 2009-08-31 10:04:55 UTC
(In reply to comment #2)
> Re: the recent comment, that makes sense to me.  Unless we close the
> connection, we should retain its state so we can interpret subsequent data as
> not-a-request.  But that still leaves ambiguity if the next data look like a
> syntactically valid request.
> 
> I guess closing the connection is the only safe thing to do here, unless
> someone has a better idea.  Thinking.

I think this is right.  The RFC spec gives these two options:

"If [the server] responds with a final status code, it MAY close the transport connection or it MAY continue to read and discard the rest of the request."

Closing the connection is nice and immediate.  Reading and discarding the rest of the request is fine as well, but assumes that the client will also behave nicely.  If the next chunk looks like a valid request, then any ambiguity is on the shoulders of the client.  However, since this would be a change in behavior from prior versions, it could break existing clients that are reliant upon the current buggy behavior.

I agree that the safest course of action is to close the connection, and this is one of the options specified in the RFC, so that should be fine
Comment 4 Nick Kew 2009-11-02 07:36:25 UTC
Created attachment 24460 [details]
Experimental patch.

I'm attaching a patch that should fix this by closing the connection, with the proviso that r->expecting_100 is set (a check for the request header itself at this point would be slower).  If you have a testbed for provoking this behavior, it would be great if you could test the patch against it.
Comment 5 Nick Kew 2009-11-02 07:45:01 UTC
Scrap that: it's not going to work because we don't set r->status to HTTP_CONTINUE.  Need to fix up the two points where an interim response is sent so we have a test that works.
Comment 6 Nick Kew 2009-11-02 08:13:22 UTC
Created attachment 24461 [details]
Amended experimental patch

OK, anyone in a position to test-drive the amended patch is encouraged to do so.  This one unsets r->expecting_100 as soon as an interim response is sent (which might want further thought) but best to test whether it fixes this bug first!
Comment 7 Stuart A. Malone 2009-11-03 05:25:16 UTC
Nick, the patch is working for me. I applied it to the httpd-2.2.14 sources and re-ran my .NET Framework test code that combines PROPFIND, digest authentication, and 100-continue. Before the patch, the test would fail if the PROPFIND contained a body. After the patch, the PROPFIND succeeds both with and without a body.  It's a very limited test, but so far, so good.

Thanks for working on this.
Comment 8 Nick Kew 2009-12-07 10:13:29 UTC
*** Bug 46709 has been marked as a duplicate of this bug. ***
Comment 9 Nick Kew 2009-12-07 10:17:15 UTC
(In reply to comment #7)
> Nick, the patch is working for me. I applied it to the httpd-2.2.14 sources and
> re-ran my .NET Framework test code that combines PROPFIND, digest
> authentication, and 100-continue. Before the patch, the test would fail if the
> PROPFIND contained a body. After the patch, the PROPFIND succeeds both with and
> without a body.  It's a very limited test, but so far, so good.
> 
> Thanks for working on this.

Thanks for the comment - just noticed it when I marked another bug a duplicate of this.

Bug me if I don't revisit the patch in the next week or two, given your confirmation.
Comment 10 Joe Orton 2009-12-07 13:52:35 UTC
Disabling keepalive unconditionally for all requests which include Expect: 100-continue is overkill.  I think it would be sufficient to fix this bug to disable keepalive when sending a final response, if the client has sent "Expect: 100-continue", and no 1xx interim response has already been sent.

RFC 2616 lists that as a "MAY" if I interpret section 8.2.3 correctly.
Comment 11 Nick Kew 2009-12-07 15:59:16 UTC
(In reply to comment #10)
> Disabling keepalive unconditionally for all requests which include Expect:
> 100-continue is overkill.  I think it would be sufficient to fix this bug to
> disable keepalive when sending a final response, if the client has sent
> "Expect: 100-continue", and no 1xx interim response has already been sent.
> 
> RFC 2616 lists that as a "MAY" if I interpret section 8.2.3 correctly.

Joe, is that a reservation about the patch?  It will disable keepalive if and only if we sent a non-100 response when the client was expecting 100.  That's an errordocument (or some app that can't deal with 100 and should therefore presumably force HTTP/1.0).  Seems right to me.
Comment 12 Sven Mueller 2009-12-15 04:39:41 UTC
(In reply to comment #10)
> Disabling keepalive unconditionally for all requests which include Expect:
> 100-continue is overkill.  I think it would be sufficient to fix this bug to
> disable keepalive when sending a final response, if the client has sent
> "Expect: 100-continue", and no 1xx interim response has already been sent.
> 
> RFC 2616 lists that as a "MAY" if I interpret section 8.2.3 correctly.

As I was asked in https://issues.apache.org/bugzilla/show_bug.cgi?id=46709#c3 wether I would be able to test drive this patch: No, not at the moment. It might be possible again in early 2010 (i.e. end of February, begin of March).

Anyhow, as far as I understood from the discussion, this patch disables keepalive for any response but "100 Continue", which means the connections are closed after the final response. Unfortunately, this would cause troubles here, as we found out. Some of the clients simply don't reconnect when they asked for a keep-alive session. If they get a disconnect with the non-100 response, they seem to report an error.

Please note that I don't have access to the client sources, and probably will never have that access. I just know a few facts about them:
They use the .Net framework, don't authenticate on first try (as a means to find out how to authenticate), request keep-alive and 100-continue. The rest differs: Some clients correctly reconnect on no-keepalive, some don't. Some handle a 417 return code correctly, some don't.....
Comment 13 Nick Kew 2009-12-15 05:24:35 UTC
(In reply to comment #12)

> Anyhow, as far as I understood from the discussion, this patch disables
> keepalive for any response but "100 Continue", which means the connections are
> closed after the final response. Unfortunately, this would cause troubles here,
> as we found out. Some of the clients simply don't reconnect when they asked for
> a keep-alive session. If they get a disconnect with the non-100 response, they
> seem to report an error.

Closing the connection is always at least an option under HTTP, and any client that can't cope is broken.  Bearing in mind that non-100 responses are commonly genuine errors and should indeed be reported as such, are you saying you get an error in the case of a non-error response?
Comment 14 Andy Wang 2009-12-15 13:26:22 UTC
We've tested out this patch to see if it solves the problem and indeed it does.  The behavior we were seeing before, and looking at wireshark captures was
1. Client sends request with Except: 100-continue
2. Server responds with non 100 response (401, 302, whatever) and keep the keepalive connection open.
3. Client continues to send original request body from #1 without headers
4. Server barfs (and imo, rightly so).

With the patch
1. Client sends request with Expect: 100-continue
2. Server responds with non 100 resopnse and Connection: close
3. Client continues to send original request body
4. Server responds with RST to extra crap from #3.

I think the questions for the comment #12 problem are
1) is it the fact that the keep-alive was broken that caused the problem
2) or is it that the client didn't know what to do with the TCP RST?
two separate problems, but both it seems are a problem with the client not being resilient enough.

So based on what our tests indicates, this patch seems to do what it needs to do.  It'd be better if the client was smarter and didn't waste the traffic on the extra body, but oh well.  Any idea if this is something that might make it into 2.2 branch sometime soon?
Comment 15 Sven Mueller 2009-12-16 07:07:48 UTC
(In reply to comment #13)
> (In reply to comment #12)
> 
> > Anyhow, as far as I understood from the discussion, this patch disables
> > keepalive for any response but "100 Continue", which means the connections are
> > closed after the final response. Unfortunately, this would cause troubles here,
> > as we found out. Some of the clients simply don't reconnect when they asked for
> > a keep-alive session. If they get a disconnect with the non-100 response, they
> > seem to report an error.
> 
> Closing the connection is always at least an option under HTTP, and any client
> that can't cope is broken.  Bearing in mind that non-100 responses are commonly
> genuine errors and should indeed be reported as such, are you saying you get an
> error in the case of a non-error response?

Well, yes. I agree that the clients are broken. But as those are paying customers (and their client used to work with Apache 2.0), I'm not exactly in a position to force them to immediately fix/update their clients.

What we saw was this sequence:
Client sends request with 100-continue expectation (note: It actually sends the full request, headers and body, as far as I could tell from tcpdump/wireshark protocols), unauthenticated. Server answers with "authentication required", and gets to see body of request (which actually is a single line XML thingy in our case) with the header of the authenticated request appended to it (ie. "<xml>...</xml>PUT ...." or something similar).

Trouble is the broken clients which won't retry. If we use nokeepalive on the clients, some correctly reopen the connection, sending the second request cleanly. Others however won't do so, so we needed to ignore their 100-continue expectation, causing the http daemon to read the full initial request before sending the authentication required response, after that the client successfully sent an authenticated request.

So for us, the update committed as a result of this bug report doesn't actually fix all client problems we had (but most of them, like 75%). To work around the problems with the rest of our customers clients, the ignore-continue patch I proposed in the other bug report (https://issues.apache.org/bugzilla/show_bug.cgi?id=46709) would still be needed.

<rant>Damn .Net, why can't it just do the correct things by default? Oh, I forgot it is a microsoft product, so that probably explains it</rant>

regards,
Sven
Comment 16 Nick Kew 2009-12-16 08:00:27 UTC
(In reply to comment #15)
> [chop]

This problem really belongs on the users@ list.

Does it help if you either:
 (a) use mod_headers to unset the Expect: request header for the app?
or:
 (b) use a browser match to force HTTP/1.0 for the client?
If not, then your own hack may be your best workaround.

Come to think of it, you have another valid point.  We should ensure any surplus request data are mopped up before closing the connection.  I'll try and find time to test the patch for that and fix it up if necessary.
Comment 17 Ruediger Pluem 2009-12-16 12:03:27 UTC
(In reply to comment #16)

> 
> Come to think of it, you have another valid point.  We should ensure any
> surplus request data are mopped up before closing the connection.  I'll try and
> find time to test the patch for that and fix it up if necessary.

Isn't this done already by ap_discard_request_body?
Comment 18 Nick Kew 2009-12-16 15:32:51 UTC
(In reply to comment #17)

> Isn't this done already by ap_discard_request_body?

That's what I meant to check: is anything causing that not to happen?
Comment 19 Ernest Mueller 2011-03-10 16:14:22 UTC
This is still unresolved, right?  We ran into this problem this week, we tried to upgrade our Apache installation that proxies to our app servers and it caused all our desktop software out there "in the world" to stop being able to connect correctly. Thanks to Stack Overflow we found the problem and implemented the header-stripping fix but wanted to register our interest in this getting fixed.
Comment 20 Nick Kew 2011-03-26 07:02:55 UTC
(In reply to comment #19)
> This is still unresolved, right?

You're right.  The patch has been in trunk for over a year but not backported to the 2.2.x (stable) branch.  I've just proposed it for backport.
Comment 21 Tom Baxter 2011-03-31 18:25:11 UTC
Thank you Nick. That would be greatly appreciated.I'm stuck back at 2.2.8 IBM HTTP server.
Comment 22 Walter Kacynski 2011-08-02 14:02:10 UTC
I have encountered this problem mostly with .NET clients.  Does anyone know if a bug has been submitted to Microsoft on this problem?
Comment 23 Stefan Fritsch 2011-09-17 16:02:12 UTC
r1098104 / 2.2.18