From 4f3bf8b2f7e809ec3523e0129529953fd2a24963 Mon Sep 17 00:00:00 2001 From: Claude Brisson Date: Mon, 20 Apr 2020 18:52:59 +0200 Subject: [PATCH 1/5] Filter invalid HTTP 2.0 headers from response --- .../org/apache/coyote/LocalStrings.properties | 1 + .../apache/coyote/LocalStrings_fr.properties | 1 + java/org/apache/coyote/Response.java | 37 ++++++ .../coyote/http2/TestInvalidHeader.java | 117 ++++++++++++++++++ 4 files changed, 156 insertions(+) create mode 100644 test/org/apache/coyote/http2/TestInvalidHeader.java diff --git a/java/org/apache/coyote/LocalStrings.properties b/java/org/apache/coyote/LocalStrings.properties index 6acb6be07c21..9fdf585092af 100644 --- a/java/org/apache/coyote/LocalStrings.properties +++ b/java/org/apache/coyote/LocalStrings.properties @@ -58,6 +58,7 @@ request.nullReadListener=The listener passed to setReadListener() may not be nul request.readListenerSet=The non-blocking read listener has already been set response.encoding.invalid=The encoding [{0}] is not recognised by the JRE +response.ignoringInvalidHeader=Ignoring invalid header [{0}: {1}] response.noTrailers.notSupported=A trailer fields supplier may not be set for this response. Either the underlying protocol does not support trailer fields or the protocol requires that the supplier is set before the response is committed response.notAsync=It is only valid to switch to non-blocking IO within async processing or HTTP upgrade processing response.notNonBlocking=It is invalid to call isReady() when the response has not been put into non-blocking mode diff --git a/java/org/apache/coyote/LocalStrings_fr.properties b/java/org/apache/coyote/LocalStrings_fr.properties index 30e2465ace3f..eba871ae5a9c 100644 --- a/java/org/apache/coyote/LocalStrings_fr.properties +++ b/java/org/apache/coyote/LocalStrings_fr.properties @@ -57,6 +57,7 @@ request.nullReadListener=L'écouteur passé à setReadListener() ne peut pas êt request.readListenerSet=L'écouteur des lectures non bloquantes a déjà été défini response.encoding.invalid=L''encodage [{0}] n''est pas reconnu par le JRE +response.ignoringInvalidHeader=En-tête invalide ignoré [{0}: {1}] response.noTrailers.notSupported=Un fournisseur d'en-tête de fin ne peut pas être mis sur cette réponse, soit le protocole ne supporte pas ces en-têtes, soit le protocole requiert que le fournisseur soit fourni avant le début de l'envoi de la réponse response.notAsync=Il n'est possible de passer en mode d'entrée-sorties non bloquantes que lors de traitements asynchrones ou après mise à niveau depuis HTTP response.notNonBlocking=Il n'est pas permis d'appeler isReady() quand la réponse n'a pas été mise en mode non-bloquant diff --git a/java/org/apache/coyote/Response.java b/java/org/apache/coyote/Response.java index 91473a1b770e..75f816827275 100644 --- a/java/org/apache/coyote/Response.java +++ b/java/org/apache/coyote/Response.java @@ -30,6 +30,7 @@ import jakarta.servlet.WriteListener; +import org.apache.coyote.http2.Http2OutputBuffer; import org.apache.juli.logging.Log; import org.apache.juli.logging.LogFactory; import org.apache.tomcat.util.buf.B2CConverter; @@ -61,6 +62,11 @@ public final class Response { */ private static final Locale DEFAULT_LOCALE = Locale.getDefault(); + /** + * Helper to log the invalid HTTP/2.0 header error only once per instance + */ + private static AtomicBoolean invalidHeaderWarningEmitted = new AtomicBoolean(false); + // ----------------------------------------------------- Instance Variables @@ -435,6 +441,37 @@ private boolean checkSpecialHeader( String name, String value) { return false; } } + if (outputBuffer instanceof Http2OutputBuffer && name.equalsIgnoreCase("Connection") ) { + + /* + Connection headers are invalid in HTTP/2.0, and some clients (like Safari or curl) + are very touchy about it. Most probably, an application component has added the + typical HTTP/1.x "Connection: keep-alive" header, but despide 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. + We could want to add an HTTP/2.0 option to rather always send/log an exception. + + @see https://tools.ietf.org/html/rfc7540#section-8.1.2.2 + */ + + if (log.isWarnEnabled()) + { + /* log a warning only once per instance *with the stacktrace* */ + if (!invalidHeaderWarningEmitted.getAndSet(true)) + { + try + { + throw new IllegalArgumentException(sm.getString("response.ignoringInvalidHeader", "Connection", value)); + } + catch (IllegalArgumentException iae) + { + log.warn(iae); + } + } + } + return true; + } return false; } diff --git a/test/org/apache/coyote/http2/TestInvalidHeader.java b/test/org/apache/coyote/http2/TestInvalidHeader.java new file mode 100644 index 000000000000..1f2b257f823b --- /dev/null +++ b/test/org/apache/coyote/http2/TestInvalidHeader.java @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.coyote.http2; + +import java.io.IOException; +import java.io.OutputStream; +import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.List; + +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServlet; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; + +import org.junit.Assert; +import org.junit.Test; + +import org.apache.catalina.Context; +import org.apache.catalina.startup.Tomcat; + +public class TestInvalidHeader extends Http2TestBase { + + /* + * @see org.apache.coyote.Response#checkSpecialHeaders() + */ +// @Test +// public void testInvalidHeader() throws Exception { +// +// enableHttp2(); +// configureAndStartWebApplication(); +// +// openClientConnection(); +// doHttpUpgrade(); +// sendClientPreface(); +// validateHttp2InitialResponse(); +// +// byte[] frameHeader = new byte[9]; +// ByteBuffer headersPayload = ByteBuffer.allocate(128); +// List
headers = new ArrayList<>(3); +// headers.add(new Header(":method", "GET")); +// headers.add(new Header(":scheme", "http")); +// headers.add(new Header(":path", "/simple")); +// headers.add(new Header(":authority", "localhost:" + getPort())); +// headers.add(new Header("Connection", "keep-alive")); +// +// buildGetRequest(frameHeader, headersPayload, null, headers, 3); +// +// writeFrame(frameHeader, headersPayload); +// +// readSimpleGetResponse(); +// +// Assert.assertEquals(getSimpleResponseTrace(3), output.getTrace()); +// } + + protected static class FaultyServlet extends SimpleServlet + { + + private static final long serialVersionUID = 1L; + + public static final int CONTENT_LENGTH = 8192; + + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) + throws ServletException, IOException + { + // Add faulty header + resp.addHeader("Connection", "keep-alive"); + super.doGet(req, resp); + } + } + + + @Test + public void testInvalidHeader() throws Exception { + + enableHttp2(); + + Tomcat tomcat = getTomcatInstance(); + + Context ctxt = tomcat.addContext("", null); + Tomcat.addServlet(ctxt, "simple", new FaultyServlet()); + ctxt.addServletMappingDecoded("/simple", "simple"); + + tomcat.start(); + + openClientConnection(); + doHttpUpgrade(); + sendClientPreface(); + validateHttp2InitialResponse(); + + byte[] frameHeader = new byte[9]; + ByteBuffer headersPayload = ByteBuffer.allocate(128); + buildGetRequest(frameHeader, headersPayload, null, 3, "/simple"); + + writeFrame(frameHeader, headersPayload); + + readSimpleGetResponse(); + + Assert.assertEquals(getSimpleResponseTrace(3), output.getTrace()); + } + +} From 8fb2704a5b777a82d2bcd99a5c0316c3d5a7f1e0 Mon Sep 17 00:00:00 2001 From: Claude Brisson Date: Mon, 20 Apr 2020 18:54:45 +0200 Subject: [PATCH 2/5] Code cleaning --- .../coyote/http2/TestInvalidHeader.java | 29 ------------------- 1 file changed, 29 deletions(-) diff --git a/test/org/apache/coyote/http2/TestInvalidHeader.java b/test/org/apache/coyote/http2/TestInvalidHeader.java index 1f2b257f823b..4f592409c20d 100644 --- a/test/org/apache/coyote/http2/TestInvalidHeader.java +++ b/test/org/apache/coyote/http2/TestInvalidHeader.java @@ -38,35 +38,6 @@ public class TestInvalidHeader extends Http2TestBase { /* * @see org.apache.coyote.Response#checkSpecialHeaders() */ -// @Test -// public void testInvalidHeader() throws Exception { -// -// enableHttp2(); -// configureAndStartWebApplication(); -// -// openClientConnection(); -// doHttpUpgrade(); -// sendClientPreface(); -// validateHttp2InitialResponse(); -// -// byte[] frameHeader = new byte[9]; -// ByteBuffer headersPayload = ByteBuffer.allocate(128); -// List
headers = new ArrayList<>(3); -// headers.add(new Header(":method", "GET")); -// headers.add(new Header(":scheme", "http")); -// headers.add(new Header(":path", "/simple")); -// headers.add(new Header(":authority", "localhost:" + getPort())); -// headers.add(new Header("Connection", "keep-alive")); -// -// buildGetRequest(frameHeader, headersPayload, null, headers, 3); -// -// writeFrame(frameHeader, headersPayload); -// -// readSimpleGetResponse(); -// -// Assert.assertEquals(getSimpleResponseTrace(3), output.getTrace()); -// } - protected static class FaultyServlet extends SimpleServlet { From e4175947766c4ae2c5b0c5e5ba1df964bf92cf6b Mon Sep 17 00:00:00 2001 From: Claude Brisson Date: Tue, 21 Apr 2020 13:09:11 +0200 Subject: [PATCH 3/5] Always throw an IllegalArgumentException for invalid HTTP/2.0 headers --- java/org/apache/coyote/Response.java | 43 +++++-------------- .../apache/coyote/http2/Http2TestBase.java | 19 ++++++++ .../coyote/http2/TestInvalidHeader.java | 24 ++--------- 3 files changed, 32 insertions(+), 54 deletions(-) diff --git a/java/org/apache/coyote/Response.java b/java/org/apache/coyote/Response.java index 75f816827275..2cc95f78e80f 100644 --- a/java/org/apache/coyote/Response.java +++ b/java/org/apache/coyote/Response.java @@ -62,12 +62,6 @@ public final class Response { */ private static final Locale DEFAULT_LOCALE = Locale.getDefault(); - /** - * Helper to log the invalid HTTP/2.0 header error only once per instance - */ - private static AtomicBoolean invalidHeaderWarningEmitted = new AtomicBoolean(false); - - // ----------------------------------------------------- Instance Variables /** @@ -441,36 +435,19 @@ private boolean checkSpecialHeader( String name, String value) { return false; } } + if (outputBuffer instanceof Http2OutputBuffer && name.equalsIgnoreCase("Connection") ) { /* - Connection headers are invalid in HTTP/2.0, and some clients (like Safari or curl) - are very touchy about it. Most probably, an application component has added the - typical HTTP/1.x "Connection: keep-alive" header, but despide 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. - We could want to add an HTTP/2.0 option to rather always send/log an exception. - - @see https://tools.ietf.org/html/rfc7540#section-8.1.2.2 - */ - - if (log.isWarnEnabled()) - { - /* log a warning only once per instance *with the stacktrace* */ - if (!invalidHeaderWarningEmitted.getAndSet(true)) - { - try - { - throw new IllegalArgumentException(sm.getString("response.ignoringInvalidHeader", "Connection", value)); - } - catch (IllegalArgumentException iae) - { - log.warn(iae); - } - } - } - return true; + * Connection headers are invalid in HTTP/2.0, and some clients (like Safari or curl) + * are very touchy about it. Most probably, an application component has added the + * typical HTTP/1.x "Connection: keep-alive" header, but despide the component's + * good intention, the header is faulty in HTTP/2.0 and *should* be refused. + * . + * @see https: *tools.ietf.org/html/rfc7540#section-8.1.2.2 + */ + + throw new IllegalArgumentException(sm.getString("response.ignoringInvalidHeader", "Connection", value)); } return false; } diff --git a/test/org/apache/coyote/http2/Http2TestBase.java b/test/org/apache/coyote/http2/Http2TestBase.java index 3f65876c9930..eb486078a219 100644 --- a/test/org/apache/coyote/http2/Http2TestBase.java +++ b/test/org/apache/coyote/http2/Http2TestBase.java @@ -149,6 +149,25 @@ protected void validateHttp2InitialResponse() throws Exception { output.clearTrace(); } + protected void validateHttp2InitialErrorResponse() throws Exception { + // - 101 response acts as acknowledgement of the HTTP2-Settings header + // Need to read 5 frames + // - settings (server settings - must be first) + // - settings ack (for the settings frame in the client preface) + // - ping + // - headers (for response) + // - data (for response body) + parser.readFrame(true); + parser.readFrame(true); + parser.readFrame(true); + parser.readFrame(true); + parser.readFrame(true); + + Assert.assertTrue(output.getTrace().contains("1-Header-[:status]-[500]")); + output.clearTrace(); + } + + protected void sendEmptyGetRequest(int streamId) throws IOException { byte[] frameHeader = new byte[9]; diff --git a/test/org/apache/coyote/http2/TestInvalidHeader.java b/test/org/apache/coyote/http2/TestInvalidHeader.java index 4f592409c20d..8bd10a270529 100644 --- a/test/org/apache/coyote/http2/TestInvalidHeader.java +++ b/test/org/apache/coyote/http2/TestInvalidHeader.java @@ -17,17 +17,11 @@ package org.apache.coyote.http2; import java.io.IOException; -import java.io.OutputStream; -import java.nio.ByteBuffer; -import java.util.ArrayList; -import java.util.List; import jakarta.servlet.ServletException; -import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; -import org.junit.Assert; import org.junit.Test; import org.apache.catalina.Context; @@ -38,8 +32,7 @@ public class TestInvalidHeader extends Http2TestBase { /* * @see org.apache.coyote.Response#checkSpecialHeaders() */ - protected static class FaultyServlet extends SimpleServlet - { + protected static class FaultyServlet extends SimpleServlet { private static final long serialVersionUID = 1L; @@ -47,8 +40,7 @@ protected static class FaultyServlet extends SimpleServlet @Override protected void doGet(HttpServletRequest req, HttpServletResponse resp) - throws ServletException, IOException - { + throws ServletException, IOException { // Add faulty header resp.addHeader("Connection", "keep-alive"); super.doGet(req, resp); @@ -72,17 +64,7 @@ public void testInvalidHeader() throws Exception { openClientConnection(); doHttpUpgrade(); sendClientPreface(); - validateHttp2InitialResponse(); - - byte[] frameHeader = new byte[9]; - ByteBuffer headersPayload = ByteBuffer.allocate(128); - buildGetRequest(frameHeader, headersPayload, null, 3, "/simple"); - - writeFrame(frameHeader, headersPayload); - - readSimpleGetResponse(); - - Assert.assertEquals(getSimpleResponseTrace(3), output.getTrace()); + validateHttp2InitialErrorResponse(); } } From 7c8d0ebb74151eabc29e43e3b4b3e176d6ac36b2 Mon Sep 17 00:00:00 2001 From: Claude Brisson Date: Tue, 21 Apr 2020 14:02:15 +0200 Subject: [PATCH 4/5] Take into account michael-o requested changes --- java/org/apache/coyote/LocalStrings.properties | 2 +- java/org/apache/coyote/LocalStrings_fr.properties | 2 +- java/org/apache/coyote/Response.java | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/java/org/apache/coyote/LocalStrings.properties b/java/org/apache/coyote/LocalStrings.properties index 9fdf585092af..11eb66a2b2fa 100644 --- a/java/org/apache/coyote/LocalStrings.properties +++ b/java/org/apache/coyote/LocalStrings.properties @@ -58,7 +58,7 @@ request.nullReadListener=The listener passed to setReadListener() may not be nul request.readListenerSet=The non-blocking read listener has already been set response.encoding.invalid=The encoding [{0}] is not recognised by the JRE -response.ignoringInvalidHeader=Ignoring invalid header [{0}: {1}] +response.invalidHeader=Invalid header [{0}: {1}] response.noTrailers.notSupported=A trailer fields supplier may not be set for this response. Either the underlying protocol does not support trailer fields or the protocol requires that the supplier is set before the response is committed response.notAsync=It is only valid to switch to non-blocking IO within async processing or HTTP upgrade processing response.notNonBlocking=It is invalid to call isReady() when the response has not been put into non-blocking mode diff --git a/java/org/apache/coyote/LocalStrings_fr.properties b/java/org/apache/coyote/LocalStrings_fr.properties index eba871ae5a9c..1126370f8cb9 100644 --- a/java/org/apache/coyote/LocalStrings_fr.properties +++ b/java/org/apache/coyote/LocalStrings_fr.properties @@ -57,7 +57,7 @@ request.nullReadListener=L'écouteur passé à setReadListener() ne peut pas êt request.readListenerSet=L'écouteur des lectures non bloquantes a déjà été défini response.encoding.invalid=L''encodage [{0}] n''est pas reconnu par le JRE -response.ignoringInvalidHeader=En-tête invalide ignoré [{0}: {1}] +response.invalidHeader=En-tête invalide [{0}: {1}] response.noTrailers.notSupported=Un fournisseur d'en-tête de fin ne peut pas être mis sur cette réponse, soit le protocole ne supporte pas ces en-têtes, soit le protocole requiert que le fournisseur soit fourni avant le début de l'envoi de la réponse response.notAsync=Il n'est possible de passer en mode d'entrée-sorties non bloquantes que lors de traitements asynchrones ou après mise à niveau depuis HTTP response.notNonBlocking=Il n'est pas permis d'appeler isReady() quand la réponse n'a pas été mise en mode non-bloquant diff --git a/java/org/apache/coyote/Response.java b/java/org/apache/coyote/Response.java index 2cc95f78e80f..4affbcde72d1 100644 --- a/java/org/apache/coyote/Response.java +++ b/java/org/apache/coyote/Response.java @@ -441,13 +441,13 @@ private boolean checkSpecialHeader( String name, String value) { /* * Connection headers are invalid in HTTP/2.0, and some clients (like Safari or curl) * are very touchy about it. Most probably, an application component has added the - * typical HTTP/1.x "Connection: keep-alive" header, but despide the component's + * typical HTTP/1.x "Connection: keep-alive" header, but despite the component's * good intention, the header is faulty in HTTP/2.0 and *should* be refused. * . - * @see https: *tools.ietf.org/html/rfc7540#section-8.1.2.2 + * @see https://tools.ietf.org/html/rfc7540#section-8.1.2.2 */ - throw new IllegalArgumentException(sm.getString("response.ignoringInvalidHeader", "Connection", value)); + throw new IllegalArgumentException(sm.getString("response.invalidHeader", "Connection", value)); } return false; } From 45371de75d397c4d86b5a9ef711b3da35da07ce4 Mon Sep 17 00:00:00 2001 From: Claude Brisson Date: Tue, 21 Apr 2020 14:05:01 +0200 Subject: [PATCH 5/5] Fix comment --- java/org/apache/coyote/Response.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/org/apache/coyote/Response.java b/java/org/apache/coyote/Response.java index 4affbcde72d1..85b664c3c540 100644 --- a/java/org/apache/coyote/Response.java +++ b/java/org/apache/coyote/Response.java @@ -439,10 +439,10 @@ private boolean checkSpecialHeader( String name, String value) { if (outputBuffer instanceof Http2OutputBuffer && name.equalsIgnoreCase("Connection") ) { /* - * Connection headers are invalid in HTTP/2.0, and some clients (like Safari or curl) + * Connection headers are invalid in HTTP/2, and some clients (like Safari or curl) * are very touchy about it. Most probably, an application component has added the * typical HTTP/1.x "Connection: keep-alive" header, but despite the component's - * good intention, the header is faulty in HTTP/2.0 and *should* be refused. + * good intention, the header is faulty in HTTP/2 and *should* be refused. * . * @see https://tools.ietf.org/html/rfc7540#section-8.1.2.2 */