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.

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

Incorrect log message from coraza v3 for CRS rule 920450 #570

Closed
rmb122 opened this issue Jan 12, 2023 · 7 comments · Fixed by #792
Closed

Incorrect log message from coraza v3 for CRS rule 920450 #570

rmb122 opened this issue Jan 12, 2023 · 7 comments · Fixed by #792
Assignees
@M4tteoP
Labels
bug Something isn't working

Comments

@rmb122
Copy link

rmb122 commented Jan 12, 2023

Description

The issue is same as owasp-modsecurity/ModSecurity#2423.
For CRS rule 920450 https://github.com/coreruleset/coreruleset/blob/v4.0/dev/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf#L1137, the MATCHED_VAR in log is wrong.

Steps to reproduce

run golang program below

package main

import (
    "fmt"
    "log"
    "net/http"
    "os"
    "strings"

    "github.com/corazawaf/coraza/v3"
    txhttp "github.com/corazawaf/coraza/v3/http"
    "github.com/corazawaf/coraza/v3/types"
)

func exampleHandler(w http.ResponseWriter, req *http.Request) {
    w.Header().Set("Content-Type", "text/plain")
    resBody := "Hello world, transaction not disrupted."

    if body := os.Getenv("RESPONSE_BODY"); body != "" {
        resBody = body
    }

    if h := os.Getenv("RESPONSE_HEADERS"); h != "" {
        kv := strings.Split(h, ":")
        w.Header().Set(kv[0], kv[1])
    }

    // The server generates the response
    w.Write([]byte(resBody))
}

func main() {
    waf := createWAF()

    http.Handle("/", txhttp.WrapHandler(waf, txhttp.StdLogger, http.HandlerFunc(exampleHandler)))

    fmt.Println("Server is running. Listening port: 8090")

    log.Fatal(http.ListenAndServe(":8090", nil))
}

func createWAF() coraza.WAF {
    waf, err := coraza.NewWAF(
        coraza.NewWAFConfig().
            WithErrorCallback(logError).
            WithDirectives(`SecDebugLogLevel 5
SecDebugLog /dev/null
SecRuleEngine On

SecRule &TX:restricted_headers "@eq 0" \
    "id:901165,\
    phase:1,\
    pass,\
    nolog,\
    ver:'OWASP_CRS/4.0.0-rc1',\
    setvar:'tx.restricted_headers=/accept-charset/ /content-encoding/ /proxy/ /lock-token/ /content-range/ /if/'"

SecRule REQUEST_HEADERS_NAMES "@rx ^.*$" \
    "id:920450,\
    phase:1,\
    deny,\
    log,auditlog,\
    capture,\
    t:none,t:lowercase,\
    msg:'HTTP header is restricted by policy (%{MATCHED_VAR})',\
    logdata:'Restricted header detected: %{MATCHED_VAR}',\
    severity:'CRITICAL',\
    setvar:'tx.header_name_%{tx.0}=/%{tx.0}/',\
    chain"
    SecRule TX:/^header_name_/ "@within %{tx.restricted_headers}" \
        "status:437"
`),
    )
    if err != nil {
        log.Fatal(err)
    }
    return waf
}

func logError(error types.MatchedRule) {
    msg := error.ErrorLog(0)
    fmt.Printf("[logError][%s] %s", error.Rule().Severity(), msg)
}

curl the server with restricted header

curl 127.0.0.1:8090 -H "proxy: 1"

Expected result

[client "127.0.0.1"] Coraza: Warning. HTTP header is restricted by policy (proxy) [file ""] [line "0"] [id "920450"] [rev ""] [msg "HTTP header is restricted by policy (proxy)"] [data "Restricted header detected: proxy"] [severity "critical"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/"] [unique_id "fmtcIalunsExuoQXkJC"]

Actual result

[logError][critical] [client "127.0.0.1"] Coraza: Warning. HTTP header is restricted by policy (user-agent) [file ""] [line "0"] [id "920450"] [rev ""] [msg "HTTP header is restricted by policy (user-agent)"] [data "Restricted header detected: user-agent"] [severity "critical"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/"] [unique_id "fmtcIalunsExuoQXkJC"]
[client "127.0.0.1"] Coraza: Warning. HTTP header is restricted by policy (user-agent) [file ""] [line "0"] [id "920450"] [rev ""] [msg "HTTP header is restricted by policy (accept)"] [data "Restricted header detected: accept"] [severity "critical"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/"] [unique_id "fmtcIalunsExuoQXkJC"]
[client "127.0.0.1"] Coraza: Warning. HTTP header is restricted by policy (user-agent) [file ""] [line "0"] [id "920450"] [rev ""] [msg "HTTP header is restricted by policy (proxy)"] [data "Restricted header detected: proxy"] [severity "critical"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/"] [unique_id "fmtcIalunsExuoQXkJC"]
[client "127.0.0.1"] Coraza: Warning. HTTP header is restricted by policy (user-agent) [file ""] [line "0"] [id "920450"] [rev ""] [msg "HTTP header is restricted by policy (host)"] [data "Restricted header detected: host"] [severity "critical"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/"] [unique_id "fmtcIalunsExuoQXkJC"]
[client "127.0.0.1"] Coraza: Warning. HTTP header is restricted by policy (user-agent) [file ""] [line "0"] [id "920450"] [rev ""] [msg ""] [data ""] [severity "critical"] [ver ""] [maturity "0"] [accuracy "0"] [hostname ""] [uri "/"] [unique_id "fmtcIalunsExuoQXkJC"]
@jcchavezs
Copy link
Member

jcchavezs commented Jan 12, 2023 via email

@piyushroshan
Copy link
Contributor

piyushroshan commented Jan 19, 2023 •
edited

IMHO fix should be in the rule where the msg, logdata should be moved in the chain since that is the blocking criteria. Should give that a try.
This works

SecRule REQUEST_HEADERS_NAMES "@rx ^.*$" \
    "id:920450,\
    phase:1,\
    log,auditlog,\
    capture,\
    t:none,t:lowercase,\
    severity:'CRITICAL',\
    setvar:'tx.header_name_%{tx.0}=/%{tx.0}/',\
    chain"
    SecRule TX:/^header_name_/ "@within %{tx.restricted_headers}" \
        "msg:'HTTP header is restricted by policy (%{MATCHED_VAR})',\
        logdata:'Restricted header detected: %{MATCHED_VAR}',\
        deny,\
        status:437"

@jcchavezs
Copy link
Member

Ping @fzipi

@fzipi
Copy link
Member

fzipi commented Jan 20, 2023 •
edited

Well, according to https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-(v2.x)#user-content-chain, metadata actions can be only in the first rule in the chain. 🤷

Also note that disruptive actions, execution phases, metadata actions (id, rev, msg, tag, severity, logdata), skip, and skipAfter actions can be specified only by the chain starter rule.

@jptosso
Copy link
Member

jptosso commented Jan 20, 2023

Well, according to https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-(v2.x)#user-content-chain, metadata actions can be only in the first rule in the chain. 🤷

Also note that disruptive actions, execution phases, metadata actions (id, rev, msg, tag, severity, logdata), skip, and skipAfter actions can be specified only by the chain starter rule.

I think we can change this in coraza without affecting regression. It does make sense to change metadata in different chain stages. It could be allowed for certain actions, we could even add a new group like "conditional actions" or chained actions

@jcchavezs jcchavezs added the bug Something isn't working label Feb 7, 2023
@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 Mar 10, 2023
@github-actions
Copy link

This issue was closed because it has been inactive for 14 days since being marked as stale.

@M4tteoP M4tteoP mentioned this issue May 12, 2023
Merged
@jptosso jptosso removed the stale label May 16, 2023
@jptosso jptosso reopened this May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@M4tteoP M4tteoP

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix/feat: Macro expansions, error logs redundancy, support msg/logdata in inner rules M4tteoP/coraza
6 participants
@piyushroshan @jptosso @fzipi @jcchavezs @M4tteoP @rmb122