Repository: netty/netty (Java)

Issue:
Invalid HTTP2 pseudo-headers kill the session instead of being treated as malformed
### Expected behavior

When a peer receives invalid pseudo headers the request or response should be treated as malformed as prescribed in section [8.1.2.1](https://http2.github.io/http2-spec/#rfc.section.8.1.2.1). This entails a stream level PROTOCOL_ERROR as described in section [8.1.2.6](https://http2.github.io/http2-spec/#rfc.section.8.1.2.6)

### Actual behavior

The [HpackDecoder](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java#L401) throws a connection level PROTOCOL_ERROR mid-decode, resulting in the connection being torn down.

### Steps to reproduce

Attempt to send a HTTP/2 request with an unknown pseudo header, pseudo headers out of order, or a mix of request and response type pseudo headers.


Hints:
This is causing minor issues for us migrating to version of Netty that included the commit associated with PR https://github.com/netty/netty/pull/7500. Those problems include killing connections in production (relatively infrequent, but happens enough to be noticed) and some failing tests. We can work around it by validating headers before they are sent to the peer or disabling validation but ideally the receiving end, which should do this validation regardless to adhere to the spec, only needs to do it and behave correctly.
@Scottmitch @ejona86 any comments ?
@bryce-anderson just to double check, the spec says we should send an RST?  I agree that seems reasonable.
@mosesn, that's my interpretation of http2 spec section [8.1.2.6](https://http2.github.io/http2-spec/#rfc.section.8.1.2.6):
> Malformed requests or responses that are detected MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.

I think you noted in your review of the patch that introduced validation that tearing down the connection is pretty heavy handed. 😄
Stream-level PROTOCOL_ERROR seems good.
+1

> Am 22.06.2018 um 16:46 schrieb Eric Anderson <notifications@github.com>:
> 
> Stream-level PROTOCOL_ERROR seems good.
> 
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.

I'm happy to work on this and I think we have two options.

1. We can leave validation in the HPACK decoder but move it to the end of the decode phase: we need to completely decode the headers to keep the HPACK state consistent.
2. We can move it clear out of the HPACK decoder (it's not really a HPACK concern) and change the API to specify if the headers belong to a request, response, or trailers.

I'd speculate that we should start with 1 and do 2 after a bit more research into the right place and how it will affect current users.
1) sounds good to me... @ejona86 @carl-mastrangelo @Scottmitch agree ?

Patch:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java
@@ -44,6 +44,7 @@
 import static io.netty.handler.codec.http2.Http2Error.COMPRESSION_ERROR;
 import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
 import static io.netty.handler.codec.http2.Http2Exception.connectionError;
+import static io.netty.handler.codec.http2.Http2Exception.streamError;
 import static io.netty.handler.codec.http2.Http2Headers.PseudoHeaderName.getPseudoHeader;
 import static io.netty.handler.codec.http2.Http2Headers.PseudoHeaderName.hasPseudoHeaderFormat;
 import static io.netty.util.AsciiString.EMPTY_STRING;
@@ -119,24 +120,21 @@ final class HpackDecoder {
      * This method assumes the entire header block is contained in {@code in}.
      */
     public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean validateHeaders) throws Http2Exception {
-        Http2HeadersSink sink = new Http2HeadersSink(headers, maxHeaderListSize);
-        decode(in, sink, validateHeaders);
+        Http2HeadersSink sink = new Http2HeadersSink(streamId, headers, maxHeaderListSize, validateHeaders);
+        decode(in, sink);
 
-        // we have read all of our headers. See if we have exceeded our maxHeaderListSize. We must
-        // delay throwing until this point to prevent dynamic table corruption
-        if (sink.exceededMaxLength()) {
-            headerListSizeExceeded(streamId, maxHeaderListSize, true);
-        }
+        // Now that we've read all of our headers we can perform the validation steps. We must
+        // delay throwing until this point to prevent dynamic table corruption.
+        sink.finish();
     }
 
-    private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2Exception {
+    private void decode(ByteBuf in, Sink sink) throws Http2Exception {
         int index = 0;
         int nameLength = 0;
         int valueLength = 0;
         byte state = READ_HEADER_REPRESENTATION;
         boolean huffmanEncoded = false;
         CharSequence name = null;
-        HeaderType headerType = null;
         IndexType indexType = IndexType.NONE;
         while (in.isReadable()) {
             switch (state) {
@@ -157,7 +155,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
                                 break;
                             default:
                                 HpackHeaderField indexedHeader = getIndexedHeader(index);
-                                headerType = validate(indexedHeader.name, headerType, validateHeaders);
                                 sink.appendToHeaderList(indexedHeader.name, indexedHeader.value);
                         }
                     } else if ((b & 0x40) == 0x40) {
@@ -174,7 +171,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
                             default:
                                 // Index was stored as the prefix
                                 name = readName(index);
-                                headerType = validate(name, headerType, validateHeaders);
                                 nameLength = name.length();
                                 state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
                         }
@@ -201,7 +197,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
                             default:
                                 // Index was stored as the prefix
                                 name = readName(index);
-                                headerType = validate(name, headerType, validateHeaders);
                                 nameLength = name.length();
                                 state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
                         }
@@ -215,7 +210,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
 
                 case READ_INDEXED_HEADER:
                     HpackHeaderField indexedHeader = getIndexedHeader(decodeULE128(in, index));
-                    headerType = validate(indexedHeader.name, headerType, validateHeaders);
                     sink.appendToHeaderList(indexedHeader.name, indexedHeader.value);
                     state = READ_HEADER_REPRESENTATION;
                     break;
@@ -223,7 +217,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
                 case READ_INDEXED_HEADER_NAME:
                     // Header Name matches an entry in the Header Table
                     name = readName(decodeULE128(in, index));
-                    headerType = validate(name, headerType, validateHeaders);
                     nameLength = name.length();
                     state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
                     break;
@@ -254,7 +247,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
                     }
 
                     name = readStringLiteral(in, nameLength, huffmanEncoded);
-                    headerType = validate(name, headerType, validateHeaders);
 
                     state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
                     break;
@@ -268,7 +260,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
                             state = READ_LITERAL_HEADER_VALUE_LENGTH;
                             break;
                         case 0:
-                            headerType = validate(name, headerType, validateHeaders);
                             insertHeader(sink, name, EMPTY_STRING, indexType);
                             state = READ_HEADER_REPRESENTATION;
                             break;
@@ -293,7 +284,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
                     }
 
                     CharSequence value = readStringLiteral(in, valueLength, huffmanEncoded);
-                    headerType = validate(name, headerType, validateHeaders);
                     insertHeader(sink, name, value, indexType);
                     state = READ_HEADER_REPRESENTATION;
                     break;
@@ -327,7 +317,7 @@ public void setMaxHeaderTableSize(long maxHeaderTableSize) throws Http2Exception
     }
 
     /**
-     * @deprecated use {@link #setmaxHeaderListSize(long)}; {@code maxHeaderListSizeGoAway} is
+     * @deprecated use {@link #setMaxHeaderListSize(long)}; {@code maxHeaderListSizeGoAway} is
      *     ignored
      */
     @Deprecated
@@ -385,26 +375,23 @@ private void setDynamicTableSize(long dynamicTableSize) throws Http2Exception {
         hpackDynamicTable.setCapacity(dynamicTableSize);
     }
 
-    private HeaderType validate(CharSequence name, HeaderType previousHeaderType,
-                                final boolean validateHeaders) throws Http2Exception {
-        if (!validateHeaders) {
-            return null;
-        }
-
+    private static HeaderType validate(int streamId, CharSequence name,
+                                       HeaderType previousHeaderType) throws Http2Exception {
         if (hasPseudoHeaderFormat(name)) {
             if (previousHeaderType == HeaderType.REGULAR_HEADER) {
-                throw connectionError(PROTOCOL_ERROR, "Pseudo-header field '%s' found after regular header.", name);
+                throw streamError(streamId, PROTOCOL_ERROR,
+                        "Pseudo-header field '%s' found after regular header.", name);
             }
 
             final Http2Headers.PseudoHeaderName pseudoHeader = getPseudoHeader(name);
             if (pseudoHeader == null) {
-                throw connectionError(PROTOCOL_ERROR, "Invalid HTTP/2 pseudo-header '%s' encountered.", name);
+                throw streamError(streamId, PROTOCOL_ERROR, "Invalid HTTP/2 pseudo-header '%s' encountered.", name);
             }
 
             final HeaderType currentHeaderType = pseudoHeader.isRequestOnly() ?
                     HeaderType.REQUEST_PSEUDO_HEADER : HeaderType.RESPONSE_PSEUDO_HEADER;
             if (previousHeaderType != null && currentHeaderType != previousHeaderType) {
-                throw connectionError(PROTOCOL_ERROR, "Mix of request and response pseudo-headers.");
+                throw streamError(streamId, PROTOCOL_ERROR, "Mix of request and response pseudo-headers.");
             }
 
             return currentHeaderType;
@@ -435,8 +422,7 @@ private HpackHeaderField getIndexedHeader(int index) throws Http2Exception {
         throw INDEX_HEADER_ILLEGAL_INDEX_VALUE;
     }
 
-    private void insertHeader(Sink sink, CharSequence name, CharSequence value,
-                              IndexType indexType) throws Http2Exception {
+    private void insertHeader(Sink sink, CharSequence name, CharSequence value, IndexType indexType) {
         sink.appendToHeaderList(name, value);
 
         switch (indexType) {
@@ -529,32 +515,55 @@ private enum HeaderType {
 
     private interface Sink {
         void appendToHeaderList(CharSequence name, CharSequence value);
+        void finish() throws Http2Exception;
     }
 
     private static final class Http2HeadersSink implements Sink {
         private final Http2Headers headers;
         private final long maxHeaderListSize;
+        private final int streamId;
+        private final boolean validate;
         private long headersLength;
         private boolean exceededMaxLength;
+        private HeaderType previousType;
+        private Http2Exception validationException;
 
-        public Http2HeadersSink(Http2Headers headers, long maxHeaderListSize) {
+        public Http2HeadersSink(int streamId, Http2Headers headers, long maxHeaderListSize, boolean validate) {
             this.headers = headers;
             this.maxHeaderListSize = maxHeaderListSize;
+            this.streamId = streamId;
+            this.validate = validate;
+        }
+
+        @Override
+        public void finish() throws Http2Exception {
+            if (exceededMaxLength) {
+                headerListSizeExceeded(streamId, maxHeaderListSize, true);
+            } else if (validationException != null) {
+                throw validationException;
+            }
         }
 
         @Override
         public void appendToHeaderList(CharSequence name, CharSequence value) {
             headersLength += HpackHeaderField.sizeOf(name, value);
-            if (headersLength > maxHeaderListSize) {
-                exceededMaxLength = true;
+            exceededMaxLength |= headersLength > maxHeaderListSize;
+
+            if (exceededMaxLength || validationException != null) {
+                // We don't store the header since we've already failed validation requirements.
+                return;
             }
-            if (!exceededMaxLength) {
-                headers.add(name, value);
+
+            if (validate) {
+                try {
+                    previousType = HpackDecoder.validate(streamId, name, previousType);
+                } catch (Http2Exception ex) {
+                    validationException = ex;
+                    return;
+                }
             }
-        }
 
-        public boolean exceededMaxLength() {
-            return exceededMaxLength;
+            headers.add(name, value);
         }
     }
 }


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java
@@ -544,7 +544,7 @@ public void unknownPseudoHeader() throws Exception {
 
             Http2Headers decoded = new DefaultHttp2Headers();
 
-            expectedException.expect(Http2Exception.class);
+            expectedException.expect(Http2Exception.StreamException.class);
             hpackDecoder.decode(1, in, decoded, true);
         } finally {
             in.release();
@@ -588,7 +588,7 @@ public void requestPseudoHeaderInResponse() throws Exception {
 
             Http2Headers decoded = new DefaultHttp2Headers();
 
-            expectedException.expect(Http2Exception.class);
+            expectedException.expect(Http2Exception.StreamException.class);
             hpackDecoder.decode(1, in, decoded, true);
         } finally {
             in.release();
@@ -608,7 +608,7 @@ public void responsePseudoHeaderInRequest() throws Exception {
 
             Http2Headers decoded = new DefaultHttp2Headers();
 
-            expectedException.expect(Http2Exception.class);
+            expectedException.expect(Http2Exception.StreamException.class);
             hpackDecoder.decode(1, in, decoded, true);
         } finally {
             in.release();
@@ -628,10 +628,47 @@ public void pseudoHeaderAfterRegularHeader() throws Exception {
 
             Http2Headers decoded = new DefaultHttp2Headers();
 
-            expectedException.expect(Http2Exception.class);
+            expectedException.expect(Http2Exception.StreamException.class);
             hpackDecoder.decode(1, in, decoded, true);
         } finally {
             in.release();
         }
     }
+
+    @Test
+    public void failedValidationDoesntCorruptHpack() throws Exception {
+        ByteBuf in1 = Unpooled.buffer(200);
+        ByteBuf in2 = Unpooled.buffer(200);
+        try {
+            HpackEncoder hpackEncoder = new HpackEncoder(true);
+
+            Http2Headers toEncode = new DefaultHttp2Headers();
+            toEncode.add(":method", "GET");
+            toEncode.add(":status", "200");
+            toEncode.add("foo", "bar");
+            hpackEncoder.encodeHeaders(1, in1, toEncode, NEVER_SENSITIVE);
+
+            Http2Headers decoded = new DefaultHttp2Headers();
+
+            try {
+                hpackDecoder.decode(1, in1, decoded, true);
+                fail("Should have thrown a StreamException");
+            } catch (Http2Exception.StreamException expected) {
+                assertEquals(1, expected.streamId());
+            }
+
+            // Do it again, this time without validation, to make sure the HPACK state is still sane.
+            decoded.clear();
+            hpackEncoder.encodeHeaders(1, in2, toEncode, NEVER_SENSITIVE);
+            hpackDecoder.decode(1, in2, decoded, false);
+
+            assertEquals(3, decoded.size());
+            assertEquals("GET", decoded.method().toString());
+            assertEquals("200", decoded.status().toString());
+            assertEquals("bar", decoded.get("foo").toString());
+        } finally {
+            in1.release();
+            in2.release();
+        }
+    }
 }