Crash if a rule ends with a `%`

[blotus]

Description

Coraza crashes if a rule use some (most ?) operators (@contains for example) and it ends with a % due to a bug in the macro expansion code.

It seems that everything that calls macro.NewMacro is impacted (so operators, msg field, ...).

Steps to reproduce

SecRule ARGS:id "@contains %" "id:1, phase:1,deny, status:403,msg:'crash rule',log,auditlog"

SecRule ARGS:id "foo" "id:1, phase:1,deny, status:403,msg:'crash rule %',log,auditlog"

The last rule gives this stacktrace (using the example http-server from the repo):

2024/10/23 17:06:06 [DEBUG] Parsing directive line="SecRule ARGS:id \"foo\" \"id:1, phase:1,deny, status:403,msg:'crash rule %',log,auditlog\""
panic: runtime error: index out of range [12] with length 12

goroutine 1 [running]:
github.com/corazawaf/coraza/v3/experimental/plugins/macro.(*macro).compile(0xc0000bd5c0, {0xc0000340fb, 0xc})
    /home/seb/git/coraza/experimental/plugins/macro/macro.go:107 +0x9b2
github.com/corazawaf/coraza/v3/experimental/plugins/macro.NewMacro({0xc0000340fb, 0xc})
    /home/seb/git/coraza/experimental/plugins/macro/macro.go:31 +0x51
github.com/corazawaf/coraza/v3/internal/actions.(*msgFn).Init(0xc0000340d7?, {0x8cf0a0, 0xc000002180}, {0xc0000340fb?, 0x14550?})
    /home/seb/git/coraza/internal/actions/msg.go:31 +0x7d
github.com/corazawaf/coraza/v3/internal/seclang.(*RuleParser).ParseActions(0xc00003d4e8, {0xc0000340d7?, 0x7?})
    /home/seb/git/coraza/internal/seclang/rule_parser.go:274 +0x1eb
github.com/corazawaf/coraza/v3/internal/seclang.ParseRule({0x1, 0xc000208000, {{0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}, {0x0, 0x0, ...}, ...}, ...})
    /home/seb/git/coraza/internal/seclang/rule_parser.go:372 +0x3eb
github.com/corazawaf/coraza/v3/internal/seclang.directiveSecRule(0xc0001f20e0)
    /home/seb/git/coraza/internal/seclang/directives.go:180 +0x118
github.com/corazawaf/coraza/v3/internal/seclang.(*Parser).evaluateLine(0xc00003dc28, {0xc0000340c0, 0x56})
    /home/seb/git/coraza/internal/seclang/parser.go:186 +0x3cb
github.com/corazawaf/coraza/v3/internal/seclang.(*Parser).parseString(0xc00003dc28, {0xc000036090, 0x83})
    /home/seb/git/coraza/internal/seclang/parser.go:130 +0x4dd
github.com/corazawaf/coraza/v3/internal/seclang.(*Parser).FromFile(0xc00003dc28, {0x82fdbb, 0xe})
    /home/seb/git/coraza/internal/seclang/parser.go:67 +0x29b
github.com/corazawaf/coraza/v3.NewWAF({0x8d2168?, 0xc00003dd00})
    /home/seb/git/coraza/waf.go:64 +0x24f
main.createWAF()
    /home/seb/git/coraza/examples/http-server/main.go:48 +0x2e9
main.main()
    /home/seb/git/coraza/examples/http-server/main.go:33 +0x17

Expected result

Not crashing :)

The current behavior with invalid macros is also kinda surprising: the invalid macro is just dropped from the rule. This means that a rule like SecRule ARGS:id "@contains %{" "id:1, phase:1,deny, status:403,msg:'foobar',log,auditlog" will actually end up being a @contains with an empty value (but weirdly enough %{ will be logged as the operator data in the debug logs), and will match on everything, which shouldn't be possible as @contains requires an argument (this is technically a different problem, let me know if you want to track this in a separate issue).

If an invalid macro is encountered, it would probably be best to just leave it as it (and log a warning/error to the user)

[jptosso]

Thanks this is a great finding we will take a look

[jcchavezs]

Great finding!