Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4135752506
##########
src/proxy/hdrs/VersionConverter.cc:
##########
@@ -200,8 +201,21 @@ VersionConverter::_convert_req_from_2_to_1(HTTPHdr
&header) const
// :authority
if (MIMEField *field = header.field_find(PSEUDO_HEADER_AUTHORITY);
field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
- auto authority{field->value_get()};
- header.m_http->u.req.m_url_impl->set_host(header.m_heap, authority, true);
+ // Copy out first: allocating from header.m_heap may coalesce it and free
the field's storage.
+ auto value{field->value_get()};
+ ts::LocalBuffer<char> buf(value.length());
Review Comment:
An empty `:authority` passes the HTTP/2 presence check and
`value_is_valid()`. `LocalBuffer(0).data()` is null, but `url_parse_internet()`
dereferences the start pointer before checking its length. An HTTP/2 client can
therefore crash a worker with an empty authority. Return `ParseResult::ERROR`
before constructing or parsing the buffer.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,68 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate
ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here; re-apply strict_uri_parsing. Runs after the
REQUEST type
+ // check below, since path_get() asserts that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) &&
url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces token methods, Host and Content-Length framing
(RFC 9110 8.6),
+ // and the request line and header field size limits.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max =
static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max =
static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c)
{ return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req
makes the exact call.
+ if (method.size() +
static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ if (field.name_get().size() + field.value_get().size() > field_max) {
+ return false;
+ }
+ }
Review Comment:
The HTTP/2 decoder checks name casing, and the converter rejects controls
and whitespace, but neither checks ordinary field-name syntax. A name such as
`x(foo` therefore reaches HttpSM on this direct path even though the former
`parse_req()` path rejects it. Similarly, leading or trailing SP/HT in a value
is preserved here but trimmed by the parser. Extend this gate to fall back to
serialization and parsing for either case, so forwarded headers keep the former
validation and normalization.
##########
src/proxy/hdrs/VersionConverter.cc:
##########
@@ -229,14 +245,26 @@ VersionConverter::_convert_req_from_2_to_1(HTTPHdr
&header) const
// :path
if (MIMEField *field = header.field_find(PSEUDO_HEADER_PATH);
field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT))
{
- auto path{field->value_get()};
+ auto path{field->value_get()};
+ auto *url = header.m_http->u.req.m_url_impl;
Review Comment:
`path` still views the `:path` field's storage when `set_fragment()` or
`set_query()` allocates from the same `HdrHeap`. Those allocations can coalesce
the string heaps and invalidate `path` before the next `find()` or
`set_path()`, potentially corrupting the URL used for routing and caching. Copy
the full field into a local buffer before splitting it, as the authority
conversion above already does.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +959,33 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_pending_send_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0)
pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx
interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime,
field.value_get());
+ this->_send_header.field_attach(f);
Review Comment:
On the outbound direct path, a plugin can change `server_request` after the
client header was parsed. `TSMimeHdrFieldNameSet` accepts a name such as
`x(foo`, and this loop copies it without the old `parse_req()` validation. The
HTTP/2 converter checks only controls and whitespace, so the encoder can send
an invalid field name to the origin, which may reject the stream. Validate the
finalized outbound fields with the parser's field-name rules, or serialize and
reparse this header before sending it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
