Bug 63824 - Http11Processor does not compare Connection header value case-insensitively
Summary: Http11Processor does not compare Connection header value case-insensitively
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Connectors (show other bugs)
Version: 8.5.x-trunk
Hardware: All All
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-10-09 21:16 UTC by Michael Osipov
Modified: 2019-10-18 21:18 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Osipov 2019-10-09 21:16:31 UTC
Based on the discussion here: http://mail-archives.apache.org/mod_mbox/tomcat-dev/201910.mbox/%3C6b4dc6fd-7878-ed8d-c84b-410682b6bd03%40apache.org%3E

The comparison must be equalsIgnoreCase(). At best, the method would be changed to #isConnectionValue(MimeHeaders, String) where the expected value is passed as the second arg. This would allow for testing "close" and "keep-alive".
Comment 1 Mark Thomas 2019-10-16 16:10:52 UTC
HTTP header values are case sensitive (HTTP header names are insensitive). Both RFC 2616 and RFC 7230 define the relevant token as "close" so the current case sensitive comparison is correct.

The current code also assumes "close" is the entire header value. My reading of the spec does not support that although I can't think of anything else that would be present.

The worst that could happen is that the "close" token would be added twice which, apart from the extra bandwidth, should not be an issue. I want to look into the performance implications of parsing this "properly" but I am leaning towards WONTFIX at this point.
Comment 2 Michael Osipov 2019-10-16 16:14:40 UTC
https://tools.ietf.org/html/rfc7230#section-6.1 says:


   The Connection header field's value has the following grammar:

     Connection        = 1#connection-option
     connection-option = token

   Connection options are case-insensitive.

As for the substring part:

"close2" token is not "close". As per defition, only one token is allowed, so "close, close2" ist not valid.

I see this as a valid point.
Comment 3 Mark Thomas 2019-10-16 16:26:44 UTC
(In reply to Michael Osipov from comment #2)

>    Connection options are case-insensitive.

Thanks. I missed that statement below the formal grammar.

> As per defition, only one token is allowed,
> so "close, close2" ist not valid.

"1#connection-option" means "1 or more, comma separated" so there is work to do here.

Still need to look into parsing options to see what the performance implications of doing this "properly" are.
Comment 4 Michael Osipov 2019-10-16 19:36:11 UTC
(In reply to Mark Thomas from comment #3)
> (In reply to Michael Osipov from comment #2)
> > As per defition, only one token is allowed,
> > so "close, close2" ist not valid.
> 
> "1#connection-option" means "1 or more, comma separated" so there is work to
> do here.

Arg, thanks. I do have trouble sometimes reading RFC BNF properly.
Comment 5 Mark Thomas 2019-10-17 14:02:50 UTC
Fixed in:
- master for 9.0.28 onwards
- 8.5.x for 8.5.48 onwards
- 7.0.x for 7.0.98 onwards

The performance implications, if any, were in the noise when I measured the performance  with the "proper" parsing. I therefore applied a solution using that approach.
Comment 6 Michael Osipov 2019-10-18 08:48:04 UTC
I am afraid I need to reopen this one because of this missed spot:

https://github.com/apache/tomcat/blob/master/java/org/apache/coyote/http11/Http11Processor.java#L599-L608
Comment 7 Mark Thomas 2019-10-18 21:10:42 UTC
The findBytes() check is case-insensitive (the value is forced to lower case before it is checked).
Comment 8 Michael Osipov 2019-10-18 21:18:38 UTC
Indeed, my bad. Thanks for double-checking! Wouldn't is more reasonble to use isConnectionToken()?