8000 Unused variables · Issue #2990 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Unused variables #2990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
airween opened this issue Nov 10, 2022 · 4 comments · Fixed by #3043
Closed

Unused variables #2990

airween opened this issue Nov 10, 2022 · 4 comments · Fixed by #3043

Comments

@airween
Copy link
Contributor
airween commented Nov 10, 2022

Describe the bug

It's not a real bug, more of a cleanup discussion.

The current CRS v4.0/dev branch has three transaction variables, which set (they occur in a setvar action's left side operand), but never used (don't occur either as a target, in the right-hand operand of a setvar action, or msg...):

Motivation

As you know, I added few new features to crs-rules-check tool, but haven't committed yet. The reason is that the script checks the unused variables. If we want this feature, before the commit we have to remove these variables or resolve its states.

If we do not want this feature, I can remove it, or make it optionally, possibly gives it warning instead of error (if Github action handles different levels).

Unused variables

real_ip

occurs here, in line 305:

296  SecAction \
297      "id:901321,\
298      phase:1,\
299      pass,\
300      t:none,\
301      nolog,\
302      ver:'OWASP_CRS/4.0.0-rc1',\
303      initcol:global=global,\
304      initcol:ip=%{remote_addr}_%{tx.ua_hash},\
305      setvar:'tx.real_ip=%{remote_addr}'"

There is no discussion about this TX variable yet.

942521_full

occurs here in line 1476

1457  SecRule ARGS_NAMES|ARGS|XML:/* "@rx (?i)^(?:[^\"]*?(?:\"[^\"]*?\"[^\"]*?)*?\"|[^']*?(?:'[^']*?'[^']*?)*?'|[^`]*?(?:`[^`]*?`[^`]*?)*?`)\s*(\w+)\b" \
1458      "id:942521,\
1459      phase:2,\
1460      block,\
1461      capture,\
1462      t:none,t:urlDecodeUni,\
1463      msg:'Detects basic SQL authentication bypass attempts 4.1/4',\
1464      logdata:'Matched Data: %{TX.0} found within %{MATCHED_VAR_NAME}: %{MATCHED_VAR}',\
1465      tag:'application-multi',\
1466      tag:'language-multi',\
1467      tag:'platform-multi',\
1468      tag:'attack-sqli',\
1469      tag:'OWASP_CRS',\
1470      tag:'capec/1000/152/248/66',\
1471      tag:'PCI/6.5.2',\
1472      tag:'paranoia-level/2',\
1473      ver:'OWASP_CRS/4.0.0-rc1',\
1474      severity:'CRITICAL',\
1475      setvar:'tx.942521_lhs=%{TX.1}',\
1476      setvar:'tx.942521_full=%{TX.0}',\
1477      chain"
1478      SecRule TX:942521_lhs "@rx ^(?:and|or)$" \
1479          "t:none,\
1480          setvar:'tx.sql_injection_score=+%{tx.critical_anomaly_score}',\
1481          setvar:'tx.inbound_anomaly_score_pl2=+%{tx.critical_anomaly_score}'"

We discussed about this variable here, looks like it can be removed.

anomaly_score

occurs here in lines 35 and 36

23  SecAction \
24      "id:980099,\
25      phase:5,\
26      pass,\
27      t:none,\
28      nolog,\
29      noauditlog,\
30      ver:'OWASP_CRS/4.0.0-rc1',\
31      setvar:'tx.blocking_anomaly_score=%{tx.blocking_inbound_anomaly_score}',\
32      setvar:'tx.blocking_anomaly_score=+%{tx.blocking_outbound_anomaly_score}',\
33      setvar:'tx.detection_anomaly_score=%{tx.detection_inbound_anomaly_score}',\
34      setvar:'tx.detection_anomaly_score=+%{tx.detection_outbound_anomaly_score}',\
35      setvar:'tx.anomaly_score=%{tx.blocking_inbound_anomaly_score}',\
36      setvar:'tx.anomaly_score=+%{tx.blocking_outbound_anomaly_score}'"

Discussion about this variable is here. If we want to keep this variable, we should initialize it in REQUEST-901-INITIALIZATION.conf, and later eg. in RESPONSE-980-CORRELATION.conf we show the value in a log message.

I think the first two items can be removed. But what about the anomaly_score?

Any helps are welcome.
@fzipi
Copy link
Member
fzipi commented Nov 11, 2022

tx.real_ip was used by the IP Reputation and DoS Protection rules in v3.3 (910100, 910110, 910140 - 912120, 912170 and 921171 rules).

It is difficult to move, I think, if we are using it in two different potential plugins. Technically the core should be providing it to plugins to be used. For sure it deserves to be better documented?

@airween
Copy link
Contributor Author
airween commented Nov 11, 2022

Technically the core should be providing it to plugins to be used.

I do not consider this solution lucky, rather I would prefer that each plugin initializes its own variable. But it's just an opinion.

If we support that core provides the variables for plugins, then we should initialize this (these) variable(s), and show them at the and of transaction - just for to keep the rule variables consistent.

If it is not possible, I can remove this feature, or make it optionally, probably change the output level (error -> warning/notice).

@franbuehler franbuehler mentioned this issue Nov 21, 2022
Closed
11 tasks
@franbuehler
Copy link
Contributor

Meeting decision Nov: variables real_ip and 942521_full should be removed. anomaly_score is there for audit-console (some old software) compatibility, we want to keep it. -> Next question: initialize or allowlist? Decision: we initialize this variable in rule 901200, and display its value in rule 980170, name proposal: COMBINED_SCORE=%{tx.anomaly_score}

@airween airween mentioned this issue Dec 5, 2022
Merged
@airween
Copy link
Contributor Author
airween commented Dec 5, 2022

See #3043.

@theseion theseion mentioned this issue Mar 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Cleaning up TX variables airween/coreruleset
3 participants
@airween @fzipi @franbuehler
0