From 15450af2e7c533c30cfdd52882a02006ba2fde00 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Wed, 9 Sep 2015 13:40:07 -0700 Subject: [PATCH] DefaultHttp2FrameWriter ping payload size check Motivation: The HTTP/2 spec states that the ping frame length must be 8 and is otherwise an error https://tools.ietf.org/html/rfc7540#section-6.7. The DefaultHttp2FrameReader enforces this, but the DefaultHttp2FrameWriter allows invalid frames to be written. We should not allow invalid ping frames to be written to the network. Modifications: - DefaultHttp2FrameWriter checks the frame size to be 8, or throws an exception Result: Fixes https://github.com/netty/netty/issues/3721 --- .../codec/http2/DefaultHttp2FrameReader.java | 12 +++++----- .../codec/http2/DefaultHttp2FrameWriter.java | 22 +++++++++++++------ 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java index 96bb2ca5c8..431bf0d48e 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java @@ -14,9 +14,15 @@ */ package io.netty.handler.codec.http2; +import io.netty.buffer.ByteBuf; +import io.netty.buffer.ByteBufAllocator; +import io.netty.channel.ChannelHandlerContext; +import io.netty.handler.codec.http2.Http2FrameReader.Configuration; + import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_MAX_FRAME_SIZE; import static io.netty.handler.codec.http2.Http2CodecUtil.FRAME_HEADER_LENGTH; import static io.netty.handler.codec.http2.Http2CodecUtil.INT_FIELD_LENGTH; +import static io.netty.handler.codec.http2.Http2CodecUtil.PING_FRAME_PAYLOAD_LENGTH; import static io.netty.handler.codec.http2.Http2CodecUtil.PRIORITY_ENTRY_LENGTH; import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_INITIAL_WINDOW_SIZE; import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_MAX_FRAME_SIZE; @@ -39,10 +45,6 @@ import static io.netty.handler.codec.http2.Http2FrameTypes.PUSH_PROMISE; import static io.netty.handler.codec.http2.Http2FrameTypes.RST_STREAM; import static io.netty.handler.codec.http2.Http2FrameTypes.SETTINGS; import static io.netty.handler.codec.http2.Http2FrameTypes.WINDOW_UPDATE; -import io.netty.buffer.ByteBuf; -import io.netty.buffer.ByteBufAllocator; -import io.netty.channel.ChannelHandlerContext; -import io.netty.handler.codec.http2.Http2FrameReader.Configuration; /** * A {@link Http2FrameReader} that supports all frame types defined by the HTTP/2 specification. @@ -344,7 +346,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize if (streamId != 0) { throw connectionError(PROTOCOL_ERROR, "A stream ID must be zero."); } - if (payloadLength != 8) { + if (payloadLength != PING_FRAME_PAYLOAD_LENGTH) { throw connectionError(FRAME_SIZE_ERROR, "Frame length %d incorrect size for ping.", payloadLength); } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java index 628703b571..29dbc33c44 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java @@ -15,6 +15,13 @@ package io.netty.handler.codec.http2; +import io.netty.buffer.ByteBuf; +import io.netty.channel.ChannelFuture; +import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.ChannelPromise; +import io.netty.handler.codec.http2.Http2CodecUtil.SimpleChannelPromiseAggregator; +import io.netty.handler.codec.http2.Http2FrameWriter.Configuration; + import static io.netty.buffer.Unpooled.directBuffer; import static io.netty.buffer.Unpooled.unmodifiableBuffer; import static io.netty.buffer.Unpooled.unreleasableBuffer; @@ -29,6 +36,7 @@ import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_UNSIGNED_BYTE; import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_UNSIGNED_INT; import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_WEIGHT; import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_WEIGHT; +import static io.netty.handler.codec.http2.Http2CodecUtil.PING_FRAME_PAYLOAD_LENGTH; import static io.netty.handler.codec.http2.Http2CodecUtil.PRIORITY_ENTRY_LENGTH; import static io.netty.handler.codec.http2.Http2CodecUtil.PRIORITY_FRAME_LENGTH; import static io.netty.handler.codec.http2.Http2CodecUtil.PUSH_PROMISE_FRAME_HEADER_LENGTH; @@ -53,13 +61,6 @@ import static io.netty.handler.codec.http2.Http2FrameTypes.SETTINGS; import static io.netty.handler.codec.http2.Http2FrameTypes.WINDOW_UPDATE; import static io.netty.util.internal.ObjectUtil.checkNotNull; -import io.netty.buffer.ByteBuf; -import io.netty.channel.ChannelFuture; -import io.netty.channel.ChannelHandlerContext; -import io.netty.channel.ChannelPromise; -import io.netty.handler.codec.http2.Http2CodecUtil.SimpleChannelPromiseAggregator; -import io.netty.handler.codec.http2.Http2FrameWriter.Configuration; - /** * A {@link Http2FrameWriter} that supports all frame types defined by the HTTP/2 specification. */ @@ -240,6 +241,7 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter, Http2FrameSize SimpleChannelPromiseAggregator promiseAggregator = new SimpleChannelPromiseAggregator(promise, ctx.channel(), ctx.executor()); try { + verifyPingPayload(data); Http2Flags flags = ack ? new Http2Flags().ack(true) : new Http2Flags(); ByteBuf buf = ctx.alloc().buffer(FRAME_HEADER_LENGTH); writeFrameHeaderInternal(buf, data.readableBytes(), PING, flags, 0); @@ -545,4 +547,10 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter, Http2FrameSize throw new IllegalArgumentException("WindowSizeIncrement must be >= 0"); } } + + private static void verifyPingPayload(ByteBuf data) { + if (data == null || data.readableBytes() != PING_FRAME_PAYLOAD_LENGTH) { + throw new IllegalArgumentException("Opaque data must be " + PING_FRAME_PAYLOAD_LENGTH + " bytes"); + } + } }