From c622a76ad4d988c10124ed4f175fdba491b58055 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Tue, 24 Feb 2026 13:17:23 +0100 Subject: [PATCH] HTTP/2: ignore unused PADDED flag on non-padded frames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raw frame readers were applying PADDED semantics whenever flag 0x08 was set, even for frame types that do not define padding, which violates RFC 9113. This change gates padding validation to DATA, HEADERS, and PUSH_PROMISE only, and reads the Pad Length as an unsigned octet in the NIO path. RFC says: “Unused flags MUST be ignored on receipt and MUST be left unset (0x00) when sending." --- .../core5/http2/impl/io/FrameInputBuffer.java | 9 ++++- .../http2/impl/nio/FrameInputBuffer.java | 11 ++++- .../http2/impl/io/TestFrameInOutBuffers.java | 32 +++++++++++++++ .../http2/impl/nio/TestFrameInputBuffer.java | 40 +++++++++++++++++++ 4 files changed, 89 insertions(+), 3 deletions(-) diff --git a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/io/FrameInputBuffer.java b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/io/FrameInputBuffer.java index 376f26d1ac..716f7b74fe 100644 --- a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/io/FrameInputBuffer.java +++ b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/io/FrameInputBuffer.java @@ -38,6 +38,7 @@ import org.apache.hc.core5.http2.H2TransportMetrics; import org.apache.hc.core5.http2.frame.FrameConsts; import org.apache.hc.core5.http2.frame.FrameFlag; +import org.apache.hc.core5.http2.frame.FrameType; import org.apache.hc.core5.http2.frame.RawFrame; import org.apache.hc.core5.http2.impl.BasicH2TransportMetrics; import org.apache.hc.core5.util.Args; @@ -112,7 +113,7 @@ public RawFrame read(final InputStream inStream) throws IOException { final int frameLen = payloadOff + payloadLen; fillBuffer(inStream, frameLen); - if ((flags & FrameFlag.PADDED.getValue()) > 0) { + if ((flags & FrameFlag.PADDED.getValue()) > 0 && isPaddedFrameType(type)) { if (payloadLen == 0) { throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, "Inconsistent padding"); } @@ -137,4 +138,10 @@ public H2TransportMetrics getMetrics() { return metrics; } + private static boolean isPaddedFrameType(final int type) { + return FrameType.DATA.same(type) + || FrameType.HEADERS.same(type) + || FrameType.PUSH_PROMISE.same(type); + } + } diff --git a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/FrameInputBuffer.java b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/FrameInputBuffer.java index 009f75d3dc..b1fb480224 100644 --- a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/FrameInputBuffer.java +++ b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/FrameInputBuffer.java @@ -36,6 +36,7 @@ import org.apache.hc.core5.http2.H2TransportMetrics; import org.apache.hc.core5.http2.frame.FrameConsts; import org.apache.hc.core5.http2.frame.FrameFlag; +import org.apache.hc.core5.http2.frame.FrameType; import org.apache.hc.core5.http2.frame.RawFrame; import org.apache.hc.core5.http2.impl.BasicH2TransportMetrics; import org.apache.hc.core5.util.Args; @@ -150,12 +151,12 @@ public RawFrame read(final ByteBuffer src, final ReadableByteChannel channel) th } case PAYLOAD_EXPECTED: if (buffer.remaining() >= payloadLen) { - if ((flags & FrameFlag.PADDED.getValue()) > 0) { + if ((flags & FrameFlag.PADDED.getValue()) > 0 && isPaddedFrameType(type)) { if (payloadLen == 0) { throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, "Inconsistent padding"); } buffer.mark(); - final int padding = buffer.get(); + final int padding = buffer.get() & 0xff; if (payloadLen < padding + 1) { throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, "Inconsistent padding"); } @@ -204,6 +205,12 @@ public RawFrame read(final ReadableByteChannel channel) throws IOException { return read(null, channel); } + private static boolean isPaddedFrameType(final int type) { + return FrameType.DATA.same(type) + || FrameType.HEADERS.same(type) + || FrameType.PUSH_PROMISE.same(type); + } + public void reset() { buffer.compact(); state = State.HEAD_EXPECTED; diff --git a/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/io/TestFrameInOutBuffers.java b/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/io/TestFrameInOutBuffers.java index 617f599828..4af0dc6b1e 100644 --- a/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/io/TestFrameInOutBuffers.java +++ b/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/io/TestFrameInOutBuffers.java @@ -34,6 +34,7 @@ import org.apache.hc.core5.http.ConnectionClosedException; import org.apache.hc.core5.http2.H2ConnectionException; import org.apache.hc.core5.http2.H2CorruptFrameException; +import org.apache.hc.core5.http2.H2Error; import org.apache.hc.core5.http2.frame.FrameConsts; import org.apache.hc.core5.http2.frame.FrameFlag; import org.apache.hc.core5.http2.frame.FrameType; @@ -244,5 +245,36 @@ void testReadFrameExceedingLimit() { Assertions.assertThrows(H2ConnectionException.class, () -> inBuffer.read(inputStream)); } + @Test + void testPaddedBitOnUnknownFrameTypeIgnored() throws Exception { + final FrameInputBuffer inBuffer = new FrameInputBuffer(16 * 1024); + final ByteArrayInputStream inputStream = new ByteArrayInputStream( + new byte[] { + 0, 0, 1, 10, 8, 0, 0, 0, 1, 42 // len=1,type=0x0a(flags=0x08),stream=1,payload=42 + }); + + final RawFrame frame = inBuffer.read(inputStream); + Assertions.assertNotNull(frame); + Assertions.assertEquals(0x0a, frame.getType()); + Assertions.assertEquals(0x08, frame.getFlags()); + Assertions.assertEquals(1, frame.getStreamId()); + Assertions.assertEquals(1, frame.getLength()); + Assertions.assertNotNull(frame.getPayload()); + Assertions.assertEquals(1, frame.getPayload().remaining()); + Assertions.assertEquals(42, frame.getPayload().get() & 0xff); + } + + @Test + void testPaddedValidationAppliedForDataFrame() { + final FrameInputBuffer inBuffer = new FrameInputBuffer(16 * 1024); + final ByteArrayInputStream inputStream = new ByteArrayInputStream( + new byte[] { + 0, 0, 1, 0, 8, 0, 0, 0, 1, 42 // len=1,type=DATA(flags=0x08),stream=1,padLength=42 -> invalid + }); + + final H2ConnectionException ex = Assertions.assertThrows(H2ConnectionException.class, () -> inBuffer.read(inputStream)); + Assertions.assertEquals(H2Error.PROTOCOL_ERROR.getCode(), ex.getCode()); + } + } diff --git a/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestFrameInputBuffer.java b/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestFrameInputBuffer.java index 88f07e2400..baa29c6389 100644 --- a/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestFrameInputBuffer.java +++ b/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestFrameInputBuffer.java @@ -106,4 +106,44 @@ void streamIdReservedBitMustBeIgnored() throws Exception { assertNotNull(rawFrame); assertEquals(1, rawFrame.getStreamId()); } + + @Test + void paddedBitOnUnknownFrameTypeMustBeIgnored() throws Exception { + final byte[] frame = new byte[]{ + 0x00, 0x00, 0x01, 0x0a, // length=1 + type=0x0a (unknown) + 0x08, // PADDED bit set (undefined for this type) + 0x00, 0x00, 0x00, 0x01, // streamId = 1 + 0x2a // payload byte + }; + + final FrameInputBuffer inBuf = new FrameInputBuffer(16 * 1024); + final ReadableByteChannel ch = Channels.newChannel(new ByteArrayInputStream(frame)); + + final RawFrame rawFrame = inBuf.read(ch); + assertNotNull(rawFrame); + assertEquals(0x0a, rawFrame.getType()); + assertEquals(0x08, rawFrame.getFlags()); + assertEquals(1, rawFrame.getStreamId()); + assertEquals(1, rawFrame.getLength()); + assertNotNull(rawFrame.getPayload()); + assertEquals(1, rawFrame.getPayload().remaining()); + assertEquals(0x2a, rawFrame.getPayload().get() & 0xff); + } + + @Test + void paddedSemanticsStillApplyToDataFrames() throws Exception { + // DATA frame with PADDED flag and payloadLen=1, padding=0x2a -> invalid. + // This ensures the padded-validation branch is still entered for valid frame types. + final byte[] frame = new byte[]{ + 0x00, 0x00, 0x01, 0x00, // length=1 + type=DATA + 0x08, // PADDED + 0x00, 0x00, 0x00, 0x01, // streamId = 1 + 0x2a // pad length byte (invalid for payloadLen=1) + }; + + final FrameInputBuffer inBuf = new FrameInputBuffer(16 * 1024); + final ReadableByteChannel ch = Channels.newChannel(new ByteArrayInputStream(frame)); + final H2ConnectionException ex = assertThrows(H2ConnectionException.class, () -> inBuf.read(ch)); + assertEquals(H2Error.PROTOCOL_ERROR.getCode(), ex.getCode()); + } }