From 9ae782d632ff18f7c9e645c58458b3180d257ff3 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Wed, 26 Feb 2020 09:49:39 +0100 Subject: [PATCH] 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 --- .../handler/codec/http/HttpObjectDecoder.java | 63 ++++++++++++++----- .../codec/http/HttpRequestDecoderTest.java | 25 +++++++- .../codec/http/HttpResponseDecoderTest.java | 2 +- .../util/internal/AppendableCharSequence.java | 7 +++ 4 files changed, 80 insertions(+), 17 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java index 0486104359..6d458782f0 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java @@ -752,13 +752,13 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder { int cStart; int cEnd; - aStart = findNonWhitespace(sb, 0); - aEnd = findWhitespace(sb, aStart); + aStart = findNonSPLenient(sb, 0); + aEnd = findSPLenient(sb, aStart); - bStart = findNonWhitespace(sb, aEnd); - bEnd = findWhitespace(sb, bStart); + bStart = findNonSPLenient(sb, aEnd); + bEnd = findSPLenient(sb, bStart); - cStart = findNonWhitespace(sb, bEnd); + cStart = findNonSPLenient(sb, bEnd); cEnd = findEndOfString(sb); return new String[] { @@ -775,7 +775,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 @@ -792,7 +792,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; } } @@ -810,7 +810,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 { @@ -819,19 +819,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(); @@ -846,6 +872,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; @@ -875,10 +905,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; } diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java index f92a32d0ad..6335727065 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java +++ b/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(); diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java index d4781bf7d1..b0ccd0cd9a 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java +++ b/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, 8192)); - 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)); diff --git a/common/src/main/java/io/netty/util/internal/AppendableCharSequence.java b/common/src/main/java/io/netty/util/internal/AppendableCharSequence.java index e8e6abf566..2e44b3375f 100644 --- a/common/src/main/java/io/netty/util/internal/AppendableCharSequence.java +++ b/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;