From 091ba177dc23fafaff45bbbd19f1a28fb32bfdfc Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Wed, 1 Jul 2026 18:09:56 +0200 Subject: [PATCH 1/2] fix: ctl:ruleRemoveTargetByTag not removing XML attribute targets fetch_target_exception() only derived a target's parameter from a ":"-suffix embedded in var->name, which is how most variables (ARGS, TX, REQUEST_HEADERS, ...) represent their parameter. XML variables keep the XPath expression in the separate var->param field instead, so their parameter was never compared, and ctl:ruleRemoveTargetBy* could never match an XPath target like XML://@*. Fall back to var->param when var->name has no embedded parameter, so target-exception matching also works for XML XPath targets. Fixes #3591 Co-Authored-By: Claude Sonnet 5 --- apache2/re.c | 9 +++++ tests/regression/rule/10-xml.t | 74 ++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/apache2/re.c b/apache2/re.c index d067c68419..35fb4a6fdb 100644 --- a/apache2/re.c +++ b/apache2/re.c @@ -85,6 +85,15 @@ static int fetch_target_exception(msre_rule *rule, modsec_rec *msr, msre_var *va myname = apr_strtok(myvar,":",&myvalue); } else { myname = myvar; + + /* Some variables (e.g., XML) keep their parameter (an XPath + * expression) in var->param instead of embedding it in + * var->name. Fall back to it so exception matching can still + * find a parameter to compare against. + */ + if (var->param != NULL) { + myvalue = apr_pstrdup(msr->mp, var->param); + } } match = 0; diff --git a/tests/regression/rule/10-xml.t b/tests/regression/rule/10-xml.t index ad1ed91941..45085063e3 100644 --- a/tests/regression/rule/10-xml.t +++ b/tests/regression/rule/10-xml.t @@ -428,3 +428,77 @@ ), ), }, + +### ctl:ruleRemoveTargetByTag with an XPath target (XML://@*) +# Baseline: without any target removal, the XML://@* target lets the rule +# match "attack" found in an XML attribute value. +{ + type => "rule", + comment => "ruleRemoveTargetByTag baseline: XML://\@* matches attribute value", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^(?:application(?:/soap\+|/)|text/)xml" "id:500040, \\ + phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=XML" + SecRule XML:/*|XML://\@* "\@rx attack" "id:500041, \\ + phase:2,deny,status:403,log,tag:'xml-attr-remove-test',msg:'XML attribute matched'" + ), + match_log => { + error => [ qr/Pattern match "attack" at XML\./, 1 ], + }, + match_response => { + status => qr/^403$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ + "Content-Type" => "application/xml", + ], + normalize_raw_request_data( + q( + + ), + ), + ), +}, +# Regression for issue #3591: ctl:ruleRemoveTargetByTag must strip the +# XML://@* target from the tagged rule, so it stops inspecting attribute +# values. Previously this had no effect because target-exception matching +# only ever compared the variable's embedded ":param" suffix in its name, +# which XML targets never populate (they keep the XPath expression in a +# separate field instead). +{ + type => "rule", + comment => "ruleRemoveTargetByTag removes an XML://\@* target from a tagged rule", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^(?:application(?:/soap\+|/)|text/)xml" "id:500042, \\ + phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=XML" + SecRule XML:/*|XML://\@* "\@rx attack" "id:500043, \\ + phase:2,deny,status:403,log,tag:'xml-attr-remove-test-2',msg:'XML attribute matched'" + SecAction "id:500044,phase:1,pass,nolog,ctl:ruleRemoveTargetByTag=xml-attr-remove-test-2;XML://\@*" + ), + match_log => { + debug => [ qr/fetch_target_exception: Target XML:\/\/\@\* will not be processed\./, 1 ], + -error => [ qr/Pattern match "attack" at XML\./, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ + "Content-Type" => "application/xml", + ], + normalize_raw_request_data( + q( + + ), + ), + ), +}, From 69beb777195b2f1e03e23b552c4f06b434854282 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Thu, 2 Jul 2026 13:16:45 +0200 Subject: [PATCH 2/2] Set pointer instead of creating a new copy Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- apache2/re.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apache2/re.c b/apache2/re.c index 35fb4a6fdb..ca62760c58 100644 --- a/apache2/re.c +++ b/apache2/re.c @@ -92,7 +92,7 @@ static int fetch_target_exception(msre_rule *rule, modsec_rec *msr, msre_var *va * find a parameter to compare against. */ if (var->param != NULL) { - myvalue = apr_pstrdup(msr->mp, var->param); + myvalue = var->param; } }