Repository: netty/netty
Issue: HTTP/2 binary headers requires additional validation for HTTP translation layer
Labels: defect

As a result of issue #2786 the HTTP translation layer should be doing additional validation.

From @nmittler 

> Just a note on the impact of this on the translation to HTTP/1.1 ... Section 10.3 of the spec touches on this: http://tools.ietf.org/html/draft-ietf-httpbis-http2-14#section-10.3
> So it sounds like requests with non-ascii headers should probably be rejected by the translation layer (i.e. RST_STREAM). Sound reasonable?


Issue comments:
`DefaultHttpHeaders` already implements the ascii checks (if the `validate` mode is turned on...which is exposed in the HTTP2 translation layer).  I think all that needs to be done here is catch the exception that is thrown by this validation (`IllegalArgumentException`) when creating the HTTP headers object and re-throw as an `Http2Exception`.

@Scottmitch sounds good!

@nmittler - I think with the life cycle manager changes this exception should bubble up to `decode` and be caught here https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L372.  I think the first order of business is to add a unit test to verify this but functionally I think we may be covered.

@Scottmitch sounds good!

@nmittler - Turns out the exception didn't make it all the way up after all.  See https://github.com/netty/netty/pull/3090.

Cherry picked


Pull request review comments:
Maybe you could use this utility method to help out here: https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java#L134

Good idea...done.


Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpUtil.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpUtil.java
index 7572fe46ebd..0dddd8b881f 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpUtil.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpUtil.java
@@ -14,6 +14,7 @@
  */
 package io.netty.handler.codec.http2;
 
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
 import io.netty.handler.codec.AsciiString;
 import io.netty.handler.codec.BinaryHeaders;
 import io.netty.handler.codec.TextHeaders.EntryVisitor;
@@ -210,8 +211,12 @@ public static FullHttpRequest toHttpRequest(int streamId, Http2Headers http2Head
                     throws Http2Exception {
         // HTTP/2 does not define a way to carry the version identifier that is
         // included in the HTTP/1.1 request line.
-        FullHttpRequest msg = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.valueOf(http2Headers.method()
-                        .toString()), http2Headers.path().toString(), validateHttpHeaders);
+        final AsciiString method = checkNotNull(http2Headers.method(),
+                "method header cannot be null in conversion to HTTP/1.x");
+        final AsciiString path = checkNotNull(http2Headers.path(),
+                "path header cannot be null in conversion to HTTP/1.x");
+        FullHttpRequest msg = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.valueOf(method
+                        .toString()), path.toString(), validateHttpHeaders);
         addHttp2ToHttpHeaders(streamId, http2Headers, msg, false);
         return msg;
     }
@@ -235,7 +240,8 @@ public static void addHttp2ToHttpHeaders(int streamId, Http2Headers sourceHeader
         } catch (Http2Exception ex) {
             throw ex;
         } catch (Exception ex) {
-            PlatformDependent.throwException(ex);
+            throw new Http2StreamException(streamId, Http2Error.PROTOCOL_ERROR,
+                    "HTTP/2 to HTTP/1.x headers conversion error", ex);
         }
 
         headers.remove(HttpHeaderNames.TRANSFER_ENCODING);


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java
index faa037269d1..7c0650e4e37 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java
@@ -14,12 +14,22 @@
  */
 package io.netty.handler.codec.http2;
 
+import static io.netty.handler.codec.http2.Http2TestUtil.as;
+import static io.netty.handler.codec.http2.Http2TestUtil.runInChannel;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import io.netty.bootstrap.Bootstrap;
 import io.netty.bootstrap.ServerBootstrap;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelFuture;
+import io.netty.channel.ChannelHandlerAdapter;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInitializer;
 import io.netty.channel.ChannelPipeline;
@@ -28,6 +38,7 @@
 import io.netty.channel.nio.NioEventLoopGroup;
 import io.netty.channel.socket.nio.NioServerSocketChannel;
 import io.netty.channel.socket.nio.NioSocketChannel;
+import io.netty.handler.codec.AsciiString;
 import io.netty.handler.codec.http.DefaultFullHttpRequest;
 import io.netty.handler.codec.http.DefaultFullHttpResponse;
 import io.netty.handler.codec.http.FullHttpMessage;
@@ -39,8 +50,16 @@
 import io.netty.handler.codec.http.HttpObject;
 import io.netty.handler.codec.http.HttpResponseStatus;
 import io.netty.handler.codec.http.HttpVersion;
+import io.netty.handler.codec.http2.Http2TestUtil.FrameAdapter;
+import io.netty.handler.codec.http2.Http2TestUtil.Http2Runnable;
+import io.netty.util.CharsetUtil;
 import io.netty.util.NetUtil;
 import io.netty.util.concurrent.Future;
+
+import java.net.InetSocketAddress;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -48,15 +67,6 @@
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 
-import java.net.InetSocketAddress;
-import java.util.List;
-import java.util.concurrent.CountDownLatch;
-
-import static io.netty.handler.codec.http2.Http2TestUtil.*;
-import static java.util.concurrent.TimeUnit.*;
-import static org.junit.Assert.*;
-import static org.mockito.Mockito.*;
-
 /**
  * Testing the {@link InboundHttp2ToHttpPriorityAdapter} and base class {@link InboundHttp2ToHttpAdapter} for HTTP/2
  * frames into {@link HttpObject}s
@@ -82,6 +92,7 @@ public class InboundHttp2ToHttpAdapterTest {
     private int maxContentLength;
     private HttpResponseDelegator serverDelegator;
     private HttpResponseDelegator clientDelegator;
+    private Http2Exception serverException;
 
     @Before
     public void setup() throws Exception {
@@ -112,6 +123,20 @@ protected void initChannel(Channel ch) throws Exception {
                 serverDelegator = new HttpResponseDelegator(serverListener, serverLatch);
                 p.addLast(serverDelegator);
                 serverConnectedChannel = ch;
+                p.addLast(new ChannelHandlerAdapter() {
+                    @Override
+                    public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
+                        if (cause instanceof Http2Exception) {
+                            serverException = (Http2Exception) cause;
+                            serverLatch.countDown();
+                        } else if (cause != null && cause.getCause() instanceof Http2Exception) {
+                            serverException = (Http2Exception) cause.getCause();
+                            serverLatch.countDown();
+                        } else {
+                            super.exceptionCaught(ctx, cause);
+                        }
+                    }
+                });
             }
         });
 
@@ -167,10 +192,8 @@ public void clientRequestSingleHeaderNoDataFrames() throws Exception {
             httpHeaders.set(HttpUtil.ExtensionHeaderNames.AUTHORITY.text(), "example.org");
             httpHeaders.setInt(HttpUtil.ExtensionHeaderNames.STREAM_ID.text(), 3);
             httpHeaders.setInt(HttpHeaderNames.CONTENT_LENGTH, 0);
-            final Http2Headers http2Headers =
-                    new DefaultHttp2Headers().method(as("GET")).scheme(as("https"))
-                            .authority(as("example.org"))
-                            .path(as("/some/path/resource2"));
+            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("GET")).scheme(as("https"))
+                    .authority(as("example.org")).path(as("/some/path/resource2"));
             runInChannel(clientChannel, new Http2Runnable() {
                 @Override
                 public void run() {
@@ -184,10 +207,30 @@ public void run() {
             capturedRequests = requestCaptor.getAllValues();
             assertEquals(request, capturedRequests.get(0));
         } finally {
-           request.release();
+            request.release();
         }
     }
 
+    @Test
+    public void clientRequestSingleHeaderNonAsciiShouldThrow() throws Exception {
+        final Http2Headers http2Headers = new DefaultHttp2Headers()
+                .method(as("GET"))
+                .scheme(as("https"))
+                .authority(as("example.org"))
+                .path(as("/some/path/resource2"))
+                .add(new AsciiString("çã".getBytes(CharsetUtil.UTF_8)),
+                        new AsciiString("Ãã".getBytes(CharsetUtil.UTF_8)));
+        runInChannel(clientChannel, new Http2Runnable() {
+            @Override
+            public void run() {
+                frameWriter.writeHeaders(ctxClient(), 3, http2Headers, 0, true, newPromiseClient());
+                ctxClient().flush();
+            }
+        });
+        awaitRequests();
+        assertTrue(serverException instanceof Http2StreamException);
+    }
+
     @Test
     public void clientRequestOneDataFrame() throws Exception {
         final String text = "hello world";
@@ -198,8 +241,8 @@ public void clientRequestOneDataFrame() throws Exception {
             HttpHeaders httpHeaders = request.headers();
             httpHeaders.setInt(HttpUtil.ExtensionHeaderNames.STREAM_ID.text(), 3);
             httpHeaders.setInt(HttpHeaderNames.CONTENT_LENGTH, text.length());
-            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("GET"))
-                    .path(as("/some/path/resource2"));
+            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("GET")).path(
+                    as("/some/path/resource2"));
             runInChannel(clientChannel, new Http2Runnable() {
                 @Override
                 public void run() {
@@ -228,17 +271,17 @@ public void clientRequestMultipleDataFrames() throws Exception {
             HttpHeaders httpHeaders = request.headers();
             httpHeaders.setInt(HttpUtil.ExtensionHeaderNames.STREAM_ID.text(), 3);
             httpHeaders.setInt(HttpHeaderNames.CONTENT_LENGTH, text.length());
-            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("GET"))
-                    .path(as("/some/path/resource2"));
+            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("GET")).path(
+                    as("/some/path/resource2"));
             final int midPoint = text.length() / 2;
             runInChannel(clientChannel, new Http2Runnable() {
                 @Override
                 public void run() {
                     frameWriter.writeHeaders(ctxClient(), 3, http2Headers, 0, false, newPromiseClient());
-                    frameWriter
-                    .writeData(ctxClient(), 3, content.slice(0, midPoint).retain(), 0, false, newPromiseClient());
-                    frameWriter.writeData(ctxClient(), 3, content.slice(midPoint, text.length() - midPoint).retain(), 0,
-                            true, newPromiseClient());
+                    frameWriter.writeData(ctxClient(), 3, content.slice(0, midPoint).retain(), 0, false,
+                            newPromiseClient());
+                    frameWriter.writeData(ctxClient(), 3, content.slice(midPoint, text.length() - midPoint).retain(),
+                            0, true, newPromiseClient());
                     ctxClient().flush();
                 }
             });
@@ -262,8 +305,8 @@ public void clientRequestMultipleEmptyDataFrames() throws Exception {
             HttpHeaders httpHeaders = request.headers();
             httpHeaders.setInt(HttpUtil.ExtensionHeaderNames.STREAM_ID.text(), 3);
             httpHeaders.setInt(HttpHeaderNames.CONTENT_LENGTH, text.length());
-            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("GET"))
-                    .path(as("/some/path/resource2"));
+            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("GET")).path(
+                    as("/some/path/resource2"));
             runInChannel(clientChannel, new Http2Runnable() {
                 @Override
                 public void run() {
@@ -300,13 +343,10 @@ public void clientRequestMultipleHeaders() throws Exception {
             trailingHeaders.set("FoO", "goo");
             trailingHeaders.set("foO2", "goo2");
             trailingHeaders.add("fOo2", "goo3");
-            final Http2Headers http2Headers =
-                    new DefaultHttp2Headers().method(as("GET")).path(
-                            as("/some/path/resource2"));
-            final Http2Headers http2Headers2 =
-                    new DefaultHttp2Headers().set(as("foo"), as("goo"))
-                            .set(as("foo2"), as("goo2"))
-                            .add(as("foo2"), as("goo3"));
+            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("GET")).path(
+                    as("/some/path/resource2"));
+            final Http2Headers http2Headers2 = new DefaultHttp2Headers().set(as("foo"), as("goo"))
+                    .set(as("foo2"), as("goo2")).add(as("foo2"), as("goo3"));
             runInChannel(clientChannel, new Http2Runnable() {
                 @Override
                 public void run() {
@@ -340,13 +380,10 @@ public void clientRequestTrailingHeaders() throws Exception {
             trailingHeaders.set("Foo", "goo");
             trailingHeaders.set("fOo2", "goo2");
             trailingHeaders.add("foO2", "goo3");
-            final Http2Headers http2Headers =
-                    new DefaultHttp2Headers().method(as("GET")).path(
-                            as("/some/path/resource2"));
-            final Http2Headers http2Headers2 =
-                    new DefaultHttp2Headers().set(as("foo"), as("goo"))
-                            .set(as("foo2"), as("goo2"))
-                            .add(as("foo2"), as("goo3"));
+            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("GET")).path(
+                    as("/some/path/resource2"));
+            final Http2Headers http2Headers2 = new DefaultHttp2Headers().set(as("foo"), as("goo"))
+                    .set(as("foo2"), as("goo2")).add(as("foo2"), as("goo3"));
             runInChannel(clientChannel, new Http2Runnable() {
                 @Override
                 public void run() {
@@ -386,10 +423,10 @@ public void clientRequestStreamDependencyInHttpMessageFlow() throws Exception {
             httpHeaders2.setInt(HttpUtil.ExtensionHeaderNames.STREAM_DEPENDENCY_ID.text(), 3);
             httpHeaders2.setInt(HttpUtil.ExtensionHeaderNames.STREAM_WEIGHT.text(), 123);
             httpHeaders2.setInt(HttpHeaderNames.CONTENT_LENGTH, text2.length());
-            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("PUT"))
-                    .path(as("/some/path/resource"));
-            final Http2Headers http2Headers2 = new DefaultHttp2Headers().method(as("PUT"))
-                    .path(as("/some/path/resource2"));
+            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("PUT")).path(
+                    as("/some/path/resource"));
+            final Http2Headers http2Headers2 = new DefaultHttp2Headers().method(as("PUT")).path(
+                    as("/some/path/resource2"));
             runInChannel(clientChannel, new Http2Runnable() {
                 @Override
                 public void run() {
@@ -433,10 +470,10 @@ public void clientRequestStreamDependencyOutsideHttpMessageFlow() throws Excepti
             HttpHeaders httpHeaders2 = request2.headers();
             httpHeaders2.setInt(HttpUtil.ExtensionHeaderNames.STREAM_ID.text(), 5);
             httpHeaders2.setInt(HttpHeaderNames.CONTENT_LENGTH, text2.length());
-            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("PUT"))
-                    .path(as("/some/path/resource"));
-            final Http2Headers http2Headers2 = new DefaultHttp2Headers().method(as("PUT"))
-                    .path(as("/some/path/resource2"));
+            final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("PUT")).path(
+                    as("/some/path/resource"));
+            final Http2Headers http2Headers2 = new DefaultHttp2Headers().method(as("PUT")).path(
+                    as("/some/path/resource2"));
             HttpHeaders httpHeaders3 = request3.headers();
             httpHeaders3.setInt(HttpUtil.ExtensionHeaderNames.STREAM_ID.text(), 5);
             httpHeaders3.setInt(HttpUtil.ExtensionHeaderNames.STREAM_DEPENDENCY_ID.text(), 3);
@@ -478,8 +515,8 @@ public void serverRequestPushPromise() throws Exception {
                 content, true);
         final FullHttpMessage response2 = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.CREATED,
                 content2, true);
-        final FullHttpMessage request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1,
-                HttpMethod.GET, "/push/test", true);
+        final FullHttpMessage request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/push/test",
+                true);
         try {
             HttpHeaders httpHeaders = response.headers();
             httpHeaders.setInt(HttpUtil.ExtensionHeaderNames.STREAM_ID.text(), 3);
@@ -494,8 +531,7 @@ public void serverRequestPushPromise() throws Exception {
             httpHeaders = request.headers();
             httpHeaders.setInt(HttpUtil.ExtensionHeaderNames.STREAM_ID.text(), 3);
             httpHeaders.setInt(HttpHeaderNames.CONTENT_LENGTH, 0);
-            final Http2Headers http2Headers3 = new DefaultHttp2Headers().method(as("GET"))
-                    .path(as("/push/test"));
+            final Http2Headers http2Headers3 = new DefaultHttp2Headers().method(as("GET")).path(as("/push/test"));
             runInChannel(clientChannel, new Http2Runnable() {
                 @Override
                 public void run() {
@@ -510,9 +546,8 @@ public void run() {
             assertEquals(request, capturedRequests.get(0));
 
             final Http2Headers http2Headers = new DefaultHttp2Headers().status(as("200"));
-            final Http2Headers http2Headers2 =
-                    new DefaultHttp2Headers().status(as("201")).scheme(as("https"))
-                            .authority(as("example.org"));
+            final Http2Headers http2Headers2 = new DefaultHttp2Headers().status(as("201")).scheme(as("https"))
+                    .authority(as("example.org"));
             runInChannel(serverConnectedChannel, new Http2Runnable() {
                 @Override
                 public void run() {
@@ -544,11 +579,8 @@ public void serverResponseHeaderInformational() throws Exception {
         httpHeaders.setInt(HttpUtil.ExtensionHeaderNames.STREAM_ID.text(), 3);
         httpHeaders.set(HttpHeaderNames.EXPECT, HttpHeaderValues.CONTINUE);
         httpHeaders.setInt(HttpHeaderNames.CONTENT_LENGTH, 0);
-        final Http2Headers http2Headers =
-                new DefaultHttp2Headers()
-                        .method(as("PUT"))
-                        .path(as("/info/test"))
-                        .set(as(HttpHeaderNames.EXPECT.toString()), as(HttpHeaderValues.CONTINUE.toString()));
+        final Http2Headers http2Headers = new DefaultHttp2Headers().method(as("PUT")).path(as("/info/test"))
+                .set(as(HttpHeaderNames.EXPECT.toString()), as(HttpHeaderValues.CONTINUE.toString()));
         final FullHttpMessage response = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.CONTINUE);
         final String text = "a big payload";
         final ByteBuf payload = Unpooled.copiedBuffer(text.getBytes());