Skip to content
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.

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

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

Closed
piyushroshan opened this issue Mar 3, 2022 · 5 comments
Closed

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

piyushroshan opened this issue Mar 3, 2022 · 5 comments
Labels
bug Something isn't working stale

Comments

@piyushroshan
Copy link
Contributor

piyushroshan commented Mar 3, 2022

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 piyushroshan changed the title Incorrect Macro Expansions for Log and Msg of Rule Incorrect Macro Expansions for Log and Msg of Chained Rule Mar 3, 2022
@jptosso jptosso added the bug Something isn't working label Mar 3, 2022
ShiMing-Q pushed a commit to ShiMing-Q/coraza-waf that referenced this issue Mar 13, 2022
ShiMing-Q pushed a commit to ShiMing-Q/coraza-waf that referenced this issue Mar 15, 2022
ShiMing-Q pushed a commit to ShiMing-Q/coraza-waf that referenced this issue Mar 19, 2022
@piyushroshan
Copy link
Contributor Author

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"

@jptosso jptosso mentioned this issue Mar 21, 2022
3 tasks
@ShiMing-Q
Copy link
Contributor

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?

@piyushroshan piyushroshan mentioned this issue Apr 11, 2022
3 tasks
@jptosso jptosso linked a pull request Apr 11, 2022 that will close this issue
3 tasks
@github-actions
Copy link

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

@github-actions github-actions bot added the stale label Apr 21, 2022
@jptosso
Copy link
Member

jptosso commented Apr 22, 2022

This was fixed, right @piyushroshan ?

@piyushroshan
Copy link
Contributor Author

Yes.

@jptosso jptosso closed this as completed Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants