Refuse adding invalid HTTP 2.0 headers #277

arkanovicz · 2020-04-20T17:02:28Z

Connection headers like Connection: keep-alive are invalid in HTTP/2, and some clients (like Safari or curl) are very touchy about it.

When an application component adds the typical HTTP/1.x "Connection: keep-alive" header to the response, despite the component's good intention, the header is faulty in HTTP/2.0 and SHOULD always be filtered.

The current implementation emits a warning in the logs only once per instance.

michael-o · 2020-04-20T17:08:51Z

Can you show your sample curl input/output?

arkanovicz · 2020-04-20T17:54:01Z

@michael-o curl example:

$ curl --http2 -Nv "https://some.http2.url/"
*   Trying 127.0.0.1:8833...
* TCP_NODELAY set
* Connected to some.http2.url (127.0.0.1) port 8833 (#0)
> GET / HTTP/1.1
> Host: some.http2.url:8833
> User-Agent: curl/7.65.3
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
> Accept:text/plain
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 101 
< Connection: Upgrade
< Upgrade: h2c
< Date: Mon, 20 Apr 2020 17:48:26 GMT
* Received 101
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=15
* Connection state changed (MAX_CONCURRENT_STREAMS == 100)!
* http2 error: Invalid HTTP header field was received: frame type: 1, stream: 1, name: [connection], value: [keep-alive]
* HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)
* stopped the pause stream!
* Connection #0 to host some.http2.url left intact
curl: (92) HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)

Safari will display the following error in the javascript console:

[Error] Failed to load resource: The operation couldnt be completed. Protocol error

arkanovicz · 2020-04-20T17:55:19Z

Of course, you will need to actually add the faulty header to the response to see this error.

markt-asf · 2020-04-20T18:55:32Z

I'm curious why applications think they need to set the connection header. I'd expect the container to handle this. Further, applications that want to set this header can/should use ServletRequest.getProtocol() first to check it is appropriate to do so.

If applied (and I'm not convinced it should be) the formatting of the PR needs fixing to be consistent with the Tomcat code. I'd also consider using UserDataHelper for the logging even though this isn't strictly a user data issue.

rmaucher · 2020-04-20T18:57:08Z

-1
Tomcat covers the most egregious cases, but this is probably not one of them. Even in HTTP/1.1, what the application does setting this connection header is nonsense and will never help. You will need to fix the application.

michael-o · 2020-04-20T19:14:14Z

I fail to see this issue:

Pure:

$ curl --http2 -NvIsq http://localhost:8080
HTTP/1.1 101
Connection: Upgrade
Upgrade: h2c
Date: Mon, 20 Apr 2020 19:10:48 GMT

HTTP/2 200
content-type: text/html;charset=UTF-8
date: Mon, 20 Apr 2020 19:10:48 GMT

*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> HEAD / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.63.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
>
< HTTP/1.1 101
< Connection: Upgrade
< Upgrade: h2c
< Date: Mon, 20 Apr 2020 19:10:48 GMT
* Received 101
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Connection state changed (MAX_CONCURRENT_STREAMS == 100)!
< HTTP/2 200
< content-type: text/html;charset=UTF-8
< date: Mon, 20 Apr 2020 19:10:48 GMT
<
* Connection #0 to host localhost left intact

with explicit headers:

$ curl --http2 -NvIsq http://localhost:8080 -H "Connection: keep-alive"
HTTP/1.1 101
Connection: Upgrade, keep-alive
Upgrade: h2c
Date: Mon, 20 Apr 2020 19:11:45 GMT
Keep-Alive: timeout=20

HTTP/2 200
content-type: text/html;charset=UTF-8
date: Mon, 20 Apr 2020 19:11:45 GMT

*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> HEAD / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.63.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
> Connection: keep-alive
>
< HTTP/1.1 101
< Connection: Upgrade, keep-alive
< Upgrade: h2c
< Date: Mon, 20 Apr 2020 19:11:45 GMT
< Keep-Alive: timeout=20
* Received 101
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Connection state changed (MAX_CONCURRENT_STREAMS == 100)!
< HTTP/2 200
< content-type: text/html;charset=UTF-8
< date: Mon, 20 Apr 2020 19:11:45 GMT
<
* Connection #0 to host localhost left intact

Running Tomcat 9.0.34 and

$ curl --version
curl 7.63.0 (x86_64-w64-mingw32) libcurl/7.63.0 OpenSSL/1.1.1a (WinSSL) zlib/1.2.11 libidn2/2.0.5 nghttp2/1.35.1
Release-Date: 2018-12-12
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smtp smtps telnet tftp
Features: AsynchDNS IDN IPv6 Largefile SSPI Kerberos SPNEGO NTLM SSL libz TLS-SRP HTTP2 HTTPS-proxy MultiSSL Metalink

Even with

C:\Users\mosipov>curl --version
curl 7.69.1 (x86_64-pc-win32) libcurl/7.69.1 OpenSSL/1.1.1f (Schannel) zlib/1.2.11 brotli/1.0.7 WinIDN libssh2/1.9.0 nghttp2/1.40.0
Release-Date: 2020-03-11
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: AsynchDNS HTTP2 HTTPS-proxy IDN IPv6 Kerberos Largefile MultiSSL NTLM SPNEGO SSL SSPI TLS-SRP brotli libz

Please ugrade your curl first!

rmaucher · 2020-04-20T19:33:51Z

Since it's now 2020, shouldn't it be doable to block any attempt to set the connection header by the application ?

markt-asf · 2020-04-20T19:41:09Z

That would also allow some clean up in the current code that sets the header and has to take account of any value that may have been set by the application.

arkanovicz · 2020-04-20T19:49:11Z

@markt-asf This happens for instance in SSE components (tomcat does not provide such components), or more generally in any J2EE filters or servlets, coded before HTTP/2.0, which want to ensure that the connection is kept alive.

@rmaucher It means that you choose tomcat to let buggy server-side components break HTTP/2.0 streams. Other user agents do always ignore this faulty header, so what's the point in sending it in the first place? Helping to find bad client code by breaking more often? Of course, it's a strategy, but considering the vast amount of HTTP/1.1-specific code in the nature that is supposed to migrate to HTTP/2.0, it's better to just log an informative warning and get things straight. Or directly throw.

@michael-o I'm using a recent curl. There is a misunderstanding, here, I'm talking about a header sent by the server in the HTTP response.

Since it's now 2020, shouldn't it be doable to block any attempt to set the connection header by the application ?
That would also allow some clean up in the current code that sets the header and has to take account of any value that may have been set by the application.

@rmaucher That would mean to throw the exception right away, it's enough to remove the try catch in this patch. Yes, certainly simpler.

If applied (and I'm not convinced it should be) the formatting of the PR needs fixing to be consistent with the Tomcat code.

@markt-asf Can you give me some pointers on the needed fixes?

michael-o · 2020-04-20T20:27:33Z

Why can't I reproduce it although I have an h2c response?!

michael-o · 2020-04-20T20:36:44Z

We are talking about this code block:

tomcat/java/org/apache/coyote/http11/Http11Processor.java

Lines 932 to 956 in 65bc61a

    
           if (protocol.getUseKeepAliveResponseHeader()) { 
        
               boolean connectionKeepAlivePresent = 
        
                   isConnectionToken(request.getMimeHeaders(), Constants.KEEP_ALIVE_HEADER_VALUE_TOKEN); 
        
               if (connectionKeepAlivePresent) { 
        
                   int keepAliveTimeout = protocol.getKeepAliveTimeout(); 
        
                   if (keepAliveTimeout > 0) { 
        
                       String value = "timeout=" + keepAliveTimeout / 1000L; 
        
                       headers.setValue(Constants.KEEP_ALIVE_HEADER_NAME).setString(value); 
        
                       if (http11) { 
        
                           // Append if there is already a Connection header, 
        
                           // else create the header 
        
                           MessageBytes connectionHeaderValue = headers.getValue(Constants.CONNECTION); 
        
                           if (connectionHeaderValue == null) { 
        
                               headers.addValue(Constants.CONNECTION).setString(Constants.KEEP_ALIVE_HEADER_VALUE_TOKEN); 
        
                           } else { 
        
                               connectionHeaderValue.setString( 
        
                                       connectionHeaderValue.getString() + ", " + Constants.KEEP_ALIVE_HEADER_VALUE_TOKEN); 
        
                           } 
        
                       } 
        
                   } 
        
               } 
        
           }

arkanovicz · 2020-04-20T22:46:09Z

I see. But at that point, you do not know who set up the header, so the stack trace won't be informative.

michael-o · 2020-04-20T22:52:55Z

I see. But at that point, you do not know who set up the header, so the stack trace won't be informative.

Can you retry with a vanilla Tomcat and the most recent curl?

arkanovicz · 2020-04-21T01:35:32Z

Can you retry with a vanilla Tomcat and the most recent curl?

I've spent enough time digging for this bug (to end up finding it in a SSE dependency), including step by step debugging in 9.0.33 sources (then checking it is still present with 9.0.34) and in the specs and the browser sources to not really need to retry anything to persuade myself of the pertinenece of this patch.

markt-asf · 2020-04-21T06:56:13Z

@arkanovicz
You can ignore requests to retest this. I was able to recreate the issue with curl using the steps you describe.
Could you expand the SSE acronym please. I want to make sure I understand you correctly.
Code format issues are opening braces { should not be on a new line and multi-line comments either use // before every line or, if the /* ... */ style is used each intermediate line starts with an aligned *

More generally...

It would be worth reviewing the HTTP/2 spec to check if there are any other headers that are invalid for HTTP/2.

The global blocking off applications setting Connection headers seems reasonable at first consideration but needs more thought/review in case there are use cases where it is arguably valid / necessary to do so.

martin-g · 2020-04-21T08:11:03Z

java/org/apache/coyote/Response.java

+    /**
+     * Helper to log the invalid HTTP/2.0 header error only once per instance
+     */
+    private static AtomicBoolean invalidHeaderWarningEmitted = new AtomicBoolean(false);


per instance of what ? Because you use static here, so it is on class loader level.

@martin-g It's a langage abuse. Per instance of running tomcat, sorry. Anyhow, @rmaucher suggestion to just throw the exception is simpler, so the AtomicBoolean is deprecated.

arkanovicz · 2020-04-21T09:48:38Z

Could you expand the SSE acronym please. I want to make sure I understand you correctly.

Server-Side Events

Code format issues are opening braces { should not be on a new line and multi-line comments either use // before every line or, if the /* ... */ style is used each intermediate line starts with an aligned *

Ok, noted.

More generally...

It would be worth reviewing the HTTP/2 spec to check if there are any other headers that are invalid for HTTP/2.

To my knowledge, only the Connection headers.

The global blocking off applications setting Connection headers seems reasonable at first consideration but needs more thought/review in case there are use cases where it is arguably valid / necessary to do so.

At best, if the faulty header doesn't provoke an error (required by the specs), it will be ignored. Here's what the spec says:

Intermediaries that process HTTP requests or responses (i.e., any intermediary not acting as a tunnel) MUST NOT forward a malformed request or response. Malformed requests or responses that are detected MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.

For malformed requests, a server MAY send an HTTP response prior to closing or resetting the stream. Clients MUST NOT accept a malformed response. Note that these requirements are intended to protect against several types of common attacks against HTTP; they are deliberately strict because being permissive can expose implementations to these vulnerabilities.

michael-o

The exception message does not correspond to the behavior. The header isn't ignored, but an exception is signaled. Isn't that a contradiction?

java/org/apache/coyote/Response.java

rmaucher · 2020-04-21T15:07:30Z

I will maintain my -1
For starters, any such HTTP/2 specific nonsense safety nets need to be added to StreamProcessor.prepareHeaders instead of other random locations.

rmaucher · 2020-04-22T09:22:06Z

The initial post says SHOULD, but after actually checking the spec it is a MUST.
https://tools.ietf.org/html/rfc7540#section-8.1.2.2

It is really odd the specification made such a choice given how many applications toy with that header in an attempt to do something useful, and given they have no idea if they're using HTTP/1.1 or HTTP/2. So this looks to me like a spec mistake [for compatibility, they should have said it has to be ignored and can be freely removed, whatever], and it is not very surprising most HTTP/2 clients would not check this requirement.

arkanovicz · 2020-04-22T12:37:10Z

@rmaucher The spec authors justify themselves of this strict policy by saying that otherwise it's a vector of attack. I don't know the whereabouts, here.

About where to put the filtering, in Response.addHeader() or StreamProcessor.prepareHeaders(), I understand your point of view (which is, I suppose, to gather all protocol stuff together) but targeting StreamProcessor doesn't respect the Fail Fast paradigm, which prevails IMHO.

rmaucher · 2020-04-22T12:48:10Z

Still, -1, again for your patch. In addition to being ugly, there's no provision in the Servlet spec to throw an exception on random header names, especially common ones, so failing, fast or slow, is wrong.

markt-asf · 2020-04-23T15:11:25Z

I've been reading the HTTP/2 RFC and there is more to this than simply blocking the connection header.

What the HTTP/2 and HTTP/1.1 specs suggest we should be doing in parsing an attempt to set the connection header and then blocking that header and and connection level headers it specifies whether set previously or not.
There is the general question of whether we should be targeting just HTTP/2 or whether we should be preventing applications doing this regardless of protocol.

We need to figure out what we actually want to do first.

I'm currently leaning towards introducing logging of attempts to set connection level headers with a warning that a future version will block the attempt. Probably with UserDataHelper to keep log volumes down even though this isn't really user data.

rmaucher · 2020-04-23T15:37:04Z

Yes, it is accurate if there's a "connection: foobar" header, then there could be a "foobar" header and in that case it's tied to the connection header.

Note about my earlier "proposal" for HTTP/1.1, the connection header is used by the websocket.server.UpgradeUtil helper class to allow upgrade through the API. So it's not possible to filter it and be done, this would have to be fixed as well [not sure how]. Servlet apps can upgrade through the proper API and would not be affected.

michael-o · 2020-04-23T16:36:23Z

Please note that there may be client components which may explicitly close connections with Connection: close. E.g., in SpnegoAuthenticator.

rmaucher · 2020-04-24T13:57:19Z

Indeed, there are two very legacy looking workarounds for brokeness in spnego. One of them can be removed since using Java 8u40 is not reasonable anymore. What about that connection close one ? IMO it's useless since there is no default, so an admin would have to figure out whatever broken clients are out there, then populate it. Not reasonable. If the feature is useful, then it should have a default with all the known broken clients.

michael-o · 2020-04-24T14:24:16Z

@rmaucher The question is when is it justified for component or webapp code to close the connection? Should a container solely decide to close the connection?

markt-asf · 2020-04-24T17:28:41Z

In an ideal world, anything in the Connection header should only be the concern of the container. Historically that hasn't been possible due to broken clients. The question is, are we at the point where it would be possible? Logging a warning when an application attempts this is one way to help us find out. Users will complain about the log message. Whether they are able to fix the app and/or client in question will tell us how safe it would be for us to move to a ban.

pirateskipper · 2021-08-23T15:26:33Z

java/org/apache/coyote/Response.java

@@ -435,6 +435,20 @@ private boolean checkSpecialHeader( String name, String value) {
                return false;
            }
        }
+
+        if (outputBuffer instanceof Http2OutputBuffer && name.equalsIgnoreCase("Connection") ) {


if (outputBuffer instanceof Http2OutputBuffer && "Connection".equalsIgnoreCase(name) ) {

arkanovicz added 2 commits April 20, 2020 18:52

Filter invalid HTTP 2.0 headers from response

4f3bf8b

Code cleaning

8fb2704

martin-g reviewed Apr 21, 2020

View reviewed changes

Always throw an IllegalArgumentException for invalid HTTP/2.0 headers

e417594

michael-o requested changes Apr 21, 2020

View reviewed changes

java/org/apache/coyote/Response.java Outdated Show resolved Hide resolved

java/org/apache/coyote/Response.java Outdated Show resolved Hide resolved

java/org/apache/coyote/Response.java Outdated Show resolved Hide resolved

arkanovicz added 2 commits April 21, 2020 14:02

Take into account michael-o requested changes

7c8d0eb

Fix comment

45371de

arkanovicz changed the title ~~Filter invalid HTTP 2.0 headers from response~~ Refuse adding invalid HTTP 2.0 headers Apr 21, 2020

bclozel mentioned this pull request Apr 14, 2021

Spring adding 'chunked' transfer encoding even if this header already exists [SPR-16985] spring-projects/spring-framework#21523

Closed

pirateskipper reviewed Aug 23, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refuse adding invalid HTTP 2.0 headers #277

Refuse adding invalid HTTP 2.0 headers #277

arkanovicz commented Apr 20, 2020 •

edited by michael-o

michael-o commented Apr 20, 2020

arkanovicz commented Apr 20, 2020

arkanovicz commented Apr 20, 2020

markt-asf commented Apr 20, 2020

rmaucher commented Apr 20, 2020

michael-o commented Apr 20, 2020 •

edited

rmaucher commented Apr 20, 2020

markt-asf commented Apr 20, 2020

arkanovicz commented Apr 20, 2020

michael-o commented Apr 20, 2020 •

edited

michael-o commented Apr 20, 2020

arkanovicz commented Apr 20, 2020

michael-o commented Apr 20, 2020 •

edited

arkanovicz commented Apr 21, 2020

markt-asf commented Apr 21, 2020

martin-g Apr 21, 2020

arkanovicz Apr 21, 2020

arkanovicz commented Apr 21, 2020

michael-o left a comment

rmaucher commented Apr 21, 2020

rmaucher commented Apr 22, 2020

arkanovicz commented Apr 22, 2020

rmaucher commented Apr 22, 2020

markt-asf commented Apr 23, 2020

rmaucher commented Apr 23, 2020

michael-o commented Apr 23, 2020

rmaucher commented Apr 24, 2020

michael-o commented Apr 24, 2020

markt-asf commented Apr 24, 2020

pirateskipper Aug 23, 2021

Refuse adding invalid HTTP 2.0 headers #277

Are you sure you want to change the base?

Refuse adding invalid HTTP 2.0 headers #277

Conversation

arkanovicz commented Apr 20, 2020 • edited by michael-o

michael-o commented Apr 20, 2020

arkanovicz commented Apr 20, 2020

arkanovicz commented Apr 20, 2020

markt-asf commented Apr 20, 2020

rmaucher commented Apr 20, 2020

michael-o commented Apr 20, 2020 • edited

rmaucher commented Apr 20, 2020

markt-asf commented Apr 20, 2020

arkanovicz commented Apr 20, 2020

michael-o commented Apr 20, 2020 • edited

michael-o commented Apr 20, 2020

arkanovicz commented Apr 20, 2020

michael-o commented Apr 20, 2020 • edited

arkanovicz commented Apr 21, 2020

markt-asf commented Apr 21, 2020

martin-g Apr 21, 2020

Choose a reason for hiding this comment

arkanovicz Apr 21, 2020

Choose a reason for hiding this comment

arkanovicz commented Apr 21, 2020

michael-o left a comment

Choose a reason for hiding this comment

rmaucher commented Apr 21, 2020

rmaucher commented Apr 22, 2020

arkanovicz commented Apr 22, 2020

rmaucher commented Apr 22, 2020

markt-asf commented Apr 23, 2020

rmaucher commented Apr 23, 2020

michael-o commented Apr 23, 2020

rmaucher commented Apr 24, 2020

michael-o commented Apr 24, 2020

markt-asf commented Apr 24, 2020

pirateskipper Aug 23, 2021

Choose a reason for hiding this comment

arkanovicz commented Apr 20, 2020 •

edited by michael-o

michael-o commented Apr 20, 2020 •

edited

michael-o commented Apr 20, 2020 •

edited

michael-o commented Apr 20, 2020 •

edited