Skip to content

Fixed bug in the calculation of the header block fragment length #6923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bryancall merged 1 commit into from
Jun 22, 2020
Merged

Fixed bug in the calculation of the header block fragment length #6923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion proxy/http2/HPACK.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
49 changes: 28 additions & 21 deletions proxy/http2/Http2ConnectionState.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,13 +260,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;
Expand All @@ -285,7 +278,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");
}
Expand All @@ -301,14 +295,20 @@ 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) {
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR,
"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;
}
Expand All @@ -328,11 +328,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)) {
Copy link
Member

@zizhong zizhong Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing.
Why do we need max(A, 2 * A)?
Possible reasons could be,

  1. Wrong variables
  2. A < 0.

Copy link
Contributor Author

@bryancall bryancall Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am doing that in case we have an integer overflow.

Copy link
Member

@zizhong zizhong Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL!

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) {
Expand Down Expand Up @@ -890,21 +898,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();
Expand Down
6 changes: 2 additions & 4 deletions proxy/http2/Http2Stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,8 @@ class Http2Stream : public ProxyTransaction
//////////////////
// 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;

Expand Down