Skip to content

Commit

Permalink
Merge pull request from GHSA-wx5j-54mm-rqqq
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@normanmaurer
normanmaurer committed Dec 9, 2021
1 parent d80a34e commit 07aa6b5
Show file tree
Hide file tree
Showing 4 changed files with 171 additions and 10 deletions.
8 changes: 8 additions & 0 deletions codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java
View file
Expand Up @@ -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':
Expand All @@ -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':
Expand Down
8 changes: 4 additions & 4 deletions codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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) + ")");
Expand Down
87 changes: 81 additions & 6 deletions codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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" +
Expand All @@ -366,19 +436,19 @@ 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";
testInvalidHeaders0(requestStr);
}

@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" +
Expand Down Expand Up @@ -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());
}

Expand All @@ -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());
}
}
78 changes: 78 additions & 0 deletions codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java
View file
Expand Up @@ -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());
}
}

0 comments on commit 07aa6b5

Please sign in to comment.