HttpObjectDecoder should limit the number of control chars (#10112)

Motivation:

At the moment HttpObjectDecoder does not limit the number of controls chars. These should be counted with the initial line and so a limit should be exposed

Modifications:

- Change LineParser to also be used when skipping control chars and so enforce a limit
- Add various tests to ensure that limit is enforced

Result:

Fixes https://github.com/netty/netty/issues/10111

codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java

@@ -185,12 +185,8 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
         }
 
         switch (currentState) {
-        case SKIP_CONTROL_CHARS: {
-            if (!skipControlCharacters(buffer)) {
-                return;
-            }
-            currentState = State.READ_INITIAL;
-        }
+        case SKIP_CONTROL_CHARS:
+            // Fall-through
         case READ_INITIAL: try {
             AppendableCharSequence line = lineParser.parse(buffer);
             if (line == null) {
@@ -545,22 +541,6 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
         return chunk;
     }
 
-    private static boolean skipControlCharacters(ByteBuf buffer) {
-        boolean skiped = false;
-        final int wIdx = buffer.writerIndex();
-        int rIdx = buffer.readerIndex();
-        while (wIdx > rIdx) {
-            int c = buffer.getUnsignedByte(rIdx++);
-            if (!Character.isISOControl(c) && !Character.isWhitespace(c)) {
-                rIdx--;
-                skiped = true;
-                break;
-            }
-        }
-        buffer.readerIndex(rIdx);
-        return skiped;
-    }
-
     private State readHeaders(ByteBuf buffer) {
         final HttpMessage message = this.message;
         final HttpHeaders headers = message.headers();
@@ -911,6 +891,13 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
                 return false;
             }
 
+            increaseCount();
+
+            seq.append(nextByte);
+            return true;
+        }
+
+        protected final void increaseCount() {
             if (++ size > maxLength) {
                 // TODO: Respond with Bad Request and discard the traffic
                 //    or close the connection.
@@ -918,9 +905,6 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
                 //       If decoding a response, just throw an exception.
                 throw newException(maxLength);
             }
-
-            seq.append(nextByte);
-            return true;
         }
 
         protected TooLongFrameException newException(int maxLength) {
@@ -928,7 +912,7 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
         }
     }
 
-    private static final class LineParser extends HeaderParser {
+    private final class LineParser extends HeaderParser {
 
         LineParser(AppendableCharSequence seq, int maxLength) {
             super(seq, maxLength);
@@ -940,6 +924,19 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
             return super.parse(buffer);
         }
 
+        @Override
+        public boolean process(byte value) throws Exception {
+            if (currentState == State.SKIP_CONTROL_CHARS) {
+                char c = (char) (value & 0xFF);
+                if (Character.isISOControl(c) || Character.isWhitespace(c)) {
+                    increaseCount();
+                    return true;
+                }
+                currentState = State.READ_INITIAL;
+            }
+            return super.process(value);
+        }
+
         @Override
         protected TooLongFrameException newException(int maxLength) {
             return new TooLongFrameException("An HTTP line is larger than " + maxLength + " bytes.");

codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java

@@ -308,6 +308,42 @@ public class HttpRequestDecoderTest {
         assertFalse(channel.finish());
     }
 
+    @Test
+    public void testTooLargeInitialLineWithWSOnly() {
+        testTooLargeInitialLineWithControlCharsOnly("                    ");
+    }
+
+    @Test
+    public void testTooLargeInitialLineWithCRLFOnly() {
+        testTooLargeInitialLineWithControlCharsOnly("\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n");
+    }
+
+    private static void testTooLargeInitialLineWithControlCharsOnly(String controlChars) {
+        EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder(15, 1024));
+        String requestStr = controlChars + "GET / HTTP/1.1\r\n" +
+                "Host: localhost1\r\n\r\n";
+
+        assertTrue(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)));
+        HttpRequest request = channel.readInbound();
+        assertTrue(request.decoderResult().isFailure());
+        assertTrue(request.decoderResult().cause() instanceof TooLongFrameException);
+        assertFalse(channel.finish());
+    }
+
+    @Test
+    public void testInitialLineWithLeadingControlChars() {
+        EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder());
+        String crlf = "\r\n";
+        String request =  crlf + "GET /some/path HTTP/1.1" + crlf +
+                "Host: localhost" + crlf + crlf;
+        assertTrue(channel.writeInbound(Unpooled.copiedBuffer(request, CharsetUtil.US_ASCII)));
+        HttpRequest req = channel.readInbound();
+        assertEquals(HttpMethod.GET, req.method());
+        assertEquals("/some/path", req.uri());
+        assertEquals(HttpVersion.HTTP_1_1, req.protocolVersion());
+        assertTrue(channel.finishAndReleaseAll());
+    }
+
     @Test
     public void testTooLargeHeaders() {
         EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder(1024, 10));