From 8795a45b4a8c2c762923504c6a67b34635c0e6a0 Mon Sep 17 00:00:00 2001 From: bneradt Date: Tue, 8 Sep 2026 14:55:01 -0500 Subject: [PATCH] header_rewrite: reject a modifier-only config line A header_rewrite configuration line consisting of nothing but a modifier section, such as a stray "[L]", crashes Traffic Server at config load time. The parser consumes the trailing modifiers with a pop_back() and then indexes tokens[0] unconditionally, so an empty token list is read out of bounds. On a hardened build that aborts during plugin init, which takes the whole server down; elsewhere it is a read of a destroyed std::string. Any operator who can write a header_rewrite config can trigger it. This patch guards the token list after the modifier section is consumed. A line with no condition or operator left is not a valid rule, so the parser now reports the offending modifiers with TSError and returns false, which the existing caller already handles by logging the line number and skipping the line. The rest of the config loads normally. This change also adds a unit test covering both a short, small-string optimized modifier token and a longer heap allocated one, plus an end-to-end autest that loads such a config and verifies that Traffic Server starts, logs the rejection, and still applies the surrounding rules. Fixes: #13639 Co-authored-by: Claude Opus 5 --- plugins/header_rewrite/header_rewrite_test.cc | 19 +++- plugins/header_rewrite/parser.cc | 14 ++- ...eader_rewrite_orphan_modifiers.replay.yaml | 89 +++++++++++++++++++ .../header_rewrite_orphan_modifiers.test.py | 28 ++++++ .../header_rewrite/orphan_modifiers.conf | 30 +++++++ 5 files changed, 175 insertions(+), 5 deletions(-) create mode 100644 tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.replay.yaml create mode 100644 tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.test.py create mode 100644 tests/gold_tests/pluginTest/header_rewrite/orphan_modifiers.conf diff --git a/plugins/header_rewrite/header_rewrite_test.cc b/plugins/header_rewrite/header_rewrite_test.cc index 7ec83230a36..f742baeb060 100644 --- a/plugins/header_rewrite/header_rewrite_test.cc +++ b/plugins/header_rewrite/header_rewrite_test.cc @@ -58,7 +58,7 @@ class ParserTest : public Parser public: ParserTest(const std::string &line) : res(true) { - Parser::parse_line(line); + parse_succeeded = Parser::parse_line(line); std::cout << "Finished parser test: " << line << std::endl; } @@ -79,6 +79,7 @@ class ParserTest : public Parser } bool res; + bool parse_succeeded = false; }; class SimpleTokenizerTest : public HRWSimpleTokenizer @@ -436,6 +437,22 @@ test_parsing() END_TEST(); } + { /* modifiers with no condition or operator to attach them to */ + ParserTest p("[L]"); + + CHECK_EQ(p.parse_succeeded, false); + + END_TEST(); + } + + { /* same, but long enough that the token is heap allocated rather than SSO */ + ParserTest p("[AND,NOCASE,NOT,L,QSA,I,EXT,PRE]"); + + CHECK_EQ(p.parse_succeeded, false); + + END_TEST(); + } + return errors; } diff --git a/plugins/header_rewrite/parser.cc b/plugins/header_rewrite/parser.cc index c1f467d39a8..6bf81b793f3 100644 --- a/plugins/header_rewrite/parser.cc +++ b/plugins/header_rewrite/parser.cc @@ -171,11 +171,12 @@ Parser::preprocess(std::vector tokens) { // The last token might be the "flags" section, lets consume it if it is if (tokens.size() > 0) { - std::string m = tokens[tokens.size() - 1]; + const std::string flags = tokens[tokens.size() - 1]; + + if (!flags.empty() && (flags[0] == '[')) { + if (flags[flags.size() - 1] == ']') { + std::string m = flags.substr(1, flags.size() - 2); - if (!m.empty() && (m[0] == '[')) { - if (m[m.size() - 1] == ']') { - m = m.substr(1, m.size() - 2); if (m.find_first_of(',') != std::string::npos) { std::istringstream iss(m); std::string t; @@ -192,6 +193,11 @@ Parser::preprocess(std::vector tokens) _mods.push_back(m); } tokens.pop_back(); // consume it, so we don't concatenate it into the value + + if (tokens.empty()) { + TSError("[%s] modifiers must follow a condition or operator: %s", PLUGIN_NAME, flags.c_str()); + return false; + } } else { TSError("[%s] mods have to be enclosed in []", PLUGIN_NAME); return false; diff --git a/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.replay.yaml b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.replay.yaml new file mode 100644 index 00000000000..bd0b95968b0 --- /dev/null +++ b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.replay.yaml @@ -0,0 +1,89 @@ +# 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. + +meta: + version: "1.0" + +autest: + description: 'Verify header_rewrite rejects a modifier-only config line' + + server: + name: 'server' + + client: + name: 'client' + + ats: + name: 'ts' + + process_config: + # The rejected config lines are reported with TSError, which lands in + # diags.log as an ERROR. That is the expected behavior here. + disable_log_checks: true + + copy_to_config_dir: + - 'orphan_modifiers.conf' + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'header_rewrite' + + plugin_config: + - 'header_rewrite.so orphan_modifiers.conf' + + remap_config: + - from: "http://www.example.com/" + to: "http://127.0.0.1:{SERVER_HTTP_PORT}/" + + log_validation: + diags_log: + contains: + - expression: 'modifiers must follow a condition or operator: \[L\]' + description: 'header_rewrite must reject the short modifier-only line' + - expression: 'modifiers must follow a condition or operator: \[AND,NOCASE,NOT,L,QSA,I,EXT,PRE\]' + description: 'header_rewrite must reject the long modifier-only line' + +sessions: +- transactions: + + ############################################################################# + # ATS came up despite the bad lines, and the valid rule that follows them + # still fires. + ############################################################################# + - client-request: + method: "GET" + version: "1.1" + url: /orphan_modifiers/ + headers: + fields: + - [ Host, www.example.com ] + - [ uuid, orphan-modifiers ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "3" ] + content: + encoding: plain + data: xxx + + proxy-response: + status: 200 + headers: + fields: + - [ X-Orphan-Modifiers-Survived, { value: "yes", as: equal } ] diff --git a/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.test.py b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.test.py new file mode 100644 index 00000000000..7f2c7073c3d --- /dev/null +++ b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.test.py @@ -0,0 +1,28 @@ +''' +Test that header_rewrite rejects a config line that is nothing but modifiers. +''' +# 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. + +Test.Summary = ''' +A header_rewrite line holding only a modifier section, such as "[L]", leaves the +parser with no tokens once the section is consumed. Verify that it is rejected +rather than read out of bounds. +''' + +Test.SkipUnless(Condition.PluginExists('header_rewrite.so')) + +Test.ATSReplayTest(replay_file="header_rewrite_orphan_modifiers.replay.yaml",) diff --git a/tests/gold_tests/pluginTest/header_rewrite/orphan_modifiers.conf b/tests/gold_tests/pluginTest/header_rewrite/orphan_modifiers.conf new file mode 100644 index 00000000000..90a72ec3a02 --- /dev/null +++ b/tests/gold_tests/pluginTest/header_rewrite/orphan_modifiers.conf @@ -0,0 +1,30 @@ +# +# 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. + +# A modifier section on a line of its own has no condition or operator to +# attach to, so consuming it leaves the parser with an empty token list. Such a +# line must be rejected and skipped. +[L] + +# A longer one, so the token is heap allocated rather than living in the +# std::string small-string buffer. +[AND,NOCASE,NOT,L,QSA,I,EXT,PRE] + +# The surrounding rules must still load and fire, proving the bad line was +# skipped rather than aborting the whole config. +cond %{SEND_RESPONSE_HDR_HOOK} + set-header X-Orphan-Modifiers-Survived yes