From 6b17ccda54672e9ead7f8b37757b6bc129c162f0 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Tue, 18 Oct 2022 22:49:30 -0700 Subject: [PATCH] H2 TE header parsing bug fix (#2398) Motivation: HTTP/2 has special rules that TE header is only allowed to have a value of `trailers`. The sanitizing of the TE headers may have infinite looped in some scenarios. --- servicetalk-buffer-api/build.gradle | 3 + .../io/servicetalk/buffer/api/Matchers.java | 42 ++++++++- .../servicetalk/http/netty/H2ToStH1Utils.java | 38 ++++++--- .../H2PriorKnowledgeFeatureParityTest.java | 85 ++++++++++++++++++- 4 files changed, 155 insertions(+), 13 deletions(-) diff --git a/servicetalk-buffer-api/build.gradle b/servicetalk-buffer-api/build.gradle index b2c636ff8a..72f859de46 100644 --- a/servicetalk-buffer-api/build.gradle +++ b/servicetalk-buffer-api/build.gradle @@ -29,5 +29,8 @@ dependencies { testImplementation "org.hamcrest:hamcrest:$hamcrestVersion" testImplementation "org.mockito:mockito-core:$mockitoCoreVersion" + testFixturesImplementation platform(project(":servicetalk-dependencies")) + testFixturesImplementation project(":servicetalk-annotations") + testFixturesImplementation "com.google.code.findbugs:jsr305" testFixturesImplementation "org.hamcrest:hamcrest:$hamcrestVersion" } diff --git a/servicetalk-buffer-api/src/testFixtures/java/io/servicetalk/buffer/api/Matchers.java b/servicetalk-buffer-api/src/testFixtures/java/io/servicetalk/buffer/api/Matchers.java index c95ac08197..967988ec6a 100644 --- a/servicetalk-buffer-api/src/testFixtures/java/io/servicetalk/buffer/api/Matchers.java +++ b/servicetalk-buffer-api/src/testFixtures/java/io/servicetalk/buffer/api/Matchers.java @@ -15,12 +15,19 @@ */ package io.servicetalk.buffer.api; +import org.hamcrest.BaseMatcher; import org.hamcrest.Description; import org.hamcrest.Matcher; import org.hamcrest.TypeSafeMatcher; +import java.util.ArrayList; +import java.util.List; +import javax.annotation.Nullable; + import static io.servicetalk.buffer.api.CharSequences.contentEquals; +import static io.servicetalk.buffer.api.CharSequences.contentEqualsIgnoreCase; import static java.util.Objects.requireNonNull; +import static org.hamcrest.collection.IsIterableContainingInOrder.contains; /** * Custom {@link Matcher}s specific to http-api. @@ -40,7 +47,6 @@ private Matchers() { public static Matcher contentEqualTo(final CharSequence expected) { requireNonNull(expected); return new TypeSafeMatcher() { - @Override protected boolean matchesSafely(final CharSequence item) { return contentEquals(expected, item); @@ -52,4 +58,38 @@ public void describeTo(final Description description) { } }; } + + /** + * Matcher which compares in order while ignoring case. + * @param items The expected items. + * @param {@link CharSequence} type to match. + * @return Matcher which compares in order while ignoring case. + */ + @SafeVarargs + public static Matcher> containsIgnoreCase(E... items) { + final List> matchers = new ArrayList<>(items.length); + for (E item : items) { + matchers.add(new CharsEqualsIgnoreCase(item)); + } + return contains(matchers); + } + + private static final class CharsEqualsIgnoreCase extends BaseMatcher { + @Nullable + private final CharSequence expected; + + private CharsEqualsIgnoreCase(final CharSequence expected) { + this.expected = expected; + } + + @Override + public boolean matches(final Object actual) { + return actual instanceof CharSequence && contentEqualsIgnoreCase(expected, (CharSequence) actual); + } + + @Override + public void describeTo(final Description description) { + description.appendValue(expected); + } + } } diff --git a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ToStH1Utils.java b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ToStH1Utils.java index 0602693cf9..9b8aaa4874 100644 --- a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ToStH1Utils.java +++ b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ToStH1Utils.java @@ -28,6 +28,7 @@ import static io.netty.handler.codec.http.HttpHeaderNames.TE; import static io.netty.handler.codec.http.HttpHeaderValues.TRAILERS; +import static io.servicetalk.buffer.api.CharSequences.contentEqualsIgnoreCase; import static io.servicetalk.buffer.api.CharSequences.newAsciiString; import static io.servicetalk.http.api.HttpHeaderNames.CONNECTION; import static io.servicetalk.http.api.HttpHeaderNames.COOKIE; @@ -185,19 +186,34 @@ static Http2Headers h1HeadersToH2Headers(HttpHeaders h1Headers) { Iterator teItr = h1Headers.valuesIterator(TE); boolean addTrailers = false; while (teItr.hasNext()) { - String teValue = teItr.next().toString(); - int i = teValue.indexOf(','); - if (i != -1) { - int start = 0; - do { - if (teValue.substring(start, i).compareToIgnoreCase(TRAILERS.toString()) == 0) { + final CharSequence teSequence = teItr.next(); + if (addTrailers) { + teItr.remove(); + } else { + int i = indexOf(teSequence, ',', 0); + if (i != -1) { + int start = 0; + do { + if (contentEqualsIgnoreCase(teSequence.subSequence(start, i), TRAILERS)) { + addTrailers = true; + break; + } + start = i + 1; + // Check if we need to skip OWS + // https://www.rfc-editor.org/rfc/rfc9110.html#section-10.1.4 + if (start < teSequence.length() && teSequence.charAt(start) == ' ') { + ++start; + } + } while (start < teSequence.length() && (i = indexOf(teSequence, ',', start)) != -1); + + if (!addTrailers && start < teSequence.length() && + contentEqualsIgnoreCase(teSequence.subSequence(start, teSequence.length()), TRAILERS)) { addTrailers = true; - break; } - } while (start < teValue.length() && (i = teValue.indexOf(',', start)) != -1); - teItr.remove(); - } else if (teValue.compareToIgnoreCase(TRAILERS.toString()) != 0) { - teItr.remove(); + teItr.remove(); + } else if (!contentEqualsIgnoreCase(teSequence, TRAILERS)) { + teItr.remove(); + } } } if (addTrailers) { // add after iteration to avoid concurrent modification. diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/H2PriorKnowledgeFeatureParityTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/H2PriorKnowledgeFeatureParityTest.java index 0691ae3bd1..c12c230050 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/H2PriorKnowledgeFeatureParityTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/H2PriorKnowledgeFeatureParityTest.java @@ -35,7 +35,9 @@ import io.servicetalk.http.api.HttpEventKey; import io.servicetalk.http.api.HttpExecutionStrategy; import io.servicetalk.http.api.HttpHeaders; +import io.servicetalk.http.api.HttpHeadersFactory; import io.servicetalk.http.api.HttpMetaData; +import io.servicetalk.http.api.HttpProtocolConfig; import io.servicetalk.http.api.HttpRequest; import io.servicetalk.http.api.HttpRequestMethod; import io.servicetalk.http.api.HttpResponse; @@ -123,9 +125,11 @@ import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TYPE; import static io.netty.handler.codec.http.HttpHeaderNames.TRAILER; +import static io.netty.handler.codec.http.HttpHeaderValues.TRAILERS; import static io.netty.handler.codec.http2.Http2CodecUtil.SMALLEST_MAX_CONCURRENT_STREAMS; import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR; import static io.servicetalk.buffer.api.EmptyBuffer.EMPTY_BUFFER; +import static io.servicetalk.buffer.api.Matchers.containsIgnoreCase; import static io.servicetalk.buffer.api.Matchers.contentEqualTo; import static io.servicetalk.concurrent.api.Completable.completed; import static io.servicetalk.concurrent.api.Processors.newPublisherProcessor; @@ -140,6 +144,7 @@ import static io.servicetalk.http.api.HttpHeaderNames.COOKIE; import static io.servicetalk.http.api.HttpHeaderNames.EXPECT; import static io.servicetalk.http.api.HttpHeaderNames.SET_COOKIE; +import static io.servicetalk.http.api.HttpHeaderNames.TE; import static io.servicetalk.http.api.HttpHeaderNames.TRANSFER_ENCODING; import static io.servicetalk.http.api.HttpHeaderNames.UPGRADE; import static io.servicetalk.http.api.HttpHeaderValues.CHUNKED; @@ -389,6 +394,70 @@ void cookiesRoundTrip(HttpTestExecutionStrategy strategy, boolean h2PriorKnowled } } + @ParameterizedTest(name = "{displayName} [{index}] client={0}, h2PriorKnowledge={1}") + @MethodSource("clientExecutors") + void teHeaderOnlyAllowsTrailers(HttpTestExecutionStrategy strategy, boolean h2PriorKnowledge) throws Exception { + setUp(strategy, h2PriorKnowledge); + // Newer versions of Netty validate at addition time so for using ServiceTalk headers, so we can add invalid + // headers and assert that ServiceTalk filters them. + final HttpHeadersFactory headersFactory = DefaultHttpHeadersFactory.INSTANCE; + InetSocketAddress serverAddress = bindHttpEchoServer(null, null, headersFactory); + try (BlockingHttpClient client = forSingleAddress(HostAndPort.of(serverAddress)) + .protocols(applyHeadersFactory(h2PriorKnowledge, headersFactory)) + .executionStrategy(clientExecutionStrategy).buildBlocking()) { + // Test individual headers + HttpRequest request = client.get("/"); + request.addHeader(TE, "foo"); + request.addHeader(TE, TRAILERS); + request.addHeader(TE, "bar"); + HttpResponse response = client.request(request); + assertThat(response.headers().values(TE), h2PriorKnowledge ? + containsIgnoreCase(TRAILERS) : containsIgnoreCase("foo", TRAILERS, "bar")); + + // Test single header value, comma separated trailers last + request = client.get("/"); + request.addHeader(TE, "foo," + TRAILERS); + response = client.request(request); + assertThat(response.headers().values(TE), h2PriorKnowledge ? + containsIgnoreCase(TRAILERS) : containsIgnoreCase("foo," + TRAILERS)); + + // Test single header value, comma separated trailers last with OWS + request = client.get("/"); + request.addHeader(TE, "foo, " + TRAILERS); + response = client.request(request); + assertThat(response.headers().values(TE), h2PriorKnowledge ? + containsIgnoreCase(TRAILERS) : containsIgnoreCase("foo, " + TRAILERS)); + + // Test single header value, comma separated trailers first + request = client.get("/"); + request.addHeader(TE, TRAILERS + ",foo"); + response = client.request(request); + assertThat(response.headers().values(TE), h2PriorKnowledge ? + containsIgnoreCase(TRAILERS) : containsIgnoreCase(TRAILERS + ",foo")); + + // Test single header value, comma separated trailers first with OWS + request = client.get("/"); + request.addHeader(TE, TRAILERS + ", foo"); + response = client.request(request); + assertThat(response.headers().values(TE), h2PriorKnowledge ? + containsIgnoreCase(TRAILERS) : containsIgnoreCase(TRAILERS + ", foo")); + + // Test single header value, comma separated trailers middle + request = client.get("/"); + request.addHeader(TE, "foo," + TRAILERS + ",bar"); + response = client.request(request); + assertThat(response.headers().values(TE), h2PriorKnowledge ? + containsIgnoreCase(TRAILERS) : containsIgnoreCase("foo," + TRAILERS + ",bar")); + + // Test single header value, comma separated trailers middle with OWS + request = client.get("/"); + request.addHeader(TE, "foo, " + TRAILERS + ", bar"); + response = client.request(request); + assertThat(response.headers().values(TE), h2PriorKnowledge ? + containsIgnoreCase(TRAILERS) : containsIgnoreCase("foo, " + TRAILERS + ", bar")); + } + } + @ParameterizedTest(name = "{displayName} [{index}] client={0}, h2PriorKnowledge={1}") @MethodSource("clientExecutors") void serverAllowDropTrailers(HttpTestExecutionStrategy strategy, boolean h2PriorKnowledge) throws Exception { @@ -1735,8 +1804,21 @@ private InetSocketAddress bindHttpEchoServer() throws Exception { private InetSocketAddress bindHttpEchoServer(@Nullable StreamingHttpServiceFilterFactory filterFactory, @Nullable CountDownLatch connectionOnClosingLatch) throws Exception { + return bindHttpEchoServer(filterFactory, connectionOnClosingLatch, null); + } + + private static HttpProtocolConfig applyHeadersFactory(boolean h2PriorKnowledge, + @Nullable HttpHeadersFactory headersFactory) { + return h2PriorKnowledge ? + headersFactory == null ? h2Default() : h2().headersFactory(headersFactory).build() : + headersFactory == null ? h1Default() : h1().headersFactory(headersFactory).build(); + } + + private InetSocketAddress bindHttpEchoServer(@Nullable StreamingHttpServiceFilterFactory filterFactory, + @Nullable CountDownLatch connectionOnClosingLatch, + @Nullable HttpHeadersFactory headersFactory) throws Exception { HttpServerBuilder serverBuilder = HttpServers.forAddress(localAddress(0)) - .protocols(h2PriorKnowledge ? h2Default() : h1Default()); + .protocols(applyHeadersFactory(h2PriorKnowledge, headersFactory)); if (filterFactory != null) { serverBuilder.appendServiceFilter(filterFactory); } @@ -1779,6 +1861,7 @@ public Completable accept(final ConnectionContext context) { resp.setHeader(TRANSFER_ENCODING, transferEncoding); } resp.headers().set(COOKIE, request.headers().valuesIterator(COOKIE)); + resp.headers().set(TE, request.headers().valuesIterator(TE)); return succeeded(resp); }).toFuture().get(); return (InetSocketAddress) h1ServerContext.listenAddress();