This is an automated email from the ASF dual-hosted git repository.
bcall pushed a commit to branch 7.1.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/7.1.x by this push:
new d80440c Fixed bug in the calculation of the header block fragment
length
d80440c is described below
commit d80440c92afd53b68196a83a82e66975748f9402
Author: Bryan Call <bc...@apache.org>
AuthorDate: Wed Apr 29 13:39:17 2020 -0700
Fixed bug in the calculation of the header block fragment length
Co-authored-by: Masaori Koshiba <masa...@apache.org>
---
proxy/http2/HPACK.cc | 6 ++++-
proxy/http2/Http2ConnectionState.cc | 49 +++++++++++++++++++++----------------
proxy/http2/Http2Stream.h | 6 +----
3 files changed, 34 insertions(+), 27 deletions(-)
diff --git a/proxy/http2/HPACK.cc b/proxy/http2/HPACK.cc
index e06b7b7..cd6aee6 100644
--- a/proxy/http2/HPACK.cc
+++ b/proxy/http2/HPACK.cc
@@ -941,7 +941,11 @@ hpack_decode_header_block(HpackIndexingTable
&indexing_table, HTTPHdr *hdr, cons
field->name_get(&name_len);
field->value_get(&value_len);
- total_header_size += name_len + value_len;
+
+ // [RFC 7540] 6.5.2. SETTINGS_MAX_HEADER_LIST_SIZE:
+ // The value is based on the uncompressed size of header fields, including
the length of the name and value in octets plus an
+ // overhead of 32 octets for each header field.
+ total_header_size += name_len + value_len + ADDITIONAL_OCTETS;
if (total_header_size > max_header_size) {
return HPACK_ERROR_SIZE_EXCEEDED_ERROR;
diff --git a/proxy/http2/Http2ConnectionState.cc
b/proxy/http2/Http2ConnectionState.cc
index e33a08e..75ef601 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -218,13 +218,6 @@ rcv_headers_frame(Http2ConnectionState &cstate, const
Http2Frame &frame)
}
}
- // keep track of how many bytes we get in the frame
- stream->request_header_length += payload_length;
- if (stream->request_header_length > Http2::max_header_list_size) {
- return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM,
Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR,
- "recv headers payload for headers greater than header
length");
- }
-
Http2HeadersParameter params;
uint32_t header_block_fragment_offset = 0;
uint32_t header_block_fragment_length = payload_length;
@@ -243,7 +236,8 @@ rcv_headers_frame(Http2ConnectionState &cstate, const
Http2Frame &frame)
"recv headers failed to parse");
}
- if (params.pad_length > payload_length) {
+ // Payload length can't be smaller than the pad length
+ if ((params.pad_length + HTTP2_HEADERS_PADLEN_LEN) >
header_block_fragment_length) {
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION,
Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR,
"recv headers pad > payload length");
}
@@ -259,7 +253,7 @@ rcv_headers_frame(Http2ConnectionState &cstate, const
Http2Frame &frame)
frame.reader()->memcpy(buf, HTTP2_PRIORITY_LEN,
header_block_fragment_offset);
if (!http2_parse_priority_parameter(make_iovec(buf, HTTP2_PRIORITY_LEN),
params.priority)) {
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION,
Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR,
- "recv headers prioirity parameters failed parse");
+ "recv headers priority parameters failed parse");
}
// Protocol error if the stream depends on itself
if (stream_id == params.priority.stream_dependency) {
@@ -267,6 +261,12 @@ rcv_headers_frame(Http2ConnectionState &cstate, const
Http2Frame &frame)
"recv headers self dependency");
}
+ // Payload length can't be smaller than the priority length
+ if (HTTP2_PRIORITY_LEN > header_block_fragment_length) {
+ return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION,
Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR,
+ "recv priority length > payload length");
+ }
+
header_block_fragment_offset += HTTP2_PRIORITY_LEN;
header_block_fragment_length -= HTTP2_PRIORITY_LEN;
}
@@ -286,11 +286,19 @@ rcv_headers_frame(Http2ConnectionState &cstate, const
Http2Frame &frame)
}
}
+ stream->header_blocks_length = header_block_fragment_length;
+
+ // ATS advertises SETTINGS_MAX_HEADER_LIST_SIZE as a limit of total header
blocks length. (Details in [RFC 7560] 10.5.1.)
+ // Make it double to relax the limit in cases of 1) HPACK is used naively,
or 2) Huffman Encoding generates large header blocks.
+ // The total "decoded" header length is strictly checked by
hpack_decode_header_block().
+ if (stream->header_blocks_length > std::max(Http2::max_header_list_size,
Http2::max_header_list_size * 2)) {
+ return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION,
Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM,
+ "header blocks too large");
+ }
+
stream->header_blocks = static_cast<uint8_t
*>(ats_malloc(header_block_fragment_length));
frame.reader()->memcpy(stream->header_blocks, header_block_fragment_length,
header_block_fragment_offset);
- stream->header_blocks_length = header_block_fragment_length;
-
if (frame.header().flags & HTTP2_FLAGS_HEADERS_END_HEADERS) {
// NOTE: If there are END_HEADERS flag, decode stored Header Blocks.
if (!stream->change_state(HTTP2_FRAME_TYPE_HEADERS, frame.header().flags)
&& stream->has_trailing_header() == false) {
@@ -831,21 +839,20 @@ rcv_continuation_frame(Http2ConnectionState &cstate,
const Http2Frame &frame)
}
}
- // keep track of how many bytes we get in the frame
- stream->request_header_length += payload_length;
- if (stream->request_header_length > Http2::max_header_list_size) {
- return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION,
Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR,
- "continuation payload for headers exceeded");
- }
-
uint32_t header_blocks_offset = stream->header_blocks_length;
stream->header_blocks_length += payload_length;
- if (stream->header_blocks_length > 0) {
- stream->header_blocks = static_cast<uint8_t
*>(ats_realloc(stream->header_blocks, stream->header_blocks_length));
- frame.reader()->memcpy(stream->header_blocks + header_blocks_offset,
payload_length);
+ // ATS advertises SETTINGS_MAX_HEADER_LIST_SIZE as a limit of total header
blocks length. (Details in [RFC 7560] 10.5.1.)
+ // Make it double to relax the limit in cases of 1) HPACK is used naively,
or 2) Huffman Encoding generates large header blocks.
+ // The total "decoded" header length is strictly checked by
hpack_decode_header_block().
+ if (stream->header_blocks_length > std::max(Http2::max_header_list_size,
Http2::max_header_list_size * 2)) {
+ return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION,
Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM,
+ "header blocks too large");
}
+ stream->header_blocks = static_cast<uint8_t
*>(ats_realloc(stream->header_blocks, stream->header_blocks_length));
+ frame.reader()->memcpy(stream->header_blocks + header_blocks_offset,
payload_length);
+
if (frame.header().flags & HTTP2_FLAGS_HEADERS_END_HEADERS) {
// NOTE: If there are END_HEADERS flag, decode stored Header Blocks.
cstate.clear_continued_stream_id();
diff --git a/proxy/http2/Http2Stream.h b/proxy/http2/Http2Stream.h
index 591f5b5..115a34a 100644
--- a/proxy/http2/Http2Stream.h
+++ b/proxy/http2/Http2Stream.h
@@ -41,7 +41,6 @@ public:
Http2Stream(Http2StreamId sid = 0, ssize_t initial_rwnd =
Http2::initial_window_size)
: header_blocks(NULL),
header_blocks_length(0),
- request_header_length(0),
recv_end_stream(false),
send_end_stream(false),
sent_request_header(false),
@@ -190,10 +189,7 @@ public:
int64_t read_vio_read_avail();
uint8_t *header_blocks;
- uint32_t header_blocks_length; // total length of header blocks (not include
- // Padding or other fields)
- uint32_t request_header_length; // total length of payload (include Padding
- // and other fields)
+ uint32_t header_blocks_length = 0; // total length of header blocks (not
include Padding or other fields)
bool recv_end_stream;
bool send_end_stream;
