More strict parsing of initial line / http headers (#10058)

Motivation:

Our parsing of the initial line / http headers did treat some characters as separators which should better trigger an exception during parsing.

Modifications:

- Tighten up parsing of the inital line by follow recommentation of RFC7230
- Restrict separators to OWS for http headers
- Add unit test

Result:

Stricter parsing of HTTP1

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

@@ -748,13 +748,13 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
         int cStart;
         int cEnd;
 
-        aStart = findNonWhitespace(sb, 0);
+        aStart = findNonSPLenient(sb, 0);
-        aEnd = findWhitespace(sb, aStart);
+        aEnd = findSPLenient(sb, aStart);
 
-        bStart = findNonWhitespace(sb, aEnd);
+        bStart = findNonSPLenient(sb, aEnd);
-        bEnd = findWhitespace(sb, bStart);
+        bEnd = findSPLenient(sb, bStart);
 
-        cStart = findNonWhitespace(sb, bEnd);
+        cStart = findNonSPLenient(sb, bEnd);
         cEnd = findEndOfString(sb);
 
         return new String[] {
@@ -771,7 +771,7 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
         int valueStart;
         int valueEnd;
 
-        nameStart = findNonWhitespace(sb, 0);
+        nameStart = findNonWhitespace(sb, 0, false);
         for (nameEnd = nameStart; nameEnd < length; nameEnd ++) {
             char ch = sb.charAtUnsafe(nameEnd);
             // https://tools.ietf.org/html/rfc7230#section-3.2.4
@@ -788,7 +788,7 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
                     // is done in the DefaultHttpHeaders implementation.
                     //
                     // In the case of decoding a response we will "skip" the whitespace.
-                    (!isDecodingRequest() && Character.isWhitespace(ch))) {
+                    (!isDecodingRequest() && isOWS(ch))) {
                 break;
             }
         }
@@ -806,7 +806,7 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
         }
 
         name = sb.subStringUnsafe(nameStart, nameEnd);
-        valueStart = findNonWhitespace(sb, colonEnd);
+        valueStart = findNonWhitespace(sb, colonEnd, true);
         if (valueStart == length) {
             value = EMPTY_VALUE;
         } else {
@@ -815,19 +815,45 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
         }
     }
 
-    private static int findNonWhitespace(AppendableCharSequence sb, int offset) {
+    private static int findNonSPLenient(AppendableCharSequence sb, int offset) {
         for (int result = offset; result < sb.length(); ++result) {
-            if (!Character.isWhitespace(sb.charAtUnsafe(result))) {
+            char c = sb.charAtUnsafe(result);
+            // See https://tools.ietf.org/html/rfc7230#section-3.5
+            if (isSPLenient(c)) {
+                continue;
+            }
+            if (Character.isWhitespace(c)) {
+                // Any other whitespace delimiter is invalid
+                throw new IllegalArgumentException("Invalid separator");
+            }
+            return result;
+        }
+        return sb.length();
+    }
+
+    private static int findSPLenient(AppendableCharSequence sb, int offset) {
+        for (int result = offset; result < sb.length(); ++result) {
+            if (isSPLenient(sb.charAtUnsafe(result))) {
                 return result;
             }
         }
         return sb.length();
     }
 
-    private static int findWhitespace(AppendableCharSequence sb, int offset) {
+    private static boolean isSPLenient(char c) {
+        // See https://tools.ietf.org/html/rfc7230#section-3.5
+        return c == ' ' || c == (char) 0x09 || c == (char) 0x0B || c == (char) 0x0C || c == (char) 0x0D;
+    }
+
+    private static int findNonWhitespace(AppendableCharSequence sb, int offset, boolean validateOWS) {
         for (int result = offset; result < sb.length(); ++result) {
-            if (Character.isWhitespace(sb.charAtUnsafe(result))) {
+            char c = sb.charAtUnsafe(result);
+            if (!Character.isWhitespace(c)) {
                 return result;
+            } else if (validateOWS && !isOWS(c)) {
+                // Only OWS is supported for whitespace
+                throw new IllegalArgumentException("Invalid separator, only a single space or horizontal tab allowed," +
+                        " but received a '" + c + "'");
             }
         }
         return sb.length();
@@ -842,6 +868,10 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
         return 0;
     }
 
+    private static boolean isOWS(char ch) {
+        return ch == ' ' || ch == (char) 0x09;
+    }
+
     private static class HeaderParser implements ByteProcessor {
         private final AppendableCharSequence seq;
         private final int maxLength;
@@ -871,10 +901,13 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
         @Override
         public boolean process(byte value) throws Exception {
             char nextByte = (char) (value & 0xFF);
-            if (nextByte == HttpConstants.CR) {
-                return true;
-            }
             if (nextByte == HttpConstants.LF) {
+                int len = seq.length();
+                // Drop CR if we had a CRLF pair
+                if (len >= 1 && seq.charAtUnsafe(len - 1) == HttpConstants.CR) {
+                    -- size;
+                    seq.setLength(len - 1);
+                }
                 return false;
             }
 

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

@@ -400,6 +400,30 @@ public class HttpRequestDecoderTest {
         testInvalidHeaders0(requestStr);
     }
 
+    @Test
+    public void testContentLengthAndTransferEncodingHeadersWithVerticalTab() {
+        testContentLengthAndTransferEncodingHeadersWithInvalidSeparator((char) 0x0b, false);
+        testContentLengthAndTransferEncodingHeadersWithInvalidSeparator((char) 0x0b, true);
+    }
+
+    @Test
+    public void testContentLengthAndTransferEncodingHeadersWithCR() {
+        testContentLengthAndTransferEncodingHeadersWithInvalidSeparator((char) 0x0d, false);
+        testContentLengthAndTransferEncodingHeadersWithInvalidSeparator((char) 0x0d, true);
+    }
+
+    private static void testContentLengthAndTransferEncodingHeadersWithInvalidSeparator(
+            char separator, boolean extraLine) {
+        String requestStr = "POST / HTTP/1.1\r\n" +
+                "Host: example.com\r\n" +
+                "Connection: close\r\n" +
+                "Content-Length: 9\r\n" +
+                "Transfer-Encoding:" + separator + "chunked\r\n\r\n" +
+                (extraLine ? "0\r\n\r\n" : "") +
+                "something\r\n\r\n";
+        testInvalidHeaders0(requestStr);
+    }
+
     @Test
     public void testContentLengthHeaderAndChunked() {
         String requestStr = "POST / HTTP/1.1\r\n" +
@@ -408,7 +432,6 @@ public class HttpRequestDecoderTest {
                 "Content-Length: 5\r\n" +
                 "Transfer-Encoding: chunked\r\n\r\n" +
                 "0\r\n\r\n";
-
         EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder());
         assertTrue(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)));
         HttpRequest request = channel.readInbound();

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

@@ -50,7 +50,7 @@ public class HttpResponseDecoderTest {
         final int maxHeaderSize = 8192;
 
         final EmbeddedChannel ch = new EmbeddedChannel(new HttpResponseDecoder(4096, maxHeaderSize));
-        final char[] bytes = new char[maxHeaderSize / 2 - 2];
+        final char[] bytes = new char[maxHeaderSize / 2 - 4];
         Arrays.fill(bytes, 'a');
 
         ch.writeInbound(Unpooled.copiedBuffer("HTTP/1.1 200 OK\r\n", CharsetUtil.US_ASCII));

common/src/main/java/io/netty/util/internal/AppendableCharSequence.java

@@ -37,6 +37,13 @@ public final class AppendableCharSequence implements CharSequence, Appendable {
         pos = chars.length;
     }
 
+    public void setLength(int length) {
+        if (length < 0 || length > pos) {
+            throw new IllegalArgumentException("length: " + length + " (length: >= 0, <= " + pos + ')');
+        }
+        this.pos = length;
+    }
+
     @Override
     public int length() {
         return pos;