On 20/04/2020 10:45, Rainer Jung wrote:
> Hi Mark, hi all,
>
> Am 20.04.2020 um 11:15 schrieb Mark Thomas:
>> On 19/04/2020 14:04, Rainer Jung wrote:
>>> It might be too simplistic, but the following at least stops the
>>> connection close (but I don't know, whether it also prevents it in cases
>>> where it should still be done):
>>>
>>> diff --git a/java/org/apache/coyote/http11/Http11Processor.java
>>> b/java/org/apache/coyote/http11/Http11Processor.java
>>> index aa1569cfdc..f38993b04d 100644
>>> --- a/java/org/apache/coyote/http11/Http11Processor.java
>>> +++ b/java/org/apache/coyote/http11/Http11Processor.java
>>> @@ -513,7 +513,7 @@ public class Http11Processor extends
>>> AbstractProcessor {
>>>
>>>
>>> private void checkExpectationAndResponseStatus() {
>>> - if (request.hasExpectation() &&
>>> + if (request.hasExpectation() && !isRequestBodyFullyRead() &&
>>> (response.getStatus() < 200 || response.getStatus() >
>>> 299)) {
>>> // Client sent Expect: 100-continue but received a
>>> // non-2xx final response. Disable keep-alive (if enabled)
>>>
>>> Regards,
>>>
>>> Rainer
>>
>> That fix looks right to me.
>>
>> The case we need to avoid is keeping keep-alive enabled when the client
>> and the server have a different view of how much of the request body has
>> been read as that can create request injection issues which are
>> particularly problematic behind a reverse proxy.
>>
>> If the server knows it has read the entire body then the client
>> certainly knows it sent the entire body and all is well.
>
> Soooo, what about in addition drop the non 2xx check. Even if we
> returned a 2xx, shouldn't we close if
>
> request.hasExpectation() && !isRequestBodyFullyRead()
>
> It isn't very likely, that an application returns a 2xx before having
> read the body, but the limitation to non-2xx seems a bit artificial.
There are several moving parts here. There is scope for improvement but
we need to be careful not to introduce a security risk.
> Thinking more about it: what is special here about the expectation case?
> If "!isRequestBodyFullyRead()" make sense as a condition term in these
> places, why bother for limiting it to "request.hasExpectation()"?
> Shouldn't then "!isRequestBodyFullyRead()" be the only check and we
> close the connection for any status code and even when we had no
> expectation? Depending on the detailed behavior of
> isRequestBodyFullyRead() we might need to add a check, if the request
> had a body at all. What do you think?
I don't think the above is quite right. Let me try and explain myself
better.
The non-expectation case is simple. If the client declares it is going
to send a request body it has to send one. Tomcat always knows where one
request ends and the next starts.
What makes things different in the expectation case is that it becomes
optional for the client to send the request body without an easy way for
Tomcat to determine (in some cases) if the client intends to send the
body or not. If Tomcat can't tell if a body is going to be sent it can't
tell if the next byte from the client is the request body or the start
of a new request. The only safe option is to close the connection.
In the expectation + 2xx response case, the client has to send the
request body (if it hasn't already done so) unless Tomcat sends
"Connection: close" so Tomcat always knows what the next byte is and it
is safe to use keep alive.
The expectation + non-2xx response case is where I think we can do
better. If we know the request body has already been sent in full, we
know that the next byte will be the start of the next request and it
will be safe to use keep alive. This is essentially what your original
proposal does and I support it.
If the request body hasn't been read we can't tell if the client decided
to sent it or not and in that case we have to close the connection to
protect against request injection.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
