Multiphase work

http/middleware_test.go

@@ -123,14 +123,14 @@ func createMultipartRequest(t *testing.T) *http.Request {
 }
 
 // from issue https://github.com/corazawaf/coraza/issues/159 @zpeasystart
-func TestDirectiveSecAuditLog(t *testing.T) {
+func TestChainEvaluation(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	waf.RequestBodyAccess = true
 	if err := seclang.NewParser(waf).FromString(`
 	SecRule REQUEST_FILENAME "@unconditionalMatch" "id:100, phase:2, t:none, log, setvar:'tx.count=+1',chain"
-	SecRule ARGS:username "@unconditionalMatch" "t:none, setvar:'tx.count=+2',chain"
-	SecRule ARGS:password "@unconditionalMatch" "t:none, setvar:'tx.count=+3'"
-		`); err != nil {
+		SecRule ARGS:username "@unconditionalMatch" "t:none, setvar:'tx.count=+2',chain"
+			SecRule ARGS:password "@unconditionalMatch" "t:none, setvar:'tx.count=+3'"
+	`); err != nil {
 		t.Fatal(err)
 	}
 	if err := waf.Validate(); err != nil {
@@ -286,6 +286,7 @@ func TestHttpServer(t *testing.T) {
 			expectedProto:  "HTTP/1.1",
 			expectedStatus: 201,
 		},
+		// TODO(MultiPhase)[ARGS_forced_phased2]: rule 10 has ARGS:id, therefore it is evaluated only at phase 2.
 		"deny passes over allow due to ordering": {
 			reqURI:         "/allow_me?id=0",
 			expectedProto:  "HTTP/1.1",

internal/corazawaf/rule.go

@@ -245,6 +245,10 @@ func (r *Rule) doEvaluate(phase types.RulePhase, tx *Transaction, cache map[tran
 						}
 					}
 				}
+			} else if multiphaseEvaluation && (r.HasChain && phase < r.chainMinPhase) {
+				// When multiphase evaluation is enabled, if the variable is available, but the whole chain is not,
+				// we don't evaluate tue rule yet.
+				continue
 			}
 			var values []types.MatchData
 			for _, c := range ecol {

internal/corazawaf/rulegroup.go

@@ -212,7 +212,7 @@ RulesLoop:
 	if tx.AllowType == corazatypes.AllowTypePhase {
 		tx.AllowType = corazatypes.AllowTypeUnset
 	}
-	// // Reset Skip counter at the end of each phase. Skip actions work only within the current processing phase
+	// Reset Skip counter at the end of each phase. Skip actions work only within the current processing phase
 	tx.Skip = 0
 
 	tx.stopWatches[phase] = time.Now().UnixNano() - ts

internal/seclang/rules_test.go

@@ -39,6 +39,10 @@ func TestRuleMatch(t *testing.T) {
 	}
 }
 
+// TODO(MultiPhase)[ARGS_forced_phased2]: Via minPhase we are forcing that ARGS has to be evaluated in phase 2.
+// This tests expects that ARGS is evaluated in phase 1, and a matched rule already happens right after ProcessRequestHeaders
+// Solution: minPhase (or the check) should also look at the inferredPhases slice. Phase 1 is true, so pick the minimal between
+// Inferred and minPhase.
 func TestRuleMatchWithRegex(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	parser := NewParser(waf)
@@ -152,6 +156,7 @@ func TestSecAuditLogs(t *testing.T) {
 	}
 }
 
+// TODO(MultiPhase)[ARGS_forced_phased2]
 func TestRuleLogging(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	var logs []string
@@ -189,14 +194,15 @@ func TestRuleLogging(t *testing.T) {
 	}
 }
 
+// TODO(MultiPhase)[ARGS_forced_phased2]
 func TestRuleChains(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	parser := NewParser(waf)
 	err := parser.FromString(`
 		SecRule ARGS "123" "id:1,phase:1,log,chain"
 			SecRule &ARGS "@gt 0" "chain"
 			SecRule ARGS "456" "setvar:'tx.test=ok'"
-		
+
 		SecRule ARGS "123" "id:2,phase:1,log,chain"
 			SecRule &ARGS "@gt 100" "chain"
 			SecRule ARGS "456" "setvar:'tx.test2=fail'"
@@ -213,6 +219,7 @@ func TestRuleChains(t *testing.T) {
 	}
 }
 
+// TODO(MultiPhase)[ARGS_forced_phased2]
 func TestTagsAreNotPrintedTwice(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	var logs []string
@@ -245,6 +252,7 @@ func TestTagsAreNotPrintedTwice(t *testing.T) {
 	}
 }
 
+// TODO(MultiPhase)[ARGS_forced_phased2]
 func TestStatusFromInterruptions(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	var logs []string

magefile.go

@@ -118,6 +118,14 @@ func Coverage() error {
 	if err := sh.RunV("go", "test", "-coverprofile=build/coverage-ftw.txt", "-covermode=atomic", "-coverpkg=./...", "./testing/coreruleset"); err != nil {
 		return err
 	}
+	// Execute coverage tests with multiphase evaluation enabled
+	if err := sh.RunV("go", "test", "-race", "-coverprofile=build/coverage-multiphase.txt", "-covermode=atomic", "-coverpkg=./...", "-tags=coraza.rule.multiphase_evaluation", "./..."); err != nil {
+		return err
+	}
+	// Execute coverage tests with multiphase evaluation enabled
+	if err := sh.RunV("go", "test", "-race", "-coverprofile=build/coverage-examples.txt", "-covermode=atomic", "-tags=coraza.rule.multiphase_evaluation", "-coverpkg=./...", "./examples/http-server"); err != nil {
+		return err
+	}
 	// Execute FTW tests with multiphase evaluation enabled as well
 	if err := sh.RunV("go", "test", "-coverprofile=build/coverage-ftw-multiphase.txt", "-covermode=atomic", "-coverpkg=./...", "-tags=coraza.rule.multiphase_evaluation", "./testing/coreruleset"); err != nil {
 		return err

testing/auditlog_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/corazawaf/coraza/v3/internal/seclang"
 )
 
+// TODO(MultiPhase)[ARGS_forced_phased2]
 func TestAuditLogMessages(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	parser := seclang.NewParser(waf)
@@ -106,6 +107,7 @@ func TestAuditLogRelevantOnly(t *testing.T) {
 	}
 }
 
+// TODO(MultiPhase)[ARGS_forced_phased2]
 func TestAuditLogRelevantOnlyOk(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	parser := seclang.NewParser(waf)

testing/engine/allow.go

@@ -127,12 +127,56 @@ var _ = profile.RegisterProfile(profile.Profile{
 						},
 					},
 				},
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI:    "/response_allow",
+							Method: "POST",
+							Headers: map[string]string{
+								"Content-type": "application/x-www-form-urlencoded",
+							},
+							Data: "response_allow",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{
+								70,
+							},
+							NonTriggeredRules: []int{
+								71,
+							},
+							Interruption: &profile.ExpectedInterruption{
+								Status: 500,
+								Data:   "",
+								RuleID: 70,
+								Action: "deny",
+							},
+						},
+					},
+				},
 			},
 		},
 	},
+	// TODO(MultiPhase)[MovingIntoAllowedPhase]: see rules 45 and 46. Rule 45 allows all the request phases (1 and 2)
+	// Rule 46 is anticipated to phase 1, therefore it is skept. When phase 3 is evaluated, rule 46 is skipped because
+	// it is not anymore in its minPhase.
+	// It applies also to rules 31-42 and 11-22.
+	// Possible solution: we have to find a way to be sure that rules have been evaluated in an inferred phase or not.
+	// A solution with overhead would be adding a boolean for each rule.
+	// We have to consider at least two cases:
+	// 1) The rule anticipated has been evaluated in the inferred phase before an allow action happend (it is okay do not evaluate afterwards)
+	// 2) The rule anticipated has not been evaluated because of an allow action. We have to try to evaluate it in other inferred phases)
+
+	// TODO(MultiPhase)[MovingAllowingRules]: Moving rules with allow action leads to unwanted allowed requests.
+	// See rules 70 and 71. Rule 71, being anticipated at phase:1, is evaluated before rule 70. The latter should have denied the request at phase:2
+	// Possible solution: assegnation of Inferred phases should check for allow actions and do not assign the earlier phases to rules with allow action.
+	// Possible solution: Evaluate the rule, but wait before enforcing the allow action untile the right phase is reached. Problems can arise because of rules orderin.
+	// E.g. a phase 3 with "allow", anticipated at phase:1 could delay the allow action when phase:3 is reached, but the rules order is not respected. Maybe other
+	// phase 3 rules should have been evaluated before the allow action.
+
 	Rules: `
 SecDebugLogLevel 5
 SecRequestBodyAccess On
+
 SecRule REQUEST_URI "/allow_me" "id:1,phase:1,allow,msg:'ALLOWED'"
 SecRule REQUEST_URI "/allow_me" "id:2,phase:1,deny,msg:'DENIED'"
 
@@ -151,18 +195,22 @@ SecRule REQUEST_URI "/request_allow" "id:34,phase:2,deny,msg:'NOT DENIED'"
 SecRule REQUEST_URI "/request_allow" "id:42,phase:3,deny,status:500,msg:'DENIED'"
 
 # Rule 45 allows only request phases (1 and 2), it should not impact on phase 3.
-# Therefor, rule 46 is expected to be triggered.
+# Therefore, rule 46 is expected to be triggered.
 SecRule REQUEST_URI "/useless_request_allow" "id:45,phase:1,allow:request,msg:'Allowed at the request'"
 SecRule REQUEST_URI "/useless_request_allow" "id:46,phase:3,deny,status:500,msg:'DENIED'"
 
 # Rule 50 allows only the current phase (phase 1), it should not impact any other rule (being part of other phases).
-# Rule 61 is meant to allow only from phase 3, rule 51, at phase 2 should deny the request
+# Rule 61 is meant to allow only from phase 3 (so phase 3 and 4), rule 51, at phase 2 should deny the request
 # before reaching phase 3. Therefore rule 61 and 62 should not be triggered.
-# Suitable for testing that allow:phase is not propagated to other phases and for testing
-# multiphase evaluation combined with with allow actions.
+# Suitable for testing that allow:phase is not propagated to other phases
 SecRule REQUEST_URI "/allow_only_response" "id:50,phase:1,allow:phase,msg:'Allowed phase 1'"
 SecRule REQUEST_BODY "allow_only_response" "id:51,phase:2,deny,status:500,msg:'Denied request'"
 SecRule REQUEST_URI "/allow_only_response" "id:61,phase:3,allow,msg:'Allowed Response not triggered'"
 SecRule REQUEST_URI "/allow_only_response" "id:62,phase:4,deny,msg:'Deny response not triggered'"
+
+# Rule 70 should deny the request at phase:2 before rule 71.
+# Suitable for testing multiphase evaluation combined with allow actions.
+SecRule REQUEST_BODY "response_allow" "id:70,phase:2,deny,status:500,msg:'Denied request'"
+SecRule REQUEST_URI "/response_allow" "id:71,phase:3,allow,msg:'Allowed Response not triggered'"
 `,
 })

testing/engine/chains_multiphase.go

@@ -0,0 +1,54 @@
+// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors
+// SPDX-License-Identifier: Apache-2.0
+
+//go:build coraza.rule.multiphase_evaluation
+// +build coraza.rule.multiphase_evaluation
+
+package engine
+
+import (
+	"github.com/corazawaf/coraza/v3/testing/profile"
+)
+
+var _ = profile.RegisterProfile(profile.Profile{
+	Meta: profile.Meta{
+		Author:      "M4tteoP",
+		Description: "Test if the chain action works with multiphase evaluation specific tests",
+		Enabled:     true,
+		Name:        "chains_multiphase.yaml",
+	},
+	Tests: []profile.Test{
+		{
+			Title: "chains",
+			Stages: []profile.Stage{
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI:     "/chain_phase2",
+							Method:  "POST",
+							Headers: map[string]string{"Content-Type": "application/x-www-form-urlencoded"},
+							Data:    "chain_phase2",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules:    []int{10},
+							NonTriggeredRules: []int{11, 12, 13},
+						},
+					},
+				},
+			},
+		},
+	},
+	Rules: `
+SecDebugLogLevel 5
+SecRequestBodyAccess On
+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'"
+SecRule REQUEST_URI "/chain_phase2" "id:11, phase:3, t:none, log, chain"
+	SecRule TX:set1 "!@eq 1" "deny"
+SecRule REQUEST_URI "/chain_phase2" "id:12, phase:3, t:none, log, chain"
+	SecRule TX:set1 "!@eq 2" "deny"
+SecRule REQUEST_URI "/chain_phase2" "id:13, phase:3, t:none, log, chain"
+	SecRule TX:set1 "!@eq 3" "deny"
+`,
+})

testing/engine/skip.go

@@ -118,6 +118,9 @@ var _ = profile.RegisterProfile(profile.Profile{
 			},
 		},
 	},
+	// TODO(MultiPhase)[[MovingSkipAndSkipAfterRules] Similar to MovingAllowingRules, being actions that alter the these in which they are triggered,
+	// anticipating them can lead to wrong flows. Possible solution: do not permit to anticipate rules with Skip and SkipAfter. It also would permit
+	// to be safe with rules with multiple targets and relative actions that I think would be executed twice
 	Rules: `
 SecDebugLogLevel 5
 SecRequestBodyAccess On