From 9a8d62b4d20450b0d81f6d5cc1bbe32f4b767d1f Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Tue, 25 Nov 2025 16:37:59 +0100 Subject: [PATCH 01/10] feat: add check for TX.N usage without capture Signed-off-by: Felipe Zipitria --- README.md | 46 ++- src/crs_linter/rules/check_capture.py | 186 ++++++++---- src/crs_linter/rules/variables_usage.py | 10 +- tests/test_check_capture.py | 385 ++++++++++++++++++++++++ 4 files changed, 559 insertions(+), 68 deletions(-) create mode 100644 tests/test_check_capture.py diff --git a/README.md b/README.md index ab0575c..5809a1f 100644 --- a/README.md +++ b/README.md @@ -167,12 +167,12 @@ util/APPROVED_TAGS file. **Source:** `src/crs_linter/rules/check_capture.py` -Check that every chained rule has a `capture` action if it uses TX.N variable. +Check that rules using TX.N variables have a corresponding `capture` action. -This rule ensures that chained rules using captured transaction variables -(TX:0, TX:1, TX:2, etc.) have a corresponding `capture` action in a -previous rule in the chain. +This rule ensures that captured transaction variables (TX:0, TX:1, TX:2, etc.) +are only used when a `capture` action has been defined in the rule chain. +<<<<<<< HEAD Example of a passing rule: ```apache @@ -200,6 +200,44 @@ SecRule ARGS "@rx attack" \ chain" SecRule TX:0 "@eq attack" # Fails: uses TX:0 without prior capture ``` +======= +TX.N variables can be referenced in multiple ways: +1. As a rule target: `SecRule TX:1 "@eq attack"` +2. In action arguments: `msg:'Matched: %{TX.1}'`, `logdata:'Data: %{TX.0}'` +3. In operator arguments: `@rx %{TX.1}` +4. In setvar assignments: `setvar:tx.foo=%{TX.1}` + +Example of a passing rule (with capture): + SecRule ARGS "@rx (attack)" \ + "id:2,\ + phase:2,\ + deny,\ + capture,\ + msg:'Attack detected: %{TX.1}',\ + logdata:'Pattern: %{TX.0}',\ + chain" + SecRule TX:1 "@eq attack" + +Example of a failing rule (missing capture for target): + SecRule ARGS "@rx attack" \ + "id:3,\ + phase:2,\ + deny,\ + t:none,\ + nolog,\ + chain" + SecRule TX:0 "@eq attack" # Fails: uses TX:0 without capture + +Example of a failing rule (missing capture for action argument): + SecRule ARGS "@rx attack" \ + "id:4,\ + phase:2,\ + deny,\ + msg:'Matched: %{TX.1}'" # Fails: references TX.1 without capture + +This check addresses the issue found in CRS PR #4265 where %{TX.N} was +used in action arguments without verifying that capture was defined. +>>>>>>> f88f28b (feat: add check for TX.N usage without capture) ## CrsTag diff --git a/src/crs_linter/rules/check_capture.py b/src/crs_linter/rules/check_capture.py index 38e34eb..b40ca41 100644 --- a/src/crs_linter/rules/check_capture.py +++ b/src/crs_linter/rules/check_capture.py @@ -4,24 +4,29 @@ class CheckCapture(Rule): - """Check that every chained rule has a `capture` action if it uses TX.N variable. + """Check that rules using TX.N variables have a corresponding `capture` action. - This rule ensures that chained rules using captured transaction variables - (TX:0, TX:1, TX:2, etc.) have a corresponding `capture` action in a - previous rule in the chain. + This rule ensures that captured transaction variables (TX:0, TX:1, TX:2, etc.) + are only used when a `capture` action has been defined in the rule chain. - Example of a passing rule: - SecRule ARGS "@rx attack" \\ + TX.N variables can be referenced in multiple ways: + 1. As a rule target: `SecRule TX:1 "@eq attack"` + 2. In action arguments: `msg:'Matched: %{TX.1}'`, `logdata:'Data: %{TX.0}'` + 3. In operator arguments: `@rx %{TX.1}` + 4. In setvar assignments: `setvar:tx.foo=%{TX.1}` + + Example of a passing rule (with capture): + SecRule ARGS "@rx (attack)" \\ "id:2,\\ phase:2,\\ deny,\\ capture,\\ - t:none,\\ - nolog,\\ + msg:'Attack detected: %{TX.1}',\\ + logdata:'Pattern: %{TX.0}',\\ chain" SecRule TX:1 "@eq attack" - Example of a failing rule (missing capture): + Example of a failing rule (missing capture for target): SecRule ARGS "@rx attack" \\ "id:3,\\ phase:2,\\ @@ -29,7 +34,17 @@ class CheckCapture(Rule): t:none,\\ nolog,\\ chain" - SecRule TX:0 "@eq attack" # Fails: uses TX:0 without prior capture + SecRule TX:0 "@eq attack" # Fails: uses TX:0 without capture + + Example of a failing rule (missing capture for action argument): + SecRule ARGS "@rx attack" \\ + "id:4,\\ + phase:2,\\ + deny,\\ + msg:'Matched: %{TX.1}'" # Fails: references TX.1 without capture + + This check addresses the issue found in CRS PR #4265 where %{TX.N} was + used in action arguments without verifying that capture was defined. """ def __init__(self): @@ -40,65 +55,118 @@ def __init__(self): self.error_title = "capture is missing" self.args = ("data",) + # Regex patterns for detecting TX.N references + self.target_pattern = re.compile(r"^\d$") # For target variables: TX:1 + self.expansion_pattern = re.compile( + r"%\{TX[.:](\d)\}", # Matches %{TX.0} or %{TX:1} + re.IGNORECASE + ) + + # Actions that commonly use variable expansion + self.actions_to_check = { + 'msg', 'logdata', 'setvar', 'tag' + } + def check(self, data): """ - check that every chained rule has a `capture` action if it uses TX.N variable + Check that TX.N variables are only used when capture action is defined. + + This checks for TX.N references in: + - Rule targets (existing functionality) + - Action arguments (msg, logdata, setvar, tag) + - Operator arguments """ chained = False ruleid = 0 chainlevel = 0 capture_level = None - re_number = re.compile(r"^\d$") has_capture = False use_captured_var = False + use_captured_var_in_expansion = False # Track if TX.N is used in expansion captured_var_chain_level = 0 + for d in data: # only the SecRule object is relevant - if d["type"].lower() == "secrule": - for v in d["variables"]: - if v["variable"].lower() == "tx" and re_number.match( - v["variable_part"] - ): - # only the first occurrence required - if not use_captured_var: - use_captured_var = True - captured_var_chain_level = chainlevel - if "actions" in d: - if not chained: - ruleid = 0 - chainlevel = 0 - else: - chained = False - for a in d["actions"]: - if a["act_name"] == "id": - ruleid = int(a["act_arg"]) - if a["act_name"] == "chain": - chained = True - chainlevel += 1 - if a["act_name"] == "capture": - capture_level = chainlevel - has_capture = True - if ruleid > 0 and not chained: # end of chained rule - if use_captured_var: - # we allow if target with TX:N is in the first rule - # of a chained rule without 'capture' - if captured_var_chain_level > 0: - if ( - not has_capture - or captured_var_chain_level < capture_level - ): - yield LintProblem( - line=a["lineno"], - end_line=a["lineno"], - desc=f"rule uses TX.N without capture; rule id: {ruleid}", - rule="capture", - ) - # clear variables - chained = False - chainlevel = 0 - has_capture = False - capture_level = 0 - captured_var_chain_level = 0 - use_captured_var = False - ruleid = 0 + if d["type"].lower() != "secrule": + continue + + # Check 1: TX.N as target variable (existing check) + for v in d["variables"]: + if (v["variable"].lower() == "tx" and + self.target_pattern.match(v["variable_part"])): + # only the first occurrence required + if not use_captured_var: + use_captured_var = True + captured_var_chain_level = chainlevel + + # Check 2: TX.N in operator arguments + if "operator_argument" in d and d["operator_argument"]: + op_arg = d["operator_argument"] + if self.expansion_pattern.search(op_arg): + if not use_captured_var: + use_captured_var = True + use_captured_var_in_expansion = True + captured_var_chain_level = chainlevel + + # Check 3: TX.N in action arguments + if "actions" in d: + if not chained: + ruleid = 0 + chainlevel = 0 + else: + chained = False + + for a in d["actions"]: + if a["act_name"] == "id": + ruleid = int(a["act_arg"]) + if a["act_name"] == "chain": + chained = True + chainlevel += 1 + if a["act_name"] == "capture": + capture_level = chainlevel + has_capture = True + + # Check if action argument contains TX.N reference + if (a["act_name"] in self.actions_to_check and + a.get("act_arg")): + if self.expansion_pattern.search(a["act_arg"]): + if not use_captured_var: + use_captured_var = True + use_captured_var_in_expansion = True + captured_var_chain_level = chainlevel + + # End of rule/chain - validate + if ruleid > 0 and not chained: + if use_captured_var: + # Rules for requiring capture: + # 1. TX.N as target in chained rule (not first) - require capture (original check) + # 2. TX.N in expansion (%{TX.N}) anywhere - require capture (new check for issue #69) + should_error = False + + if captured_var_chain_level > 0: + # TX.N used in a chained rule (not the first) + if not has_capture or captured_var_chain_level < capture_level: + should_error = True + elif use_captured_var_in_expansion and not has_capture: + # TX.N used in expansion (%{TX.N}) without capture + # This is the new check for issue #69 + should_error = True + + if should_error: + yield LintProblem( + line=a["lineno"], + end_line=a["lineno"], + desc=f"rule uses TX.N without capture; rule id: {ruleid}", + rule="capture", + ) + + # clear variables + chained = False + chainlevel = 0 + has_capture = False + capture_level = 0 + captured_var_chain_level = 0 + use_captured_var = False + use_captured_var_in_expansion = False + ruleid = 0 diff --git a/src/crs_linter/rules/variables_usage.py b/src/crs_linter/rules/variables_usage.py index 5d9a3d1..db88160 100644 --- a/src/crs_linter/rules/variables_usage.py +++ b/src/crs_linter/rules/variables_usage.py @@ -87,16 +87,16 @@ def check(self, data, globtxvars): # act_arg <- tx.inbound_anomaly_score_threshold # act_atg_val <- 5 if "act_arg" in a and a["act_arg"] is not None: - val_act = re.findall(r"%\{(tx.[^%]*)}", a["act_arg"], re.I) + val_act = re.findall(r"%\{(tx.[^%]*)}", a["act_arg"], flags=re.I) # Check act_arg_val for TX variable references in action argument values # (e.g., the right-hand side of setvar assignments like "setvar:tx.foo=%{tx.bar}") if "act_arg_val" in a and a["act_arg_val"] is not None: val_act_arg = re.findall( - r"%\{(tx.[^%]*)}", a["act_arg_val"], re.I + r"%\{(tx.[^%]*)}", a["act_arg_val"], flags=re.I ) for v in val_act + val_act_arg: v = v.lower().replace("tx.", "") - if not re.match(r"^\d$", v, re.I): + if not re.match(r"^\d$", v, flags=re.I): if ( v not in globtxvars or phase < globtxvars[v]["phase"] @@ -114,11 +114,11 @@ def check(self, data, globtxvars): globtxvars[v]["used"] = True if "operator_argument" in d: - oparg = re.findall(r"%\{(tx.[^%]*)}", d["operator_argument"], re.I) + oparg = re.findall(r"%\{(tx.[^%]*)}", d["operator_argument"], flags=re.I) if oparg: for o in oparg: o = o.lower() - o = re.sub(r"tx\.", "", o, re.I) + o = re.sub(r"tx\.", "", o, flags=re.I) if ( ( o not in globtxvars diff --git a/tests/test_check_capture.py b/tests/test_check_capture.py new file mode 100644 index 0000000..70f96f4 --- /dev/null +++ b/tests/test_check_capture.py @@ -0,0 +1,385 @@ +import tempfile +import os +from crs_linter.linter import Linter, parse_config + + +def test_capture_check_target_without_capture_fails(): + """Test that using TX.N as target without capture is caught (original functionality).""" + invalid_rule = ( + 'SecRule ARGS "@rx attack" \\\n' + ' "id:1001,\\\n' + ' phase:2,\\\n' + ' deny,\\\n' + ' t:none,\\\n' + ' nolog,\\\n' + ' chain"\n' + ' SecRule TX:0 "@eq attack"' # Uses TX:0 without capture + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(invalid_rule) + temp_file = f.name + + try: + parsed = parse_config(invalid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=invalid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) > 0, \ + "Expected capture error for TX:0 target without capture action" + assert "TX.N" in capture_problems[0].desc + finally: + os.unlink(temp_file) + + +def test_capture_check_msg_without_capture_fails(): + """Test that using %{TX.N} in msg without capture is caught.""" + invalid_rule = ( + 'SecRule ARGS "@rx attack" \\\n' + ' "id:1002,\\\n' + ' phase:2,\\\n' + ' deny,\\\n' + ' t:none,\\\n' + ' msg:\'Attack detected: %{TX.1}\'"' # Uses %{TX.1} without capture + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(invalid_rule) + temp_file = f.name + + try: + parsed = parse_config(invalid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=invalid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) > 0, \ + "Expected capture error for %{TX.1} in msg without capture action" + assert "TX.N" in capture_problems[0].desc + finally: + os.unlink(temp_file) + + +def test_capture_check_logdata_without_capture_fails(): + """Test that using %{TX.N} in logdata without capture is caught.""" + invalid_rule = ( + 'SecRule ARGS "@rx attack" \\\n' + ' "id:1003,\\\n' + ' phase:2,\\\n' + ' deny,\\\n' + ' logdata:\'Matched data: %{TX.0}\'"' # Uses %{TX.0} without capture + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(invalid_rule) + temp_file = f.name + + try: + parsed = parse_config(invalid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=invalid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) > 0, \ + "Expected capture error for %{TX.0} in logdata without capture action" + assert "TX.N" in capture_problems[0].desc + finally: + os.unlink(temp_file) + + +def test_capture_check_setvar_without_capture_fails(): + """Test that using %{TX.N} in setvar without capture is caught.""" + invalid_rule = ( + 'SecRule ARGS "@rx attack" \\\n' + ' "id:1004,\\\n' + ' phase:2,\\\n' + ' pass,\\\n' + " setvar:'tx.foo=%{TX.1}'\"" # Uses %{TX.1} without capture + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(invalid_rule) + temp_file = f.name + + try: + parsed = parse_config(invalid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=invalid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) > 0, \ + "Expected capture error for %{TX.1} in setvar without capture action" + assert "TX.N" in capture_problems[0].desc + finally: + os.unlink(temp_file) + + +def test_capture_check_tag_without_capture_fails(): + """Test that using %{TX.N} in tag without capture is caught.""" + invalid_rule = ( + 'SecRule ARGS "@rx attack" \\\n' + ' "id:1005,\\\n' + ' phase:2,\\\n' + ' pass,\\\n' + ' tag:\'attack-%{TX.0}\'"' # Uses %{TX.0} without capture + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(invalid_rule) + temp_file = f.name + + try: + parsed = parse_config(invalid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=invalid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) > 0, \ + "Expected capture error for %{TX.0} in tag without capture action" + assert "TX.N" in capture_problems[0].desc + finally: + os.unlink(temp_file) + + +def test_capture_check_with_capture_passes(): + """Test that using TX.N with capture action passes.""" + valid_rule = ( + 'SecRule ARGS "@rx (attack)" \\\n' + ' "id:2001,\\\n' + ' phase:2,\\\n' + ' deny,\\\n' + ' capture,\\\n' + ' msg:\'Attack detected: %{TX.1}\',\\\n' + ' logdata:\'Pattern: %{TX.0}\'"' + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(valid_rule) + temp_file = f.name + + try: + parsed = parse_config(valid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=valid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) == 0, \ + f"Expected no capture errors with capture action, but found: {capture_problems}" + finally: + os.unlink(temp_file) + + +def test_capture_check_chained_with_capture_passes(): + """Test that chained rules with capture action pass.""" + valid_rule = ( + 'SecRule ARGS "@rx (attack)" \\\n' + ' "id:2002,\\\n' + ' phase:2,\\\n' + ' deny,\\\n' + ' capture,\\\n' + ' msg:\'Attack detected: %{TX.1}\',\\\n' + ' chain"\n' + ' SecRule TX:1 "@eq attack"' + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(valid_rule) + temp_file = f.name + + try: + parsed = parse_config(valid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=valid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) == 0, \ + f"Expected no capture errors with capture in chained rule, but found: {capture_problems}" + finally: + os.unlink(temp_file) + + +def test_capture_check_multiple_tx_references(): + """Test that multiple TX.N references without capture are all caught.""" + invalid_rule = ( + 'SecRule ARGS "@rx attack" \\\n' + ' "id:1006,\\\n' + ' phase:2,\\\n' + ' deny,\\\n' + ' msg:\'Attack: %{TX.1}\',\\\n' + ' logdata:\'Data: %{TX.0}\'"' # Multiple TX.N references + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(invalid_rule) + temp_file = f.name + + try: + parsed = parse_config(invalid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=invalid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) > 0, \ + "Expected capture error for multiple TX.N references without capture" + finally: + os.unlink(temp_file) + + +def test_capture_check_case_insensitive(): + """Test that TX.N detection is case-insensitive.""" + invalid_rule = ( + 'SecRule ARGS "@rx attack" \\\n' + ' "id:1007,\\\n' + ' phase:2,\\\n' + ' deny,\\\n' + ' msg:\'Attack: %{tx.1}\'"' # Lowercase tx + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(invalid_rule) + temp_file = f.name + + try: + parsed = parse_config(invalid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=invalid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) > 0, \ + "Expected capture error for lowercase %{tx.1} without capture" + finally: + os.unlink(temp_file) + + +def test_capture_check_colon_syntax(): + """Test that TX:N syntax (with colon) is detected.""" + invalid_rule = ( + 'SecRule ARGS "@rx attack" \\\n' + ' "id:1008,\\\n' + ' phase:2,\\\n' + ' deny,\\\n' + ' msg:\'Attack: %{TX:1}\'"' # TX:1 with colon + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(invalid_rule) + temp_file = f.name + + try: + parsed = parse_config(invalid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=invalid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) > 0, \ + "Expected capture error for %{TX:1} (colon syntax) without capture" + finally: + os.unlink(temp_file) + + +def test_capture_check_no_false_positive_on_other_tx_vars(): + """Test that non-captured TX variables (like TX:foo) don't trigger false positives.""" + valid_rule = ( + 'SecRule ARGS "@rx attack" \\\n' + ' "id:2003,\\\n' + ' phase:2,\\\n' + ' pass,\\\n' + ' msg:\'Attack: %{TX.custom_var}\',\\\n' + ' setvar:tx.custom_var=1"' # TX.custom_var is not a captured variable + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(valid_rule) + temp_file = f.name + + try: + parsed = parse_config(valid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=valid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + # Should not report capture errors for non-numeric TX variables + assert len(capture_problems) == 0, \ + f"Should not flag non-captured TX variables, but found: {capture_problems}" + finally: + os.unlink(temp_file) + + +def test_capture_check_operator_argument(): + """Test that TX.N in operator arguments without capture is caught.""" + invalid_rule = ( + 'SecRule ARGS "@rx %{TX.1}" \\\n' # TX.1 in operator argument + ' "id:1009,\\\n' + ' phase:2,\\\n' + ' deny"' + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(invalid_rule) + temp_file = f.name + + try: + parsed = parse_config(invalid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=invalid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) > 0, \ + "Expected capture error for %{TX.1} in operator argument without capture" + finally: + os.unlink(temp_file) + + +def test_capture_check_first_rule_in_chain_allowed(): + """Test that TX.N in first rule of chain without capture is allowed.""" + valid_rule = ( + 'SecRule TX:1 "@eq foo" \\\n' # TX:1 in first rule - allowed + ' "id:2004,\\\n' + ' phase:2,\\\n' + ' pass,\\\n' + ' chain"\n' + ' SecRule ARGS "@rx bar"' + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(valid_rule) + temp_file = f.name + + try: + parsed = parse_config(valid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=valid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + # First rule in chain is allowed to use TX:N without capture + assert len(capture_problems) == 0, \ + f"First rule in chain should be allowed to use TX:N, but found: {capture_problems}" + finally: + os.unlink(temp_file) From 1285348e301f2c61730be648f238715002cad618 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Zipitr=C3=ADa?= <3012076+fzipi@users.noreply.github.com> Date: Tue, 6 Jan 2026 10:29:04 -0300 Subject: [PATCH 02/10] fix: check argument or value for TX.N references Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/crs_linter/rules/check_capture.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/crs_linter/rules/check_capture.py b/src/crs_linter/rules/check_capture.py index b40ca41..e9059c3 100644 --- a/src/crs_linter/rules/check_capture.py +++ b/src/crs_linter/rules/check_capture.py @@ -126,14 +126,16 @@ def check(self, data): capture_level = chainlevel has_capture = True - # Check if action argument contains TX.N reference - if (a["act_name"] in self.actions_to_check and - a.get("act_arg")): - if self.expansion_pattern.search(a["act_arg"]): - if not use_captured_var: - use_captured_var = True - use_captured_var_in_expansion = True - captured_var_chain_level = chainlevel + # Check if action argument (or value) contains TX.N reference + if a["act_name"] in self.actions_to_check: + for field in ("act_arg", "act_arg_val"): + value = a.get(field) + if value and self.expansion_pattern.search(value): + if not use_captured_var: + use_captured_var = True + use_captured_var_in_expansion = True + captured_var_chain_level = chainlevel + break # End of rule/chain - validate if ruleid > 0 and not chained: From 14fafdcdab4faebdd6d7fa851ce8681c95ac4c2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Zipitr=C3=ADa?= <3012076+fzipi@users.noreply.github.com> Date: Tue, 6 Jan 2026 10:31:07 -0300 Subject: [PATCH 03/10] fix: remove guard condition and set flag when expansion pattern Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/crs_linter/rules/check_capture.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/crs_linter/rules/check_capture.py b/src/crs_linter/rules/check_capture.py index e9059c3..50a3236 100644 --- a/src/crs_linter/rules/check_capture.py +++ b/src/crs_linter/rules/check_capture.py @@ -105,8 +105,10 @@ def check(self, data): if self.expansion_pattern.search(op_arg): if not use_captured_var: use_captured_var = True - use_captured_var_in_expansion = True captured_var_chain_level = chainlevel + # Always track that TX.N is used in an expansion, even if it + # was already seen as a target elsewhere in the rule chain. + use_captured_var_in_expansion = True # Check 3: TX.N in action arguments if "actions" in d: From 23f1eb3ec05242d437e1a5be128093fe4c772bf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Zipitr=C3=ADa?= <3012076+fzipi@users.noreply.github.com> Date: Tue, 6 Jan 2026 11:10:40 -0300 Subject: [PATCH 04/10] fix: check capture_level is defined Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/crs_linter/rules/check_capture.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/crs_linter/rules/check_capture.py b/src/crs_linter/rules/check_capture.py index 50a3236..f5f1466 100644 --- a/src/crs_linter/rules/check_capture.py +++ b/src/crs_linter/rules/check_capture.py @@ -151,10 +151,11 @@ def check(self, data): # TX.N used in a chained rule (not the first) if not has_capture or captured_var_chain_level < capture_level: should_error = True - elif use_captured_var_in_expansion and not has_capture: - # TX.N used in expansion (%{TX.N}) without capture - # This is the new check for issue #69 - should_error = True + elif use_captured_var_in_expansion: + # TX.N used in expansion (%{TX.N}); require that capture is + # defined at or before the chain level where TX.N is used. + if not has_capture or capture_level > captured_var_chain_level: + should_error = True if should_error: yield LintProblem( From ef82e627da0dc8494916960a99495492c604e94d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 6 Jan 2026 14:11:10 +0000 Subject: [PATCH 05/10] Initial plan From f5d9047a0332f2ef81acce1e1cbb820e528319e3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 6 Jan 2026 14:16:06 +0000 Subject: [PATCH 06/10] test: add coverage for TX.N used as both target and in expansion Add test case that covers scenario where TX.N appears both as a target variable and in an expansion (e.g., in msg or logdata) within same rule. This test would have caught the bug where use_captured_var_in_expansion was not set correctly when use_captured_var was already True. Also fix the bug by moving use_captured_var_in_expansion assignment outside the guard condition, following the same pattern as c598d31. Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> --- src/crs_linter/rules/check_capture.py | 4 ++- tests/test_check_capture.py | 35 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/crs_linter/rules/check_capture.py b/src/crs_linter/rules/check_capture.py index f5f1466..7acb388 100644 --- a/src/crs_linter/rules/check_capture.py +++ b/src/crs_linter/rules/check_capture.py @@ -135,8 +135,10 @@ def check(self, data): if value and self.expansion_pattern.search(value): if not use_captured_var: use_captured_var = True - use_captured_var_in_expansion = True captured_var_chain_level = chainlevel + # Always track that TX.N is used in an expansion, even if it + # was already seen as a target elsewhere in the rule chain. + use_captured_var_in_expansion = True break # End of rule/chain - validate diff --git a/tests/test_check_capture.py b/tests/test_check_capture.py index 70f96f4..dca2ec3 100644 --- a/tests/test_check_capture.py +++ b/tests/test_check_capture.py @@ -383,3 +383,38 @@ def test_capture_check_first_rule_in_chain_allowed(): f"First rule in chain should be allowed to use TX:N, but found: {capture_problems}" finally: os.unlink(temp_file) + + +def test_capture_check_tx_as_target_and_expansion_without_capture_fails(): + """Test that using TX.N both as target and in expansion without capture is caught. + + This test covers the scenario where TX.N appears as both a target variable + and in an expansion (e.g., in msg or logdata) within the same rule. + This would catch the bug where use_captured_var_in_expansion is not set + correctly when use_captured_var is already True. + """ + invalid_rule = ( + 'SecRule TX:1 "@eq foo" \\\n' # TX:1 as target + ' "id:3001,\\\n' + ' phase:2,\\\n' + ' deny,\\\n' + " msg:'Value: %{TX.1}'\"" # TX.1 in expansion without capture + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(invalid_rule) + temp_file = f.name + + try: + parsed = parse_config(invalid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=invalid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) > 0, \ + "Expected capture error for TX.1 used as both target and in msg expansion without capture" + assert "TX.N" in capture_problems[0].desc + finally: + os.unlink(temp_file) From 5b96339af49d6a98ee2b047da8e4d8a4a2ff9468 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 14 Jan 2026 14:04:52 +0000 Subject: [PATCH 07/10] Initial plan From da48eb2753ca599596cbe39326c73fa5564abd4f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 14 Jan 2026 14:08:46 +0000 Subject: [PATCH 08/10] fix: initialize capture_level to 0 for consistency Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> --- src/crs_linter/rules/check_capture.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crs_linter/rules/check_capture.py b/src/crs_linter/rules/check_capture.py index 7acb388..650c4f3 100644 --- a/src/crs_linter/rules/check_capture.py +++ b/src/crs_linter/rules/check_capture.py @@ -79,7 +79,7 @@ def check(self, data): chained = False ruleid = 0 chainlevel = 0 - capture_level = None + capture_level = 0 has_capture = False use_captured_var = False use_captured_var_in_expansion = False # Track if TX.N is used in expansion From 611f3f8ef3a343d1fe7203055d3d10d473e09ed5 Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Fri, 23 Jan 2026 15:19:24 -0300 Subject: [PATCH 09/10] fix: rule chain has multiple capture actions at different chain levels Signed-off-by: Felipe Zipitria --- src/crs_linter/rules/check_capture.py | 6 ++-- tests/test_check_capture.py | 44 +++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/crs_linter/rules/check_capture.py b/src/crs_linter/rules/check_capture.py index 650c4f3..72c155b 100644 --- a/src/crs_linter/rules/check_capture.py +++ b/src/crs_linter/rules/check_capture.py @@ -125,8 +125,10 @@ def check(self, data): chained = True chainlevel += 1 if a["act_name"] == "capture": - capture_level = chainlevel - has_capture = True + if not has_capture: + capture_level = chainlevel + has_capture = True + # Don't update capture_level if already set; keep the earliest one # Check if action argument (or value) contains TX.N reference if a["act_name"] in self.actions_to_check: diff --git a/tests/test_check_capture.py b/tests/test_check_capture.py index dca2ec3..7555f65 100644 --- a/tests/test_check_capture.py +++ b/tests/test_check_capture.py @@ -418,3 +418,47 @@ def test_capture_check_tx_as_target_and_expansion_without_capture_fails(): assert "TX.N" in capture_problems[0].desc finally: os.unlink(temp_file) + + +def test_capture_check_multiple_captures_in_chain_passes(): + """Test that multiple capture actions in chain don't cause false positives. + + This test covers the bug where having multiple capture actions in a chain + would overwrite capture_level, causing false positives for TX.N usage in + earlier rules that had their own capture action. + + Similar to CRS rule 932205 which has: + - First rule (chainlevel 0): capture action, uses %{TX.2} in logdata + - Second rule (chainlevel 1): another capture action + + The first rule's TX.2 usage should be valid because it has capture at the same level. + """ + valid_rule = ( + 'SecRule ARGS "@rx (foo)(bar)" \\\n' # Capturing groups + ' "id:4001,\\\n' + ' phase:2,\\\n' + ' pass,\\\n' + ' capture,\\\n' # Capture at chainlevel 0 + " logdata:'Matched: %{TX.2}',\\\n" # TX.2 usage at chainlevel 0 + ' chain"\n' + ' SecRule TX:0 "@rx ^foobar$" \\\n' + ' "capture,\\\n' # Another capture at chainlevel 1 + ' t:none"' + ) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: + f.write(valid_rule) + temp_file = f.name + + try: + parsed = parse_config(valid_rule) + assert parsed is not None + + linter = Linter(parsed, filename=temp_file, file_content=valid_rule) + problems = list(linter.run_checks()) + + capture_problems = [p for p in problems if p.rule == "capture"] + assert len(capture_problems) == 0, \ + f"Should not error when TX.N has capture at same level, even with later captures: {capture_problems}" + finally: + os.unlink(temp_file) From 03214f65baba2e435c5306d4ba780f26d53d8b2c Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Sat, 24 Jan 2026 14:47:25 -0300 Subject: [PATCH 10/10] fix: rebase conflict Signed-off-by: Felipe Zipitria --- README.md | 60 +++++++++++++++++++------------------------------------ 1 file changed, 21 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index 5809a1f..a3a251c 100644 --- a/README.md +++ b/README.md @@ -172,23 +172,28 @@ Check that rules using TX.N variables have a corresponding `capture` action. This rule ensures that captured transaction variables (TX:0, TX:1, TX:2, etc.) are only used when a `capture` action has been defined in the rule chain. -<<<<<<< HEAD -Example of a passing rule: +TX.N variables can be referenced in multiple ways: +1. As a rule target: `SecRule TX:1 "@eq attack"` +2. In action arguments: `msg:'Matched: %{TX.1}'`, `logdata:'Data: %{TX.0}'` +3. In operator arguments: `@rx %{TX.1}` +4. In setvar assignments: `setvar:tx.foo=%{TX.1}` + +Example of a passing rule (with capture): ```apache -SecRule ARGS "@rx attack" \ +SecRule ARGS "@rx (attack)" \ "id:2,\ phase:2,\ deny,\ capture,\ - t:none,\ - nolog,\ + msg:'Attack detected: %{TX.1}',\ + logdata:'Pattern: %{TX.0}',\ chain" SecRule TX:1 "@eq attack" ``` -Example of a failing rule (missing capture): +Example of a failing rule (missing capture for target): ```apache SecRule ARGS "@rx attack" \ @@ -198,46 +203,23 @@ SecRule ARGS "@rx attack" \ t:none,\ nolog,\ chain" - SecRule TX:0 "@eq attack" # Fails: uses TX:0 without prior capture + SecRule TX:0 "@eq attack" # Fails: uses TX:0 without capture ``` -======= -TX.N variables can be referenced in multiple ways: -1. As a rule target: `SecRule TX:1 "@eq attack"` -2. In action arguments: `msg:'Matched: %{TX.1}'`, `logdata:'Data: %{TX.0}'` -3. In operator arguments: `@rx %{TX.1}` -4. In setvar assignments: `setvar:tx.foo=%{TX.1}` -Example of a passing rule (with capture): - SecRule ARGS "@rx (attack)" \ - "id:2,\ - phase:2,\ - deny,\ - capture,\ - msg:'Attack detected: %{TX.1}',\ - logdata:'Pattern: %{TX.0}',\ - chain" - SecRule TX:1 "@eq attack" - -Example of a failing rule (missing capture for target): - SecRule ARGS "@rx attack" \ - "id:3,\ - phase:2,\ - deny,\ - t:none,\ - nolog,\ - chain" - SecRule TX:0 "@eq attack" # Fails: uses TX:0 without capture Example of a failing rule (missing capture for action argument): - SecRule ARGS "@rx attack" \ - "id:4,\ - phase:2,\ - deny,\ - msg:'Matched: %{TX.1}'" # Fails: references TX.1 without capture + +```apache +SecRule ARGS "@rx attack" \ + "id:4,\ + phase:2,\ + deny,\ + msg:'Matched: %{TX.1}'" # Fails: references TX.1 without capture +``` + This check addresses the issue found in CRS PR #4265 where %{TX.N} was used in action arguments without verifying that capture was defined. ->>>>>>> f88f28b (feat: add check for TX.N usage without capture) ## CrsTag