[Bug 63824] New: Http11Processor does not compare Connection header value case-insensitively

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 63824] New: Http11Processor does not compare Connection header value case-insensitively

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63824

            Bug ID: 63824
           Summary: Http11Processor does not compare Connection header
                    value case-insensitively
           Product: Tomcat 8
           Version: 8.5.x-trunk
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Connectors
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ----

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".

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63824] Http11Processor does not compare Connection header value case-insensitively

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63824

Michael Osipov <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63824] Http11Processor does not compare Connection header value case-insensitively

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63824

--- Comment #1 from Mark Thomas <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63824] Http11Processor does not compare Connection header value case-insensitively

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63824

--- Comment #2 from Michael Osipov <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63824] Http11Processor does not compare Connection header value case-insensitively

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63824

--- Comment #3 from Mark Thomas <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63824] Http11Processor does not compare Connection header value case-insensitively

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63824

--- Comment #4 from Michael Osipov <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63824] Http11Processor does not compare Connection header value case-insensitively

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63824

Mark Thomas <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #5 from Mark Thomas <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63824] Http11Processor does not compare Connection header value case-insensitively

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63824

Michael Osipov <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|FIXED                       |---

--- Comment #6 from Michael Osipov <[hidden email]> ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63824] Http11Processor does not compare Connection header value case-insensitively

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63824

Mark Thomas <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|REOPENED                    |RESOLVED

--- Comment #7 from Mark Thomas <[hidden email]> ---
The findBytes() check is case-insensitive (the value is forced to lower case
before it is checked).

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63824] Http11Processor does not compare Connection header value case-insensitively

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63824

--- Comment #8 from Michael Osipov <[hidden email]> ---
Indeed, my bad. Thanks for double-checking! Wouldn't is more reasonble to use
isConnectionToken()?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]