corazawaf / coraza

OWASP Coraza WAF is a golang modsecurity compatible web application firewall library
https://www.coraza.io
Apache License 2.0
2.24k stars 222 forks source link

Incorrect Macro Expansions for Log and Msg of Chained Rule #193

Closed piyushroshan closed 2 years ago

piyushroshan commented 2 years ago

Description

The MATCHED_VAR_NAME is taken from the chained rule. It should instead be taken from the rule where msg action is defined.

Variables are expanded incorrectly for Log and Msg of Rule

MATCHED_VAR expands to REQUEST_HEADERS:host i.e key but it should be attack i.e. value of MATCHED_VAR MATCHED_VAR_NAME expands to MATCHED_VARS_NAMES:REQUEST_HEADERS:host i.e. key:value but it should be value of MATCHED_VAR_NAME

It seems the msg, data gets overridden as the chain rule gets evaluated.

Steps to reproduce

rules = `
SecRule REQUEST_HEADERS "@rx attack" \
    "id:20,\
    phase:1,\
    log,\
    msg:'Chained rule test',\
    logdata:'Found %{MATCHED_VAR} in %{MATCHED_VAR_NAME}',\
    chain"
    SecRule MATCHED_VARS_NAMES "@rx host" \
        "block"
`
waf := coraza.NewWaf()
parser, err := seclang.NewParser(waf)
if err != nil {
    fmt.Println(err)
}
err = parser.FromString(rules)
if err != nil {
    fmt.Println(err)
}
tx := waf.NewTransaction()
tx.ForceRequestBodyVariable = true
tx.AddRequestHeader("Host", "attacker")
tx.ProcessURI("/test", "POST", "HTTP/1.1")
tx.ProcessRequestHeaders()
tx.ProcessRequestBody()
fmt.Println("Interruption", tx.Interruption)
for _, match := range tx.MatchedRules {
    fmt.Println("matched Msg ", match.Message)
    fmt.Println("matched Data ", match.Data)
}

Expected result

matched Msg  Chained rule test
matched Data  Found attack in REQUEST_HEADERS:host

Actual result

matched Msg  Chained rule test
matched Data  Found REQUEST_HEADERS:host in MATCHED_VARS_NAMES:REQUEST_HEADERS:host
piyushroshan commented 2 years ago

IMHO this would not work.

A rule like the following would require preserving the last chain. So it would be better to preserve the match message and data from the rule/subrule where the msg, logdata action are specified. I have a fix in work that solves this.

SecRule REQUEST_HEADERS "@rx attack" \
    "id:20,\
    phase:1,\
    log,\
    chain"
    SecRule MATCHED_VARS_NAMES "@rx host" \
         msg:'Chained rule test',\
         logdata:'Found %{MATCHED_VAR} in %{MATCHED_VAR_NAME}',\
        "block"
ShiMing-Q commented 2 years ago

IMHO this would not work.

A rule like the following would require preserving the last chain. So it would be better to preserve the match message and data from the rule/subrule where the msg, logdata action are specified. I have a fix in work that solves this.

SecRule REQUEST_HEADERS "@rx attack" \
    "id:20,\
    phase:1,\
    log,\
    chain"
    SecRule MATCHED_VARS_NAMES "@rx host" \
         msg:'Chained rule test',\
         logdata:'Found %{MATCHED_VAR} in %{MATCHED_VAR_NAME}',\
        "block"

@piyushroshan If there are more than one chained rules, say 3, each chained rules has its logdata. Then how we will handle this? Or this is not a valid case?

github-actions[bot] commented 2 years ago

This issue is stale because it has been open for 30 days with no activity.

jptosso commented 2 years ago

This was fixed, right @piyushroshan ?

piyushroshan commented 2 years ago

Yes.