This is an automated email from the ASF dual-hosted git repository.
bcall pushed a commit to branch 8.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/8.0.x by this push:
new 4cf5b88 Fixed bug in the calculation of the header block fragment
length
4cf5b88 is described below
commit 4cf5b887b3f7dc081988f1b7d71d37bee2f35c96
Author: Bryan Call <bc...@apache.org>
AuthorDate: Wed Apr 22 13:35:00 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, 35 insertions(+), 26 deletions(-)
diff --git a/proxy/http2/HPACK.cc b/proxy/http2/HPACK.cc
index 70a6805..93090d6 100644
--- a/proxy/http2/HPACK.cc
+++ b/proxy/http2/HPACK.cc
@@ -946,7 +946,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 3eaa558..5bc1408 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -227,13 +227,6 @@ rcv_headers_frame(Http2ConnectionState &cstate, const
Http2Frame &frame)
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE);
}
- // 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;
@@ -252,7 +245,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");
}
@@ -268,7 +262,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) {
@@ -276,6 +270,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;
}
@@ -295,11 +295,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) {
@@ -846,21 +854,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 f46404e..0c22288 100644
--- a/proxy/http2/Http2Stream.h
+++ b/proxy/http2/Http2Stream.h
@@ -161,10 +161,8 @@ public:
int64_t read_vio_read_avail();
uint8_t *header_blocks = nullptr;
- uint32_t header_blocks_length = 0; // total length of header blocks (not
include
- // Padding or other fields)
- uint32_t request_header_length = 0; // 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 = false;
bool send_end_stream = false;
