Skip to content

Commit

Permalink
Issue #2217 "Enhances line terminator for chunk-size in Chunked Trans…
Browse files Browse the repository at this point in the history 
…fer Coding" (#2217)

+ When the org.glassfish.grizzly.http.STRICT_CHUNKED_TRANSFER_CODING_LINE_TERMINATOR_RFC_9112 option is enabled, only CRLF is allowed as the chunk-size line terminator in Chunked Transfer Coding.
+ Added testcase depending on whether option is present or not
  • Loading branch information
@carryel
carryel committed Dec 16, 2024
1 parent 4316ca1 commit 582420d
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 6 deletions.
16 changes: 15 additions & 1 deletion modules/http/src/main/java/org/glassfish/grizzly/http/ChunkedTransferEncoding.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2020 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2024 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
Expand Down Expand Up @@ -49,6 +49,9 @@ public final class ChunkedTransferEncoding implements TransferEncoding {
private static final byte[] LAST_CHUNK_CRLF_BYTES = "0\r\n".getBytes(ASCII_CHARSET);
private static final int[] DEC = HexUtils.getDecBytes();

public static final String STRICT_CHUNKED_TRANSFER_CODING_LINE_TERMINATOR_RFC_9112 = "org.glassfish.grizzly.http.STRICT_CHUNKED_TRANSFER_CODING_LINE_TERMINATOR_RFC_9112";
private static final boolean isStrictChunkedTransferCodingLineTerminatorSet = Boolean.parseBoolean(System.getProperty(STRICT_CHUNKED_TRANSFER_CODING_LINE_TERMINATOR_RFC_9112));

private final int maxHeadersSize;

public ChunkedTransferEncoding(final int maxHeadersSize) {
Expand Down Expand Up @@ -247,6 +250,12 @@ private static boolean parseHttpChunkLength(final HttpPacketParsing httpPacket,
b == Constants.CR || b == Constants.SEMI_COLON) {
parsingState.checkpoint = offset;
} else if (b == Constants.LF) {
if (isStrictChunkedTransferCodingLineTerminatorSet) {
if (parsingState.checkpoint2 == -1 || // no CR
parsingState.checkpoint2 != parsingState.checkpoint) { // not the previous CR or a repetition of a CR
throw new HttpBrokenContentException("Unexpected HTTP chunk header");
}
}
final ContentParsingState contentParsingState = httpPacket.getContentParsingState();
contentParsingState.chunkContentStart = offset + 1;
contentParsingState.chunkLength = value;
Expand All @@ -264,6 +273,11 @@ private static boolean parseHttpChunkLength(final HttpPacketParsing httpPacket,
} else {
throw new HttpBrokenContentException("Unexpected HTTP chunk header");
}
if (isStrictChunkedTransferCodingLineTerminatorSet) {
if (b == Constants.CR && parsingState.checkpoint2 == -1) { // first CR
parsingState.checkpoint2 = offset;
}
}

offset++;
}
Expand Down
73 changes: 68 additions & 5 deletions modules/http/src/test/java/org/glassfish/grizzly/http/ChunkedTransferEncodingTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2020 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2024 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
Expand All @@ -20,6 +20,7 @@
import static java.lang.Boolean.TRUE;
import static java.util.Arrays.asList;
import static java.util.concurrent.TimeUnit.SECONDS;
import static org.glassfish.grizzly.http.ChunkedTransferEncoding.STRICT_CHUNKED_TRANSFER_CODING_LINE_TERMINATOR_RFC_9112;
import static org.glassfish.grizzly.http.HttpCodecFilter.DEFAULT_MAX_HTTP_PACKET_HEADER_SIZE;
import static org.glassfish.grizzly.http.util.MimeHeaders.MAX_NUM_HEADERS_UNBOUNDED;
import static org.glassfish.grizzly.memory.Buffers.EMPTY_BUFFER;
Expand Down Expand Up @@ -90,6 +91,7 @@ static int PORT() {

private final String eol;
private final boolean isChunkWhenParsing;
private final boolean isStrictChunkedTransferCodingLineTerminatorSet;

private TCPNIOTransport transport;
private Connection connection;
Expand Down Expand Up @@ -153,6 +155,8 @@ public void after() throws Exception {
public ChunkedTransferEncodingTest(String eol, boolean isChunkWhenParsing) {
this.eol = eol;
this.isChunkWhenParsing = isChunkWhenParsing;
this.isStrictChunkedTransferCodingLineTerminatorSet =
Boolean.parseBoolean(System.getProperty(STRICT_CHUNKED_TRANSFER_CODING_LINE_TERMINATOR_RFC_9112));
}

@Test
Expand Down Expand Up @@ -258,8 +262,8 @@ public void testSpacesInChunkSizeHeader() throws Exception {
sb.append("POST / HTTP/1.1\r\n");
sb.append("Host: localhost:").append(PORT).append("\r\n");
sb.append("Transfer-Encoding: chunked\r\n\r\n");
sb.append(" ").append(msgLen).append(" ").append(eol).append(msg).append(eol);
sb.append(" 0 ").append(eol).append(eol);
sb.append(" ").append(msgLen).append(" ").append("\r\n").append(msg).append(eol);
sb.append(" 0 ").append("\r\n").append(eol);

Buffer b = Buffers.wrap(DEFAULT_MEMORY_MANAGER, sb.toString(), Charsets.ASCII_CHARSET);
Future f = connection.write(b);
Expand All @@ -268,6 +272,65 @@ public void testSpacesInChunkSizeHeader() throws Exception {
assertTrue(result.get(10, SECONDS));
}

@SuppressWarnings("unchecked")
@Test
public void testVulnerableLineTerminatorInChunkSizeHeader() throws Exception {
StringBuilder sb = new StringBuilder();
String nestedMsg = "XX";
String nestedMsgLen = Integer.toHexString(nestedMsg.length());
sb.append("\r\n");
sb.append("POST /2 HTTP/1.1").append("\r\n");
sb.append("Host: localhost:").append(PORT).append("\r\n");
sb.append("Transfer-Encoding: chunked").append("\r\n");
sb.append("\r\n");
sb.append(nestedMsgLen).append("\r\n");
sb.append(nestedMsg).append("\r\n");
String dummy = sb.toString();
String firstMsg = "A".repeat(dummy.length());
final String firstMsgLen = Integer.toHexString(firstMsg.length());

// original packet
sb = new StringBuilder();
sb.append("POST /1 HTTP/1.1").append("\r\n");
sb.append("Host: localhost:").append(PORT).append("\r\n");
sb.append("Transfer-Encoding: chunked").append("\r\n");
sb.append("\r\n");
sb.append(firstMsgLen).append(';').append('\n').append(firstMsg).append('\n').append('0').append("\r\n");
sb.append(dummy);
sb.append("0").append("\r\n"); // last-chunk
sb.append("\r\n"); // CRLF

final Buffer expectedContent = Buffers.wrap(DEFAULT_MEMORY_MANAGER, firstMsg, ASCII_CHARSET);
httpRequestCheckFilter.setCheckParameters(expectedContent, Collections.<String, Pair<String, String>>emptyMap());
Buffer b = Buffers.wrap(DEFAULT_MEMORY_MANAGER, sb.toString(), Charsets.ASCII_CHARSET);
Future f = connection.write(b);
f.get(5, SECONDS);

Future<Boolean> result;
if (!isStrictChunkedTransferCodingLineTerminatorSet) {
// first msg
result = resultQueue.poll(5, SECONDS);
assertTrue(result.get(2, SECONDS));

// nested msg
result = resultQueue.poll(5, SECONDS);
try {
result.get(2, SECONDS);
fail("Expected AssertError to be thrown on server side");
} catch (ExecutionException ignore) {
}
} else {
// first msg
result = resultQueue.poll(5, SECONDS);
try {
result.get(2, SECONDS);
fail("Expected HttpBrokenContentException to be thrown on server side");
} catch (ExecutionException ee) {
assertEquals(HttpBrokenContentException.class, ee.getCause().getClass());
}
}
}

/**
* Test private method {@link ChunkedTransferEncoding#checkOverflow(long)} via reflection.
*
Expand Down Expand Up @@ -319,10 +382,10 @@ private void doHttpRequestTest(int packetsNum, boolean hasContent, Map<String, P
sb.append(eol);

if (hasContent) {
sb.append("3").append(eol).append("a=0").append(eol).append("4").append(eol).append("&b=1").append(eol);
sb.append("3").append("\r\n").append("a=0").append(eol).append("4").append("\r\n").append("&b=1").append(eol);
}

sb.append("0").append(eol);
sb.append("0").append("\r\n");

for (Entry<String, Pair<String, String>> entry : trailerHeaders.entrySet()) {
final String value = entry.getValue().getFirst();
Expand Down

0 comments on commit 582420d

Please sign in to comment.