From cbdd1da62bd7c49c25737f449fc9d7850c75e3bf Mon Sep 17 00:00:00 2001 From: bneradt Date: Tue, 8 Sep 2026 14:55:01 -0500 Subject: [PATCH] header_rewrite: reject incomplete config lines A header_rewrite configuration line containing only modifiers, or a cond keyword without an operand, can leave the parser indexing an empty token list. Malformed configuration can therefore crash Traffic Server during plugin initialization. This patch rejects empty token lists after consuming modifiers or the cond keyword, allowing the caller to log and skip the invalid lines. Unit tests cover bare cond and short and long modifier tokens, while an AuTest verifies that ATS starts, logs the errors, and applies the remaining valid rules. Fixes: #13639 Co-authored-by: Claude Opus 5 Co-authored-by: GPT-6 Astra Medium --- plugins/header_rewrite/header_rewrite_test.cc | 27 +++++- plugins/header_rewrite/parser.cc | 18 +++- ...eader_rewrite_orphan_modifiers.replay.yaml | 91 +++++++++++++++++++ .../header_rewrite_orphan_modifiers.test.py | 29 ++++++ .../header_rewrite/orphan_modifiers.conf | 35 +++++++ 5 files changed, 195 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..a32d3b15c17 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,30 @@ 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(); + } + + for (const auto *line : {"cond", "cond [L]", "cond [AND,NOCASE,NOT,L,QSA,I,EXT,PRE]"}) { + ParserTest p(line); + + 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..2e16b1c091c 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; @@ -205,6 +211,10 @@ Parser::preprocess(std::vector tokens) } else if (tokens[0] == "cond") { _clause = CondClause::COND; tokens.erase(tokens.begin()); + if (tokens.empty()) { + TSError("[%s] cond must be followed by a condition", PLUGIN_NAME); + return false; + } } else if (tokens[0] == "else") { _clause = CondClause::ELSE; return true; 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..eca89a5839d --- /dev/null +++ b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.replay.yaml @@ -0,0 +1,91 @@ +# 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' + - expression: 'cond must be followed by a condition' + description: 'header_rewrite must reject cond without an operand' + +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..2201d9b47fd --- /dev/null +++ b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.test.py @@ -0,0 +1,29 @@ +''' +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. Also verify rejection of cond without an operand, +with or without modifiers. +''' + +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..14ca2b22bac --- /dev/null +++ b/tests/gold_tests/pluginTest/header_rewrite/orphan_modifiers.conf @@ -0,0 +1,35 @@ +# +# 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 cond keyword also needs an operand, with or without modifiers. +cond [L] +cond [AND,NOCASE,NOT,L,QSA,I,EXT,PRE] +cond + +# 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