Skip to content

Commit edb6a36

Browse files
arturobernalgarturobernalg
authored
HTTP/2: ignore unused PADDED flag on non-padded frames (#640)
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."
1 parent 09a80d5 commit edb6a36

4 files changed

Lines changed: 89 additions & 3 deletions

File tree

‎httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/io/FrameInputBuffer.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import org.apache.hc.core5.http2.H2TransportMetrics;
3939
import org.apache.hc.core5.http2.frame.FrameConsts;
4040
import org.apache.hc.core5.http2.frame.FrameFlag;
41+
import org.apache.hc.core5.http2.frame.FrameType;
4142
import org.apache.hc.core5.http2.frame.RawFrame;
4243
import org.apache.hc.core5.http2.impl.BasicH2TransportMetrics;
4344
import org.apache.hc.core5.util.Args;
@@ -112,7 +113,7 @@ public RawFrame read(final InputStream inStream) throws IOException {
112113
final int frameLen = payloadOff + payloadLen;
113114
fillBuffer(inStream, frameLen);
114115

115-
if ((flags & FrameFlag.PADDED.getValue()) > 0) {
116+
if ((flags & FrameFlag.PADDED.getValue()) > 0 && isPaddedFrameType(type)) {
116117
if (payloadLen == 0) {
117118
throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, "Inconsistent padding");
118119
}
@@ -137,4 +138,10 @@ public H2TransportMetrics getMetrics() {
137138
return metrics;
138139
}
139140

141+
private static boolean isPaddedFrameType(final int type) {
142+
return FrameType.DATA.same(type)
143+
|| FrameType.HEADERS.same(type)
144+
|| FrameType.PUSH_PROMISE.same(type);
145+
}
146+
140147
}

‎httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/FrameInputBuffer.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.apache.hc.core5.http2.H2TransportMetrics;
3737
import org.apache.hc.core5.http2.frame.FrameConsts;
3838
import org.apache.hc.core5.http2.frame.FrameFlag;
39+
import org.apache.hc.core5.http2.frame.FrameType;
3940
import org.apache.hc.core5.http2.frame.RawFrame;
4041
import org.apache.hc.core5.http2.impl.BasicH2TransportMetrics;
4142
import org.apache.hc.core5.util.Args;
@@ -150,12 +151,12 @@ public RawFrame read(final ByteBuffer src, final ReadableByteChannel channel) th
150151
}
151152
case PAYLOAD_EXPECTED:
152153
if (buffer.remaining() >= payloadLen) {
153-
if ((flags & FrameFlag.PADDED.getValue()) > 0) {
154+
if ((flags & FrameFlag.PADDED.getValue()) > 0 && isPaddedFrameType(type)) {
154155
if (payloadLen == 0) {
155156
throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, "Inconsistent padding");
156157
}
157158
buffer.mark();
158-
final int padding = buffer.get();
159+
final int padding = buffer.get() & 0xff;
159160
if (payloadLen < padding + 1) {
160161
throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, "Inconsistent padding");
161162
}
@@ -204,6 +205,12 @@ public RawFrame read(final ReadableByteChannel channel) throws IOException {
204205
return read(null, channel);
205206
}
206207

208+
private static boolean isPaddedFrameType(final int type) {
209+
return FrameType.DATA.same(type)
210+
|| FrameType.HEADERS.same(type)
211+
|| FrameType.PUSH_PROMISE.same(type);
212+
}
213+
207214
public void reset() {
208215
buffer.compact();
209216
state = State.HEAD_EXPECTED;

‎httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/io/TestFrameInOutBuffers.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import org.apache.hc.core5.http.ConnectionClosedException;
3535
import org.apache.hc.core5.http2.H2ConnectionException;
3636
import org.apache.hc.core5.http2.H2CorruptFrameException;
37+
import org.apache.hc.core5.http2.H2Error;
3738
import org.apache.hc.core5.http2.frame.FrameConsts;
3839
import org.apache.hc.core5.http2.frame.FrameFlag;
3940
import org.apache.hc.core5.http2.frame.FrameType;
@@ -244,5 +245,36 @@ void testReadFrameExceedingLimit() {
244245
Assertions.assertThrows(H2ConnectionException.class, () -> inBuffer.read(inputStream));
245246
}
246247

248+
@Test
249+
void testPaddedBitOnUnknownFrameTypeIgnored() throws Exception {
250+
final FrameInputBuffer inBuffer = new FrameInputBuffer(16 * 1024);
251+
final ByteArrayInputStream inputStream = new ByteArrayInputStream(
252+
new byte[] {
253+
0, 0, 1, 10, 8, 0, 0, 0, 1, 42 // len=1,type=0x0a(flags=0x08),stream=1,payload=42
254+
});
255+
256+
final RawFrame frame = inBuffer.read(inputStream);
257+
Assertions.assertNotNull(frame);
258+
Assertions.assertEquals(0x0a, frame.getType());
259+
Assertions.assertEquals(0x08, frame.getFlags());
260+
Assertions.assertEquals(1, frame.getStreamId());
261+
Assertions.assertEquals(1, frame.getLength());
262+
Assertions.assertNotNull(frame.getPayload());
263+
Assertions.assertEquals(1, frame.getPayload().remaining());
264+
Assertions.assertEquals(42, frame.getPayload().get() & 0xff);
265+
}
266+
267+
@Test
268+
void testPaddedValidationAppliedForDataFrame() {
269+
final FrameInputBuffer inBuffer = new FrameInputBuffer(16 * 1024);
270+
final ByteArrayInputStream inputStream = new ByteArrayInputStream(
271+
new byte[] {
272+
0, 0, 1, 0, 8, 0, 0, 0, 1, 42 // len=1,type=DATA(flags=0x08),stream=1,padLength=42 -> invalid
273+
});
274+
275+
final H2ConnectionException ex = Assertions.assertThrows(H2ConnectionException.class, () -> inBuffer.read(inputStream));
276+
Assertions.assertEquals(H2Error.PROTOCOL_ERROR.getCode(), ex.getCode());
277+
}
278+
247279
}
248280

‎httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestFrameInputBuffer.java‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,4 +106,44 @@ void streamIdReservedBitMustBeIgnored() throws Exception {
106106
assertNotNull(rawFrame);
107107
assertEquals(1, rawFrame.getStreamId());
108108
}
109+
110+
@Test
111+
void paddedBitOnUnknownFrameTypeMustBeIgnored() throws Exception {
112+
final byte[] frame = new byte[]{
113+
0x00, 0x00, 0x01, 0x0a, // length=1 + type=0x0a (unknown)
114+
0x08, // PADDED bit set (undefined for this type)
115+
0x00, 0x00, 0x00, 0x01, // streamId = 1
116+
0x2a // payload byte
117+
};
118+
119+
final FrameInputBuffer inBuf = new FrameInputBuffer(16 * 1024);
120+
final ReadableByteChannel ch = Channels.newChannel(new ByteArrayInputStream(frame));
121+
122+
final RawFrame rawFrame = inBuf.read(ch);
123+
assertNotNull(rawFrame);
124+
assertEquals(0x0a, rawFrame.getType());
125+
assertEquals(0x08, rawFrame.getFlags());
126+
assertEquals(1, rawFrame.getStreamId());
127+
assertEquals(1, rawFrame.getLength());
128+
assertNotNull(rawFrame.getPayload());
129+
assertEquals(1, rawFrame.getPayload().remaining());
130+
assertEquals(0x2a, rawFrame.getPayload().get() & 0xff);
131+
}
132+
133+
@Test
134+
void paddedSemanticsStillApplyToDataFrames() throws Exception {
135+
// DATA frame with PADDED flag and payloadLen=1, padding=0x2a -> invalid.
136+
// This ensures the padded-validation branch is still entered for valid frame types.
137+
final byte[] frame = new byte[]{
138+
0x00, 0x00, 0x01, 0x00, // length=1 + type=DATA
139+
0x08, // PADDED
140+
0x00, 0x00, 0x00, 0x01, // streamId = 1
141+
0x2a // pad length byte (invalid for payloadLen=1)
142+
};
143+
144+
final FrameInputBuffer inBuf = new FrameInputBuffer(16 * 1024);
145+
final ReadableByteChannel ch = Channels.newChannel(new ByteArrayInputStream(frame));
146+
final H2ConnectionException ex = assertThrows(H2ConnectionException.class, () -> inBuf.read(ch));
147+
assertEquals(H2Error.PROTOCOL_ERROR.getCode(), ex.getCode());
148+
}
109149
}

0 commit comments

Comments
 (0)