You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2020/06/23 19:52:05 UTC
[trafficserver] branch 9.0.x updated: Fixed bug in the calculation
of the header block fragment length (#6923)
This is an automated email from the ASF dual-hosted git repository.
zwoop pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new c7e9fee Fixed bug in the calculation of the header block fragment length (#6923)
c7e9fee is described below
commit c7e9fee34baba2449fd830388a2da50ab50511ea
Author: Bryan Call <bc...@apache.org>
AuthorDate: Mon Jun 22 16:25:51 2020 -0700
Fixed bug in the calculation of the header block fragment length (#6923)
Co-authored-by: Masaori Koshiba <ma...@apache.org>
(cherry picked from commit 481778679af293ff5558ed965481af683f703531)
---
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 97a2626..4ed3ba7 100644
--- a/proxy/http2/HPACK.cc
+++ b/proxy/http2/HPACK.cc
@@ -833,7 +833,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 ecb8cf4..53eac3b 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -257,13 +257,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;
@@ -282,7 +275,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");
}
@@ -298,7 +292,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) {
@@ -306,6 +300,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;
}
@@ -325,11 +325,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) {
@@ -887,21 +895,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 ce50f34..e923a5d 100644
--- a/proxy/http2/Http2Stream.h
+++ b/proxy/http2/Http2Stream.h
@@ -131,10 +131,8 @@ public:
//////////////////
// Variables
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;