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 376f26d1a..716f7b74f 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 009f75d3d..b1fb48022 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 617f59982..4af0dc6b1 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 88f07e240..baa29c638 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()); + } }