From 746d4ca021bb47def860e428187123792bedd5e1 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Sun, 1 Nov 2026 10:14:30 +0100 Subject: [PATCH] RFC 9112: reject signed chunk sizes and Content-Length values Long#parseLong accepts a leading '+' or '-'. The NIO chunk decoder also treated '-1' as 'no chunk head yet' and skipped the line, and failed with IllegalArgumentException on other negative values. Chunk sizes must be 1*HEXDIG and Content-Length 1*DIGIT. Reject an empty value and a leading sign before parsing, in both the blocking and non-blocking code paths. --- .../http/impl/io/ChunkedInputStream.java | 4 ++ .../hc/core5/http/impl/nio/ChunkDecoder.java | 4 ++ .../hc/core5/http/message/MessageSupport.java | 16 +++-- .../TestDefaultContentLengthStrategy.java | 9 +++ .../core5/http/impl/io/TestChunkCoding.java | 54 +++++++++++++++++ .../core5/http/impl/nio/TestChunkDecoder.java | 60 +++++++++++++++++++ .../http/message/TestMessageSupport.java | 51 ++++++++++++++++ 7 files changed, 193 insertions(+), 5 deletions(-) diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/ChunkedInputStream.java b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/ChunkedInputStream.java index 45e8456c0d..21ad612b52 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/ChunkedInputStream.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/ChunkedInputStream.java @@ -38,6 +38,7 @@ import org.apache.hc.core5.http.TruncatedChunkException; import org.apache.hc.core5.http.config.Http1Config; import org.apache.hc.core5.http.io.SessionInputBuffer; +import org.apache.hc.core5.http.message.MessageSupport; import org.apache.hc.core5.util.Args; import org.apache.hc.core5.util.CharArrayBuffer; @@ -269,6 +270,9 @@ private long getChunkSize() throws IOException { separator = lineBuffer.length(); } final String s = this.lineBuffer.substringTrimmed(0, separator); + if (MessageSupport.isSignedOrEmpty(s)) { + throw new MalformedChunkCodingException("Bad chunk header: " + s); + } try { return Long.parseLong(s, 16); } catch (final NumberFormatException e) { diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/ChunkDecoder.java b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/ChunkDecoder.java index 84a8c9a4f0..935235cc74 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/ChunkDecoder.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/ChunkDecoder.java @@ -42,6 +42,7 @@ import org.apache.hc.core5.http.config.Http1Config; import org.apache.hc.core5.http.impl.BasicHttpTransportMetrics; import org.apache.hc.core5.http.message.BufferedHeader; +import org.apache.hc.core5.http.message.MessageSupport; import org.apache.hc.core5.http.nio.SessionInputBuffer; import org.apache.hc.core5.util.Args; import org.apache.hc.core5.util.CharArrayBuffer; @@ -128,6 +129,9 @@ private void readChunkHead() throws IOException { separator = this.lineBuf.length(); } final String s = this.lineBuf.substringTrimmed(0, separator); + if (MessageSupport.isSignedOrEmpty(s)) { + throw new MalformedChunkCodingException("Bad chunk header: " + s); + } try { this.chunkSize = Long.parseLong(s, 16); } catch (final NumberFormatException e) { diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/message/MessageSupport.java b/httpcore5/src/main/java/org/apache/hc/core5/http/message/MessageSupport.java index 55acd6fa8e..3d0779fc0b 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/message/MessageSupport.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/http/message/MessageSupport.java @@ -732,6 +732,13 @@ public static Set hopByHopConnectionSpecific(final MessageHeaders header return HOP_BY_HOP; } + /** + * @since 5.5 + */ + public static boolean isSignedOrEmpty(final String s) { + return s == null || s.isEmpty() || s.charAt(0) == '+' || s.charAt(0) == '-'; + } + /** * @since 5.5 */ @@ -743,12 +750,11 @@ public static long getContentLength(final HttpMessage message) throws HttpExcept final Header contentLengthHeader = message.getFirstHeader(HttpHeaders.CONTENT_LENGTH); if (contentLengthHeader != null) { final String s = contentLengthHeader.getValue(); + if (isSignedOrEmpty(s)) { + throw new ProtocolException("Invalid content length: " + s); + } try { - final long len = Long.parseLong(s); - if (len < 0) { - throw new ProtocolException("Negative content length: " + s); - } - return len; + return Long.parseLong(s); } catch (final NumberFormatException e) { throw new ProtocolException("Invalid content length: " + s); } diff --git a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/TestDefaultContentLengthStrategy.java b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/TestDefaultContentLengthStrategy.java index c9e5eeea53..fa7b51cae4 100644 --- a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/TestDefaultContentLengthStrategy.java +++ b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/TestDefaultContentLengthStrategy.java @@ -153,6 +153,15 @@ void testEntityWithNegativeContentLength() { lenStrategy.determineLength(message)); } + @Test + void testEntityWithPlusSignedContentLength() { + final ContentLengthStrategy lenStrategy = new DefaultContentLengthStrategy(); + final HttpMessage message = new TestHttpMessage(); + message.addHeader("Content-Length", "+5"); + Assertions.assertThrows(ProtocolException.class, () -> + lenStrategy.determineLength(message)); + } + @Test void testEntityNoContentDelimiter() throws Exception { final ContentLengthStrategy lenStrategy = new DefaultContentLengthStrategy(); diff --git a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestChunkCoding.java b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestChunkCoding.java index 7a4a976457..7b63bec232 100644 --- a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestChunkCoding.java +++ b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestChunkCoding.java @@ -219,6 +219,60 @@ void testCorruptChunkedInputStreamInvalidSize() throws IOException { } } + @Test + void testCorruptChunkedInputStreamSignedSize() throws IOException { + final String s = "+5\r\n01234\r\n0\r\n\r\n"; + final SessionInputBuffer inBuffer = new SessionInputBufferImpl(16); + final ByteArrayInputStream inputStream = new ByteArrayInputStream(s.getBytes(StandardCharsets.US_ASCII)); + try (ChunkedInputStream in = new ChunkedInputStream(inBuffer, inputStream)) { + Assertions.assertThrows(MalformedChunkCodingException.class, in::read); + } + } + + @Test + void testCorruptChunkedInputStreamMinusZeroSize() throws IOException { + final String s = "-0\r\n\r\n"; + final SessionInputBuffer inBuffer = new SessionInputBufferImpl(16); + final ByteArrayInputStream inputStream = new ByteArrayInputStream(s.getBytes(StandardCharsets.US_ASCII)); + try (ChunkedInputStream in = new ChunkedInputStream(inBuffer, inputStream)) { + Assertions.assertThrows(MalformedChunkCodingException.class, in::read); + } + } + + @Test + void testCorruptChunkedInputStreamOverflowSize() throws IOException { + final String s = "FFFFFFFFFFFFFFFFF\r\n01234\r\n0\r\n\r\n"; + final SessionInputBuffer inBuffer = new SessionInputBufferImpl(16); + final ByteArrayInputStream inputStream = new ByteArrayInputStream(s.getBytes(StandardCharsets.US_ASCII)); + try (ChunkedInputStream in = new ChunkedInputStream(inBuffer, inputStream)) { + Assertions.assertThrows(MalformedChunkCodingException.class, in::read); + } + } + + @Test + void testChunkedInputStreamSurroundingWhitespaceSize() throws IOException { + final String s = " 5 \r\n01234\r\n0\r\n\r\n"; + final SessionInputBuffer inBuffer = new SessionInputBufferImpl(16); + final ByteArrayInputStream inputStream = new ByteArrayInputStream(s.getBytes(StandardCharsets.US_ASCII)); + try (ChunkedInputStream in = new ChunkedInputStream(inBuffer, inputStream)) { + final byte[] tmp = new byte[5]; + Assertions.assertEquals(5, in.read(tmp)); + Assertions.assertEquals("01234", new String(tmp, StandardCharsets.US_ASCII)); + } + } + + @Test + void testChunkedInputStreamLeadingZerosSize() throws IOException { + final String s = "0005\r\n01234\r\n0\r\n\r\n"; + final SessionInputBuffer inBuffer = new SessionInputBufferImpl(16); + final ByteArrayInputStream inputStream = new ByteArrayInputStream(s.getBytes(StandardCharsets.US_ASCII)); + try (ChunkedInputStream in = new ChunkedInputStream(inBuffer, inputStream)) { + final byte[] tmp = new byte[5]; + Assertions.assertEquals(5, in.read(tmp)); + Assertions.assertEquals("01234", new String(tmp, StandardCharsets.US_ASCII)); + } + } + // Negative chunk size @Test void testCorruptChunkedInputStreamNegativeSize() throws IOException { diff --git a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/nio/TestChunkDecoder.java b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/nio/TestChunkDecoder.java index 4e6eed6ea2..360a2e3ece 100644 --- a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/nio/TestChunkDecoder.java +++ b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/nio/TestChunkDecoder.java @@ -163,6 +163,66 @@ void testMalformedChunk() { Assertions.assertThrows(MalformedChunkCodingException.class, () -> decoder.read(dst)); } + private static void decode(final String s) throws Exception { + final ReadableByteChannel channel = new ReadableByteChannelMock( + new String[] {s}, StandardCharsets.US_ASCII); + final SessionInputBuffer inbuf = new SessionInputBufferImpl(1024, 256, 0, StandardCharsets.US_ASCII); + final ChunkDecoder decoder = new ChunkDecoder(channel, inbuf, new BasicHttpTransportMetrics()); + final ByteBuffer dst = ByteBuffer.allocate(1024); + for (int i = 0; i < 10 && !decoder.isCompleted(); i++) { + dst.clear(); + if (decoder.read(dst) == -1) { + break; + } + } + } + + @Test + void testChunkSizeMinusOneRejected() { + Assertions.assertThrows(MalformedChunkCodingException.class, + () -> decode("-1\r\n5\r\nhello\r\n0\r\n\r\n")); + } + + @Test + void testChunkSizeNegativeRejected() { + Assertions.assertThrows(MalformedChunkCodingException.class, + () -> decode("-5\r\nhello\r\n0\r\n\r\n")); + } + + @Test + void testChunkSizePlusSignRejected() { + Assertions.assertThrows(MalformedChunkCodingException.class, + () -> decode("+5\r\nhello\r\n0\r\n\r\n")); + } + + @Test + void testChunkSizeMinusZeroRejected() { + Assertions.assertThrows(MalformedChunkCodingException.class, + () -> decode("-0\r\n\r\n")); + } + + @Test + void testChunkSizeBlankRejected() { + Assertions.assertThrows(MalformedChunkCodingException.class, + () -> decode(" \r\nhello\r\n0\r\n\r\n")); + } + + @Test + void testChunkSizeOverflowRejected() { + Assertions.assertThrows(MalformedChunkCodingException.class, + () -> decode("FFFFFFFFFFFFFFFFF\r\nhello\r\n0\r\n\r\n")); + } + + @Test + void testChunkSizeSurroundingWhitespaceAccepted() throws Exception { + decode(" 5 \r\nhello\r\n0\r\n\r\n"); + } + + @Test + void testChunkSizeLeadingZerosAccepted() throws Exception { + decode("0005\r\nhello\r\n0\r\n\r\n"); + } + @Test void testIncompleteChunkDecoding() throws Exception { final String[] chunks = { diff --git a/httpcore5/src/test/java/org/apache/hc/core5/http/message/TestMessageSupport.java b/httpcore5/src/test/java/org/apache/hc/core5/http/message/TestMessageSupport.java index d30d4e7699..174a07c3d2 100644 --- a/httpcore5/src/test/java/org/apache/hc/core5/http/message/TestMessageSupport.java +++ b/httpcore5/src/test/java/org/apache/hc/core5/http/message/TestMessageSupport.java @@ -570,6 +570,57 @@ void testMessageWithNegativeContentLength() { MessageSupport.getContentLength(message)); } + @Test + void testMessageWithPlusSignedContentLength() { + final HttpMessage message = new BasicHttpResponse(200); + message.addHeader("Content-Length", "+5"); + Assertions.assertThrows(ProtocolException.class, () -> + MessageSupport.getContentLength(message)); + } + + @Test + void testMessageWithEmptyContentLength() { + final HttpMessage message = new BasicHttpResponse(200); + message.addHeader("Content-Length", ""); + Assertions.assertThrows(ProtocolException.class, () -> + MessageSupport.getContentLength(message)); + } + + @Test + void testIsSignedOrEmpty() { + Assertions.assertTrue(MessageSupport.isSignedOrEmpty(null)); + Assertions.assertTrue(MessageSupport.isSignedOrEmpty("")); + Assertions.assertTrue(MessageSupport.isSignedOrEmpty("+5")); + Assertions.assertTrue(MessageSupport.isSignedOrEmpty("-5")); + Assertions.assertTrue(MessageSupport.isSignedOrEmpty("-0")); + Assertions.assertFalse(MessageSupport.isSignedOrEmpty("5")); + Assertions.assertFalse(MessageSupport.isSignedOrEmpty("0005")); + Assertions.assertFalse(MessageSupport.isSignedOrEmpty("a+")); + } + + @Test + void testMessageWithMinusZeroContentLength() { + final HttpMessage message = new BasicHttpResponse(200); + message.addHeader("Content-Length", "-0"); + Assertions.assertThrows(ProtocolException.class, () -> + MessageSupport.getContentLength(message)); + } + + @Test + void testMessageWithOverflowContentLength() { + final HttpMessage message = new BasicHttpResponse(200); + message.addHeader("Content-Length", "99999999999999999999"); + Assertions.assertThrows(ProtocolException.class, () -> + MessageSupport.getContentLength(message)); + } + + @Test + void testMessageWithLeadingZerosContentLength() throws Exception { + final HttpMessage message = new BasicHttpResponse(200); + message.addHeader("Content-Length", "005"); + Assertions.assertEquals(5, MessageSupport.getContentLength(message)); + } + @Test void testMessageWithMultipleContentLength() throws Exception { final HttpMessage message = new BasicHttpResponse(200);