From 07aa6b5938a8b6ed7a6586e066400e2643897323 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 9 Dec 2021 14:49:43 +0100 Subject: [PATCH] Merge pull request from GHSA-wx5j-54mm-rqqq Motivation: We should validate that only OWS is allowed before / after a header name and otherwise throw. At the moment we just "strip" everything except OWS. Modifications: - Adjust code to correctly validate - Add unit tests Result: More strict and correct behaviour --- .../codec/http/DefaultHttpHeaders.java | 8 ++ .../handler/codec/http/HttpObjectDecoder.java | 8 +- .../codec/http/HttpRequestDecoderTest.java | 87 +++++++++++++++++-- .../codec/http/HttpResponseDecoderTest.java | 78 +++++++++++++++++ 4 files changed, 171 insertions(+), 10 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java index ddd68c74d98..a728e678baa 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java @@ -367,6 +367,10 @@ public HttpHeaders copy() { private static void validateHeaderNameElement(byte value) { switch (value) { + case 0x1c: + case 0x1d: + case 0x1e: + case 0x1f: case 0x00: case '\t': case '\n': @@ -391,6 +395,10 @@ private static void validateHeaderNameElement(byte value) { private static void validateHeaderNameElement(char value) { switch (value) { + case 0x1c: + case 0x1d: + case 0x1e: + case 0x1f: case 0x00: case '\t': case '\n': 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 cd2b4969663..974dafe1904 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 @@ -824,7 +824,7 @@ private void splitHeader(AppendableCharSequence sb) { int valueStart; int valueEnd; - nameStart = findNonWhitespace(sb, 0, false); + nameStart = findNonWhitespace(sb, 0); for (nameEnd = nameStart; nameEnd < length; nameEnd ++) { char ch = sb.charAtUnsafe(nameEnd); // https://tools.ietf.org/html/rfc7230#section-3.2.4 @@ -859,7 +859,7 @@ private void splitHeader(AppendableCharSequence sb) { } name = sb.subStringUnsafe(nameStart, nameEnd); - valueStart = findNonWhitespace(sb, colonEnd, true); + valueStart = findNonWhitespace(sb, colonEnd); if (valueStart == length) { value = EMPTY_VALUE; } else { @@ -898,12 +898,12 @@ private static boolean isSPLenient(char c) { return c == ' ' || c == (char) 0x09 || c == (char) 0x0B || c == (char) 0x0C || c == (char) 0x0D; } - private static int findNonWhitespace(AppendableCharSequence sb, int offset, boolean validateOWS) { + private static int findNonWhitespace(AppendableCharSequence sb, int offset) { for (int result = offset; result < sb.length(); ++result) { char c = sb.charAtUnsafe(result); if (!Character.isWhitespace(c)) { return result; - } else if (validateOWS && !isOWS(c)) { + } else if (!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 + "' (0x" + Integer.toHexString(c) + ")"); 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 9edd6929c31..cac2f867ec1 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 @@ -15,6 +15,7 @@ */ package io.netty.handler.codec.http; +import io.netty.buffer.ByteBuf; import io.netty.buffer.Unpooled; import io.netty.channel.embedded.EmbeddedChannel; import io.netty.handler.codec.TooLongFrameException; @@ -357,6 +358,75 @@ public void testTooLargeHeaders() { assertFalse(channel.finish()); } + @Test + public void testHeaderNameStartsWithControlChar1c() { + testHeaderNameStartsWithControlChar(0x1c); + } + + @Test + public void testHeaderNameStartsWithControlChar1d() { + testHeaderNameStartsWithControlChar(0x1d); + } + + @Test + public void testHeaderNameStartsWithControlChar1e() { + testHeaderNameStartsWithControlChar(0x1e); + } + + @Test + public void testHeaderNameStartsWithControlChar1f() { + testHeaderNameStartsWithControlChar(0x1f); + } + + @Test + public void testHeaderNameStartsWithControlChar0c() { + testHeaderNameStartsWithControlChar(0x0c); + } + + private void testHeaderNameStartsWithControlChar(int controlChar) { + ByteBuf requestBuffer = Unpooled.buffer(); + requestBuffer.writeCharSequence("GET /some/path HTTP/1.1\r\n" + + "Host: netty.io\r\n", CharsetUtil.US_ASCII); + requestBuffer.writeByte(controlChar); + requestBuffer.writeCharSequence("Transfer-Encoding: chunked\r\n\r\n", CharsetUtil.US_ASCII); + testInvalidHeaders0(requestBuffer); + } + + @Test + public void testHeaderNameEndsWithControlChar1c() { + testHeaderNameEndsWithControlChar(0x1c); + } + + @Test + public void testHeaderNameEndsWithControlChar1d() { + testHeaderNameEndsWithControlChar(0x1d); + } + + @Test + public void testHeaderNameEndsWithControlChar1e() { + testHeaderNameEndsWithControlChar(0x1e); + } + + @Test + public void testHeaderNameEndsWithControlChar1f() { + testHeaderNameEndsWithControlChar(0x1f); + } + + @Test + public void testHeaderNameEndsWithControlChar0c() { + testHeaderNameEndsWithControlChar(0x0c); + } + + private void testHeaderNameEndsWithControlChar(int controlChar) { + ByteBuf requestBuffer = Unpooled.buffer(); + requestBuffer.writeCharSequence("GET /some/path HTTP/1.1\r\n" + + "Host: netty.io\r\n", CharsetUtil.US_ASCII); + requestBuffer.writeCharSequence("Transfer-Encoding", CharsetUtil.US_ASCII); + requestBuffer.writeByte(controlChar); + requestBuffer.writeCharSequence(": chunked\r\n\r\n", CharsetUtil.US_ASCII); + testInvalidHeaders0(requestBuffer); + } + @Test public void testWhitespace() { String requestStr = "GET /some/path HTTP/1.1\r\n" + @@ -366,9 +436,9 @@ public void testWhitespace() { } @Test - public void testWhitespaceBeforeTransferEncoding01() { + public void testWhitespaceInTransferEncoding01() { String requestStr = "GET /some/path HTTP/1.1\r\n" + - " Transfer-Encoding : chunked\r\n" + + "Transfer-Encoding : chunked\r\n" + "Content-Length: 1\r\n" + "Host: netty.io\r\n\r\n" + "a"; @@ -376,9 +446,9 @@ public void testWhitespaceBeforeTransferEncoding01() { } @Test - public void testWhitespaceBeforeTransferEncoding02() { + public void testWhitespaceInTransferEncoding02() { String requestStr = "POST / HTTP/1.1" + - " Transfer-Encoding : chunked\r\n" + + "Transfer-Encoding : chunked\r\n" + "Host: target.com" + "Content-Length: 65\r\n\r\n" + "0\r\n\r\n" + @@ -475,6 +545,7 @@ public void testContentLengthHeaderAndChunked() { assertTrue(request.headers().contains("Transfer-Encoding", "chunked", false)); assertFalse(request.headers().contains("Content-Length")); LastHttpContent c = channel.readInbound(); + c.release(); assertFalse(channel.finish()); } @@ -499,11 +570,15 @@ public void testHttpMessageDecoderResult() { } private static void testInvalidHeaders0(String requestStr) { + testInvalidHeaders0(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)); + } + + private static void testInvalidHeaders0(ByteBuf requestBuffer) { EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder()); - assertTrue(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII))); + assertTrue(channel.writeInbound(requestBuffer)); HttpRequest request = channel.readInbound(); + assertThat(request.decoderResult().cause(), instanceOf(IllegalArgumentException.class)); assertTrue(request.decoderResult().isFailure()); - assertTrue(request.decoderResult().cause() instanceof IllegalArgumentException); assertFalse(channel.finish()); } } 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 de936389255..7d4fe92a5d6 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 @@ -799,4 +799,82 @@ public void testHttpMessageDecoderResult() { c.release(); assertFalse(channel.finish()); } + + @Test + public void testHeaderNameStartsWithControlChar1c() { + testHeaderNameStartsWithControlChar(0x1c); + } + + @Test + public void testHeaderNameStartsWithControlChar1d() { + testHeaderNameStartsWithControlChar(0x1d); + } + + @Test + public void testHeaderNameStartsWithControlChar1e() { + testHeaderNameStartsWithControlChar(0x1e); + } + + @Test + public void testHeaderNameStartsWithControlChar1f() { + testHeaderNameStartsWithControlChar(0x1f); + } + + @Test + public void testHeaderNameStartsWithControlChar0c() { + testHeaderNameStartsWithControlChar(0x0c); + } + + private void testHeaderNameStartsWithControlChar(int controlChar) { + ByteBuf responseBuffer = Unpooled.buffer(); + responseBuffer.writeCharSequence("HTTP/1.1 200 OK\r\n" + + "Host: netty.io\r\n", CharsetUtil.US_ASCII); + responseBuffer.writeByte(controlChar); + responseBuffer.writeCharSequence("Transfer-Encoding: chunked\r\n\r\n", CharsetUtil.US_ASCII); + testInvalidHeaders0(responseBuffer); + } + + @Test + public void testHeaderNameEndsWithControlChar1c() { + testHeaderNameEndsWithControlChar(0x1c); + } + + @Test + public void testHeaderNameEndsWithControlChar1d() { + testHeaderNameEndsWithControlChar(0x1d); + } + + @Test + public void testHeaderNameEndsWithControlChar1e() { + testHeaderNameEndsWithControlChar(0x1e); + } + + @Test + public void testHeaderNameEndsWithControlChar1f() { + testHeaderNameEndsWithControlChar(0x1f); + } + + @Test + public void testHeaderNameEndsWithControlChar0c() { + testHeaderNameEndsWithControlChar(0x0c); + } + + private void testHeaderNameEndsWithControlChar(int controlChar) { + ByteBuf responseBuffer = Unpooled.buffer(); + responseBuffer.writeCharSequence("HTTP/1.1 200 OK\r\n" + + "Host: netty.io\r\n", CharsetUtil.US_ASCII); + responseBuffer.writeCharSequence("Transfer-Encoding", CharsetUtil.US_ASCII); + responseBuffer.writeByte(controlChar); + responseBuffer.writeCharSequence(": chunked\r\n\r\n", CharsetUtil.US_ASCII); + testInvalidHeaders0(responseBuffer); + } + + private static void testInvalidHeaders0(ByteBuf responseBuffer) { + EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder()); + assertTrue(channel.writeInbound(responseBuffer)); + HttpResponse response = channel.readInbound(); + assertThat(response.decoderResult().cause(), instanceOf(IllegalArgumentException.class)); + assertTrue(response.decoderResult().isFailure()); + assertFalse(channel.finish()); + } }