Skip to content

Commit 38d26fc

Browse files
ok2cok2c
committed
Bug-fix: fixes a potential 'End Stream Headers' vulnerability and adds test coverage for various HTTP/2 stream reset exploits
1 parent 44b86eb commit 38d26fc

File tree

3 files changed

+282
-12
lines changed

3 files changed

+282
-12
lines changed

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

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -756,7 +756,9 @@ private void consumeFrame(final RawFrame frame) throws HttpException, IOExceptio
756756
H2Stream stream = streamMap.get(streamId);
757757
if (stream == null) {
758758
acceptHeaderFrame();
759-
759+
if (streamId <= lastStreamId.get()) {
760+
throw new H2ConnectionException(H2Error.STREAM_CLOSED, "Stream closed");
761+
}
760762
if (idGenerator.isSameSide(streamId)) {
761763
throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, "Illegal stream id: " + streamId);
762764
}
@@ -814,7 +816,6 @@ private void consumeFrame(final RawFrame frame) throws HttpException, IOExceptio
814816

815817
final H2Stream stream = getValidStream(streamId);
816818
try {
817-
818819
consumeContinuationFrame(frame, stream);
819820
} catch (final H2StreamResetException ex) {
820821
stream.localReset(ex);
@@ -1024,6 +1025,9 @@ private void consumeFrame(final RawFrame frame) throws HttpException, IOExceptio
10241025
}
10251026

10261027
private void consumeDataFrame(final RawFrame frame, final H2Stream stream) throws HttpException, IOException {
1028+
if (stream.isRemoteClosed()) {
1029+
throw new H2StreamResetException(H2Error.STREAM_CLOSED, "Stream already closed");
1030+
}
10271031
final int streamId = stream.getId();
10281032
final ByteBuffer payload = frame.getPayloadContent();
10291033
if (payload != null) {
@@ -1037,9 +1041,6 @@ private void consumeDataFrame(final RawFrame frame, final H2Stream stream) throw
10371041
maximizeWindow(0, connInputWindow);
10381042
}
10391043
}
1040-
if (stream.isRemoteClosed()) {
1041-
throw new H2StreamResetException(H2Error.STREAM_CLOSED, "Stream already closed");
1042-
}
10431044
if (frame.isFlagSet(FrameFlag.END_STREAM)) {
10441045
stream.setRemoteEndStream();
10451046
}
@@ -1081,6 +1082,9 @@ List<Header> decodeHeaders(final ByteBuffer payload) throws HttpException {
10811082
}
10821083

10831084
private void consumeHeaderFrame(final RawFrame frame, final H2Stream stream) throws HttpException, IOException {
1085+
if (stream.isRemoteClosed()) {
1086+
throw new H2StreamResetException(H2Error.STREAM_CLOSED, "Stream already closed");
1087+
}
10841088
final int streamId = stream.getId();
10851089
if (!frame.isFlagSet(FrameFlag.END_HEADERS)) {
10861090
continuation = new Continuation(streamId, frame.getType(), frame.isFlagSet(FrameFlag.END_STREAM));
@@ -1099,9 +1103,6 @@ private void consumeHeaderFrame(final RawFrame frame, final H2Stream stream) thr
10991103
if (streamListener != null) {
11001104
streamListener.onHeaderInput(this, streamId, headers);
11011105
}
1102-
if (stream.isRemoteClosed()) {
1103-
throw new H2StreamResetException(H2Error.STREAM_CLOSED, "Stream already closed");
1104-
}
11051106
if (stream.isLocalReset()) {
11061107
return;
11071108
}
@@ -1115,6 +1116,9 @@ private void consumeHeaderFrame(final RawFrame frame, final H2Stream stream) thr
11151116
}
11161117

11171118
private void consumeContinuationFrame(final RawFrame frame, final H2Stream stream) throws HttpException, IOException {
1119+
if (stream.isRemoteClosed()) {
1120+
throw new H2StreamResetException(H2Error.STREAM_CLOSED, "Stream already closed");
1121+
}
11181122
final int streamId = frame.getStreamId();
11191123
final ByteBuffer payload = frame.getPayload();
11201124
continuation.copyPayload(payload);
@@ -1126,9 +1130,6 @@ private void consumeContinuationFrame(final RawFrame frame, final H2Stream strea
11261130
if (streamListener != null) {
11271131
streamListener.onHeaderInput(this, streamId, headers);
11281132
}
1129-
if (stream.isRemoteClosed()) {
1130-
throw new H2StreamResetException(H2Error.STREAM_CLOSED, "Stream already closed");
1131-
}
11321133
if (stream.isLocalReset()) {
11331134
return;
11341135
}

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

Lines changed: 268 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,9 @@
3030
import java.io.IOException;
3131
import java.nio.ByteBuffer;
3232
import java.util.ArrayList;
33+
import java.util.Arrays;
3334
import java.util.List;
35+
import java.util.concurrent.locks.Lock;
3436

3537
import org.apache.hc.core5.function.Supplier;
3638
import org.apache.hc.core5.http.Header;
@@ -44,6 +46,8 @@
4446
import org.apache.hc.core5.http.nio.command.ExecutableCommand;
4547
import org.apache.hc.core5.http.protocol.HttpProcessor;
4648
import org.apache.hc.core5.http2.H2ConnectionException;
49+
import org.apache.hc.core5.http2.H2Error;
50+
import org.apache.hc.core5.http2.H2StreamResetException;
4751
import org.apache.hc.core5.http2.WritableByteChannelMock;
4852
import org.apache.hc.core5.http2.config.H2Config;
4953
import org.apache.hc.core5.http2.frame.DefaultFrameFactory;
@@ -72,17 +76,22 @@ class TestAbstractH2StreamMultiplexer {
7276
@Mock
7377
ProtocolIOSession protocolIOSession;
7478
@Mock
79+
Lock lock;
80+
@Mock
7581
HttpProcessor httpProcessor;
7682
@Mock
7783
H2StreamListener h2StreamListener;
7884
@Mock
7985
H2StreamHandler streamHandler;
8086
@Captor
8187
ArgumentCaptor<List<Header>> headersCaptor;
88+
@Captor
89+
ArgumentCaptor<Exception> exceptionCaptor;
8290

8391
@BeforeEach
8492
void prepareMocks() {
8593
MockitoAnnotations.openMocks(this);
94+
Mockito.when(protocolIOSession.getLock()).thenReturn(lock);
8695
}
8796

8897
static class H2StreamMultiplexerImpl extends AbstractH2StreamMultiplexer {
@@ -270,5 +279,264 @@ void testInputHeaderContinuationFrame() throws IOException, HttpException {
270279
Mockito.verify(streamHandler).consumeHeader(headersCaptor.capture(), ArgumentMatchers.eq(false));
271280
Assertions.assertFalse(headersCaptor.getValue().isEmpty());
272281
}
282+
283+
@Test
284+
void testZeroIncrement() throws Exception {
285+
final H2Config h2Config = H2Config.custom()
286+
.build();
287+
288+
final AbstractH2StreamMultiplexer streamMultiplexer = new H2StreamMultiplexerImpl(
289+
protocolIOSession,
290+
FRAME_FACTORY,
291+
StreamIdGenerator.EVEN,
292+
httpProcessor,
293+
CharCodingConfig.DEFAULT,
294+
h2Config,
295+
h2StreamListener,
296+
() -> streamHandler);
297+
298+
final ByteArrayBuffer headerBuf = new ByteArrayBuffer(200);
299+
final HPackEncoder encoder = new HPackEncoder(h2Config.getHeaderTableSize(),
300+
CharCodingSupport.createEncoder(CharCodingConfig.DEFAULT));
301+
302+
final List<Header> headers = Arrays.asList(
303+
new BasicHeader(":method", "GET"),
304+
new BasicHeader(":scheme", "http"),
305+
new BasicHeader(":path", "/"),
306+
new BasicHeader(":authority", "www.example.com"));
307+
encoder.encodeHeaders(headerBuf, headers, h2Config.isCompressionEnabled());
308+
309+
final WritableByteChannelMock writableChannel = new WritableByteChannelMock(1024);
310+
final FrameOutputBuffer outBuffer = new FrameOutputBuffer(16 * 1024);
311+
312+
final RawFrame headerFrame = FRAME_FACTORY.createHeaders(1, ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()), true, true);
313+
outBuffer.write(headerFrame, writableChannel);
314+
315+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray()));
316+
317+
Mockito.verify(streamHandler).consumeHeader(headersCaptor.capture(), ArgumentMatchers.eq(true));
318+
Assertions.assertFalse(headersCaptor.getValue().isEmpty());
319+
320+
writableChannel.reset();
321+
final ByteBuffer payload = ByteBuffer.allocate(4);
322+
payload.putInt(0);
323+
payload.flip();
324+
final RawFrame incrementFrame = new RawFrame(FrameType.WINDOW_UPDATE.getValue(), 0, 1, payload);
325+
outBuffer.write(incrementFrame, writableChannel);
326+
327+
final H2ConnectionException exception = Assertions.assertThrows(H2ConnectionException.class, () ->
328+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray())));
329+
Assertions.assertEquals(H2Error.PROTOCOL_ERROR, H2Error.getByCode(exception.getCode()));
330+
}
331+
332+
@Test
333+
void testIncrementOverflow() throws Exception {
334+
final H2Config h2Config = H2Config.custom()
335+
.build();
336+
337+
final AbstractH2StreamMultiplexer streamMultiplexer = new H2StreamMultiplexerImpl(
338+
protocolIOSession,
339+
FRAME_FACTORY,
340+
StreamIdGenerator.EVEN,
341+
httpProcessor,
342+
CharCodingConfig.DEFAULT,
343+
h2Config,
344+
h2StreamListener,
345+
() -> streamHandler);
346+
347+
final ByteArrayBuffer headerBuf = new ByteArrayBuffer(200);
348+
final HPackEncoder encoder = new HPackEncoder(h2Config.getHeaderTableSize(),
349+
CharCodingSupport.createEncoder(CharCodingConfig.DEFAULT));
350+
351+
final List<Header> headers = Arrays.asList(
352+
new BasicHeader(":method", "GET"),
353+
new BasicHeader(":scheme", "http"),
354+
new BasicHeader(":path", "/"),
355+
new BasicHeader(":authority", "www.example.com"));
356+
encoder.encodeHeaders(headerBuf, headers, h2Config.isCompressionEnabled());
357+
358+
final WritableByteChannelMock writableChannel = new WritableByteChannelMock(1024);
359+
final FrameOutputBuffer outBuffer = new FrameOutputBuffer(16 * 1024);
360+
361+
final RawFrame headerFrame = FRAME_FACTORY.createHeaders(1, ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()), true, true);
362+
outBuffer.write(headerFrame, writableChannel);
363+
364+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray()));
365+
366+
Mockito.verify(streamHandler).consumeHeader(headersCaptor.capture(), ArgumentMatchers.eq(true));
367+
Assertions.assertFalse(headersCaptor.getValue().isEmpty());
368+
369+
writableChannel.reset();
370+
final RawFrame incrementFrame1 = FRAME_FACTORY.createWindowUpdate(1, 100);
371+
outBuffer.write(incrementFrame1, writableChannel);
372+
373+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray()));
374+
375+
writableChannel.reset();
376+
final RawFrame incrementFrame2 = FRAME_FACTORY.createWindowUpdate(1, 0x7fffffff - 50);
377+
outBuffer.write(incrementFrame2, writableChannel);
378+
final H2ConnectionException exception = Assertions.assertThrows(H2ConnectionException.class, () ->
379+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray())));
380+
Assertions.assertEquals(H2Error.FLOW_CONTROL_ERROR, H2Error.getByCode(exception.getCode()));
381+
}
382+
383+
@Test
384+
void testHeadersAfterEndOfStream() throws Exception {
385+
final H2Config h2Config = H2Config.custom()
386+
.build();
387+
388+
final AbstractH2StreamMultiplexer streamMultiplexer = new H2StreamMultiplexerImpl(
389+
protocolIOSession,
390+
FRAME_FACTORY,
391+
StreamIdGenerator.EVEN,
392+
httpProcessor,
393+
CharCodingConfig.DEFAULT,
394+
h2Config,
395+
h2StreamListener,
396+
() -> streamHandler);
397+
398+
final ByteArrayBuffer headerBuf = new ByteArrayBuffer(200);
399+
final HPackEncoder encoder = new HPackEncoder(h2Config.getHeaderTableSize(),
400+
CharCodingSupport.createEncoder(CharCodingConfig.DEFAULT));
401+
402+
final List<Header> headers = Arrays.asList(
403+
new BasicHeader(":method", "GET"),
404+
new BasicHeader(":scheme", "http"),
405+
new BasicHeader(":path", "/"),
406+
new BasicHeader(":authority", "www.example.com"));
407+
encoder.encodeHeaders(headerBuf, headers, h2Config.isCompressionEnabled());
408+
409+
final WritableByteChannelMock writableChannel = new WritableByteChannelMock(1024);
410+
final FrameOutputBuffer outBuffer = new FrameOutputBuffer(16 * 1024);
411+
412+
final RawFrame headerFrame1 = FRAME_FACTORY.createHeaders(1, ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()), true, true);
413+
outBuffer.write(headerFrame1, writableChannel);
414+
415+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray()));
416+
417+
Mockito.verify(streamHandler).consumeHeader(headersCaptor.capture(), ArgumentMatchers.eq(true));
418+
Assertions.assertFalse(headersCaptor.getValue().isEmpty());
419+
420+
writableChannel.reset();
421+
final RawFrame headerFrame2 = FRAME_FACTORY.createHeaders(1, ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()), true, true);
422+
outBuffer.write(headerFrame2, writableChannel);
423+
424+
// Treat the first occurrence as a stream error
425+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray()));
426+
Mockito.verify(streamHandler).failed(exceptionCaptor.capture());
427+
Assertions.assertInstanceOf(H2StreamResetException.class, exceptionCaptor.getValue());
428+
429+
writableChannel.reset();
430+
final RawFrame headerFrame3 = FRAME_FACTORY.createHeaders(1, ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()), true, true);
431+
outBuffer.write(headerFrame3, writableChannel);
432+
433+
// Treat subsequent occurrences as a connection-wide protocol error
434+
final H2ConnectionException exception = Assertions.assertThrows(H2ConnectionException.class, () ->
435+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray())));
436+
Assertions.assertEquals(H2Error.STREAM_CLOSED, H2Error.getByCode(exception.getCode()));
437+
}
438+
439+
@Test
440+
void testDataAfterEndOfStream() throws Exception {
441+
final H2Config h2Config = H2Config.custom()
442+
.build();
443+
444+
final AbstractH2StreamMultiplexer streamMultiplexer = new H2StreamMultiplexerImpl(
445+
protocolIOSession,
446+
FRAME_FACTORY,
447+
StreamIdGenerator.EVEN,
448+
httpProcessor,
449+
CharCodingConfig.DEFAULT,
450+
h2Config,
451+
h2StreamListener,
452+
() -> streamHandler);
453+
454+
final ByteArrayBuffer headerBuf = new ByteArrayBuffer(200);
455+
final HPackEncoder encoder = new HPackEncoder(h2Config.getHeaderTableSize(),
456+
CharCodingSupport.createEncoder(CharCodingConfig.DEFAULT));
457+
458+
final List<Header> headers = Arrays.asList(
459+
new BasicHeader(":method", "GET"),
460+
new BasicHeader(":scheme", "http"),
461+
new BasicHeader(":path", "/"),
462+
new BasicHeader(":authority", "www.example.com"));
463+
encoder.encodeHeaders(headerBuf, headers, h2Config.isCompressionEnabled());
464+
465+
final WritableByteChannelMock writableChannel = new WritableByteChannelMock(1024);
466+
final FrameOutputBuffer outBuffer = new FrameOutputBuffer(16 * 1024);
467+
468+
final RawFrame headerFrame1 = FRAME_FACTORY.createHeaders(1, ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()), true, true);
469+
outBuffer.write(headerFrame1, writableChannel);
470+
471+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray()));
472+
473+
Mockito.verify(streamHandler).consumeHeader(headersCaptor.capture(), ArgumentMatchers.eq(true));
474+
Assertions.assertFalse(headersCaptor.getValue().isEmpty());
475+
476+
writableChannel.reset();
477+
final RawFrame dataFrame1 = FRAME_FACTORY.createData(1, ByteBuffer.wrap(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9, 0}), true);
478+
outBuffer.write(dataFrame1, writableChannel);
479+
480+
// Treat the first occurrence as a stream error
481+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray()));
482+
Mockito.verify(streamHandler).failed(exceptionCaptor.capture());
483+
Assertions.assertInstanceOf(H2StreamResetException.class, exceptionCaptor.getValue());
484+
485+
writableChannel.reset();
486+
final RawFrame dataFrame2 = FRAME_FACTORY.createData(1, ByteBuffer.wrap(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9, 0}), true);
487+
outBuffer.write(dataFrame2, writableChannel);
488+
489+
// Treat subsequent occurrences as a connection-wide protocol error
490+
final H2ConnectionException exception = Assertions.assertThrows(H2ConnectionException.class, () ->
491+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray())));
492+
Assertions.assertEquals(H2Error.STREAM_CLOSED, H2Error.getByCode(exception.getCode()));
493+
}
494+
495+
@Test
496+
void testContinuationAfterEndOfStream() throws Exception {
497+
final H2Config h2Config = H2Config.custom()
498+
.build();
499+
500+
final AbstractH2StreamMultiplexer streamMultiplexer = new H2StreamMultiplexerImpl(
501+
protocolIOSession,
502+
FRAME_FACTORY,
503+
StreamIdGenerator.EVEN,
504+
httpProcessor,
505+
CharCodingConfig.DEFAULT,
506+
h2Config,
507+
h2StreamListener,
508+
() -> streamHandler);
509+
510+
final ByteArrayBuffer headerBuf = new ByteArrayBuffer(200);
511+
final HPackEncoder encoder = new HPackEncoder(h2Config.getHeaderTableSize(),
512+
CharCodingSupport.createEncoder(CharCodingConfig.DEFAULT));
513+
514+
final List<Header> headers = Arrays.asList(
515+
new BasicHeader(":method", "GET"),
516+
new BasicHeader(":scheme", "http"),
517+
new BasicHeader(":path", "/"),
518+
new BasicHeader(":authority", "www.example.com"));
519+
encoder.encodeHeaders(headerBuf, headers, h2Config.isCompressionEnabled());
520+
521+
final WritableByteChannelMock writableChannel = new WritableByteChannelMock(1024);
522+
final FrameOutputBuffer outBuffer = new FrameOutputBuffer(16 * 1024);
523+
524+
final RawFrame headerFrame1 = FRAME_FACTORY.createHeaders(1, ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()), true, true);
525+
outBuffer.write(headerFrame1, writableChannel);
526+
527+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray()));
528+
529+
Mockito.verify(streamHandler).consumeHeader(headersCaptor.capture(), ArgumentMatchers.eq(true));
530+
Assertions.assertFalse(headersCaptor.getValue().isEmpty());
531+
532+
writableChannel.reset();
533+
final RawFrame continuationFrame = FRAME_FACTORY.createContinuation(1, ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()), true);
534+
outBuffer.write(continuationFrame, writableChannel);
535+
536+
final H2ConnectionException exception = Assertions.assertThrows(H2ConnectionException.class, () ->
537+
streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray())));
538+
Assertions.assertEquals(H2Error.PROTOCOL_ERROR, H2Error.getByCode(exception.getCode()));
539+
}
540+
273541
}
274542

0 commit comments

Comments
 (0)