Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions include/proxy/hdrs/HTTP.h
Original file line number Diff line number Diff line change
Expand Up @@ -1088,7 +1088,7 @@ inline void
HTTPHdr::status_set(HTTPStatus status)
{
ink_assert(valid());
ink_assert(m_http->m_polarity == HTTPType::RESPONSE);
ink_release_assert(m_http->m_polarity == HTTPType::RESPONSE);

http_hdr_status_set(m_http, status);
}
Expand All @@ -1112,7 +1112,7 @@ inline void
HTTPHdr::reason_set(std::string_view value)
{
ink_assert(valid());
ink_assert(m_http->m_polarity == HTTPType::RESPONSE);
ink_release_assert(m_http->m_polarity == HTTPType::RESPONSE);

http_hdr_reason_set(m_heap, m_http, value, true);
}
Expand Down
18 changes: 6 additions & 12 deletions plugins/header_rewrite/operators.cc
Original file line number Diff line number Diff line change
Expand Up @@ -198,19 +198,13 @@ OperatorSetStatus::initialize_hooks()
bool
OperatorSetStatus::exec(const Resources &res) const
{
switch (get_hook()) {
case TS_HTTP_READ_RESPONSE_HDR_HOOK:
case TS_HTTP_SEND_RESPONSE_HDR_HOOK:
if (res.bufp && res.hdr_loc) {
TSHttpHdrStatusSet(res.bufp, res.hdr_loc, static_cast<TSHttpStatus>(_status.get_int_value()), res.state.txnp, PLUGIN_NAME);
if (_reason && _reason_len > 0) {
TSHttpHdrReasonSet(res.bufp, res.hdr_loc, _reason, _reason_len);
}
if (res.bufp && res.hdr_loc && TSHttpHdrTypeGet(res.bufp, res.hdr_loc) == TS_HTTP_TYPE_RESPONSE) {
TSHttpHdrStatusSet(res.bufp, res.hdr_loc, static_cast<TSHttpStatus>(_status.get_int_value()), res.state.txnp, PLUGIN_NAME);
if (_reason && _reason_len > 0) {
TSHttpHdrReasonSet(res.bufp, res.hdr_loc, _reason, _reason_len);
}
break;
default:
} else {
TSHttpTxnStatusSet(res.state.txnp, static_cast<TSHttpStatus>(_status.get_int_value()), PLUGIN_NAME);
break;
}

Dbg(pi_dbg_ctl, "OperatorSetStatus::exec() invoked with status=%d", _status.get_int_value());
Expand Down Expand Up @@ -601,7 +595,7 @@ OperatorSetRedirect::exec(const Resources &res) const
const_cast<Resources &>(res).changed_url = true;
res._rri->redirect = 1;
} else {
Dbg(pi_dbg_ctl, "OperatorSetRedirect::exec() hook=%d", int(get_hook()));
Dbg(pi_dbg_ctl, "OperatorSetRedirect::exec() redirect to %s", value.c_str());
// Set the new status code and reason.
TSHttpStatus status = static_cast<TSHttpStatus>(_status.get_int_value());
TSHttpHdrStatusSet(res.bufp, res.hdr_loc, status, res.state.txnp, PLUGIN_NAME);
Expand Down
4 changes: 2 additions & 2 deletions plugins/header_rewrite/ruleset.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ RuleSet::make_condition(Parser &p, const char *filename, int lineno)

Dbg(pi_dbg_ctl, " Creating condition: %%{%s} with arg: %s", p.get_op().c_str(), p.get_arg().c_str());
c->initialize(p);
if (!c->set_hook(_hook)) {
if (!c->is_hook_valid(_hook)) {
delete c;
TSError("[%s] in %s:%d: can't use this condition in hook=%s: %%{%s} with arg: %s", PLUGIN_NAME, filename, lineno,
TSHttpHookNameLookup(_hook), p.get_op().c_str(), p.get_arg().c_str());
Expand All @@ -93,7 +93,7 @@ RuleSet::add_operator(Parser &p, const char *filename, int lineno)
if (nullptr != op) {
Dbg(pi_dbg_ctl, " Adding operator: %s(%s)=\"%s\"", p.get_op().c_str(), p.get_arg().c_str(), p.get_value().c_str());
op->initialize(p);
if (!op->set_hook(_hook)) {
if (!op->is_hook_valid(_hook)) {
delete op;
Dbg(pi_dbg_ctl, "in %s:%d: can't use this operator in hook=%s: %s(%s)", filename, lineno, TSHttpHookNameLookup(_hook),
p.get_op().c_str(), p.get_arg().c_str());
Expand Down
10 changes: 2 additions & 8 deletions plugins/header_rewrite/statement.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,9 @@ Statement::get_resource_ids() const
}

bool
Statement::set_hook(TSHttpHookID hook)
Statement::is_hook_valid(TSHttpHookID hook) const
{
bool ret = std::find(_allowed_hooks.begin(), _allowed_hooks.end(), hook) != _allowed_hooks.end();

if (ret) {
_hook = hook;
}

return ret;
return std::find(_allowed_hooks.begin(), _allowed_hooks.end(), hook) != _allowed_hooks.end();
}

// This should be overridden for any Statement which only supports some hooks
Expand Down
10 changes: 2 additions & 8 deletions plugins/header_rewrite/statement.h
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,8 @@ class Statement
Statement(const Statement &) = delete;
void operator=(const Statement &) = delete;

// Which hook are we adding this statement to?
bool set_hook(TSHttpHookID hook);
TSHttpHookID
get_hook() const
{
return _hook;
}
// Validate that this statement is allowed on the given hook. Used during parsing only.
bool is_hook_valid(TSHttpHookID hook) const;

// Which hooks are this "statement" applicable for? Used during parsing only.
void
Expand Down Expand Up @@ -267,7 +262,6 @@ class Statement

private:
ResourceIDs _rsrc = RSRC_NONE;
TSHttpHookID _hook = TS_HTTP_READ_RESPONSE_HDR_HOOK;
std::vector<TSHttpHookID> _allowed_hooks;
bool _initialized = false;
};
Expand Down
4 changes: 2 additions & 2 deletions src/proxy/hdrs/HTTP.cc
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,7 @@ http_hdr_url_set(HdrHeap *heap, HTTPHdrImpl *hh, URLImpl *url)
void
http_hdr_status_set(HTTPHdrImpl *hh, HTTPStatus status)
{
ink_assert(hh->m_polarity == HTTPType::RESPONSE);
ink_release_assert(hh->m_polarity == HTTPType::RESPONSE);
hh->u.resp.m_status = static_cast<int16_t>(status);
}

Expand All @@ -713,7 +713,7 @@ http_hdr_reason_get(HTTPHdrImpl *hh)
void
http_hdr_reason_set(HdrHeap *heap, HTTPHdrImpl *hh, std::string_view value, bool must_copy)
{
ink_assert(hh->m_polarity == HTTPType::RESPONSE);
ink_release_assert(hh->m_polarity == HTTPType::RESPONSE);
mime_str_u16_set(heap, value, &(hh->u.resp.m_ptr_reason), &(hh->u.resp.m_len_reason), must_copy);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,13 @@ autest:
args:
- "rules/rule_session_vars.conf"

- from: "http://www.example.com/from_18/"
to: "http://backend.ex:{SERVER_HTTP_PORT}/to_18/"
plugins:
- name: "header_rewrite.so"
args:
- "rules/set_status_in_if.conf"

log_validation:
traffic_out:
excludes:
Expand Down Expand Up @@ -1880,3 +1887,27 @@ sessions:
headers:
fields:
- [ X-Session-Seen, { value: "yes", as: equal } ]

# Test 67: set-status inside if/endif at REMAP_PSEUDO_HOOK time.
# Before the fix, the set-status operator inside an if/endif block retained
# its default _hook (TS_HTTP_READ_RESPONSE_HDR_HOOK), so exec() called
# TSHttpHdrStatusSet on a request buffer, corrupting the union and crashing.
- transactions:
- client-request:
method: "GET"
version: "1.1"
url: /from_18/test
headers:
fields:
- [ Host, www.example.com ]
- [ uuid, 67 ]

server-response:
status: 200
reason: OK
headers:
fields:
- [ Connection, close ]

proxy-response:
status: 403
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# Minimal reproduction for set-status inside if/endif at remap time.
# Before the fix, the operator's _hook defaulted to
# TS_HTTP_READ_RESPONSE_HDR_HOOK, so exec() called TSHttpHdrStatusSet
# on a request buffer, corrupting the header union and crashing.
#
cond %{REMAP_PSEUDO_HOOK}
if
cond %{CLIENT-URL:PATH} /from_18/
set-status 403
endif