multimatch applies operator too often

[dstelter]

Consider this rule:

SecRule ARGS:c ".*" "id:4,multimatch,t:none,t:lowercase,t:lowercase,t:lowercase,msg:%{matched_var}"

Intuitively, this rule should never invoke the operator more than twice, right?

My test cases look like this:

{
    "request": "/?c=test",
    "comment": "Only initial apply",
    "matches": [
        {"id": 4, "msg": "test"}
    ]
},
{
    "request": "/?c=Test",
    "comment": "Initial apply and after first lowercase transform",
    "matches": [
        {"id": 4, "msg": "Test"},
        {"id": 4, "msg": "test"}
    ]
}

The first test is successful, but the second request yields these matches ("(id, msg)"):

('4', 'Test'), ('4', 'test'), ('4', 'test')

The operator is applied once too often if there are more multiple transforms and any of them modify the input. This problem only occurs if multiple transforms are present. The operator is always applied with the final transform value.

Another case:

Rule:

SecRule ARGS:b ".*" "id:3,multimatch,t:none,t:lowercase,t:hexEncode,t:length,t:removeNulls,msg:%{matched_var}"

Test:

{
    "request": "/?b=Test",
    "matches": [
        {"id": 3, "msg": "Test"},
        {"id": 3, "msg": "test"},
        {"id": 3, "msg": "74657374"},
        {"id": 3, "msg": "8"}
    ]
}

Reported matches:

('3', 'Test'), ('3', 'test'), ('3', '74657374'), ('3', '8'), ('3', '8')

Removing the removeNulls transform makes the duplicate match disappear, but it shouldn't appear at all if removeNulls doesn't modify the input.

Am I misinterpreting the multimatch specification or is there a bug?

[dstelter]

Also, there is an interesting edge case: If a multimatch rule has no transforms at all, the operator is never applied.

This is consistent with the reference, yet somewhat unintuitive:

With multiMatch, variables are checked against the operator before and after every transformation function that changes the input.

[csanders-git]

@dstelter you can think of it being applied but since there are no transformations is just the initial match

[csanders-git]

@dstelter this is an interesting reduction perhaps this information should be stored as a set. For now its just an inefficiency, correct - no missing data?

[dstelter]

Regarding no-match-at-all:
I would expect it to fire the initial match, but this never happens.

SecRule REQUEST_BASENAME ".*" "id:1,msg:'block everything',block,multimatch"

This rule will never block anything!

[dstelter]

Firing the operator too often can have unintended side effects if the action increases anomaly score.
The specific rule that bit me is CRS/950901: https://github.com/SpiderLabs/owasp-modsecurity-crs/blob/master/base_rules/modsecurity_crs_41_sql_injection_attacks.conf#L76

I suppose one could/should refactor this to increase the score in a chained subrule, but the underlying issue remains. I'll submit a PR in a moment.

[csanders-git]

Thanks for the pull - we'll run it through the build bot forthcoming. In general i think you are right this pull request is kind of a double edged sword esp with anomaly score.