I would not call it an accident, but definitely something that will lead to false positives.

Simply removing them - as you propose - is a straightforward solution, but we also loose detection capabilities that way.

We do not really have a regex for php configuration directives, but the @pmf approach is hard to fix.

Anybody has a proposal?

I can confirm they were re-added in CRSv4 as part of the bug bounty see: https://github.com/coreruleset/coreruleset/pull/3028/files#diff-e08f25b74f02a4dfc08f6dbba996d2d811c0d67324c72c63ea97dd57e10f5af5R106

It doesn't look like these words are causing many false positives. I can't find any reports of this causing false positives, and I haven't encountered any personally. An easy fix would be to move these to PL-2, or we could move the false positive prone words to another rule using a regular expression that does something like this engine(?:\s+)?=. I'm not sure how bypassable the latter would be.

Same thinking here.

I mean the rule looks for the keywords and then it goes on to match on =. So engine is not enough. The parameter also needs to contain = which reduces FPs quite a bit I suppose.

@ssigwart have you seen FPs in the wild, or did you just stumble over the keywords when looking at the rules (which would be a perfectly valid reason for a report, it's more a question of perspective).

Thanks, @dune73 and @EsadCetiner. I'm upgrading from version 3 to 4 now and was comparing them to thwart off potential FPs.

Even with the = sign, I thought these were highly likely to have an FP when used in text areas. For example, here are a couple plausible FPs.

Our promise = No bioengineered ingredients.

For extension pricing, see below:
Option 1 = $10
Option 2 = $20

Tour Pricing Below:
Engine = $30
Cabin = $10
Caboose = $20

I don't know a ton about the vulnerability to know the impact, but I was thinking that requiring only whitespace after would cut down on a lot of FPs. It wouldn't stop the FP on Engine = $30 though.

That's a good price for the Cabin.

Thanks for the context. Yes, FPs like this are totally possible.

This rule does not necessarily look at an exploit but it searches requests for traces of PHP code.

Also: Your thinking how to reduce likeliness of FPs is good, but unfortunately, this is very hard to express in ModSecurity rule language. The problems is we have the entire parameter and the only thing we we know is that somewhere in that parameter, there is one of the keywords.

I'm not too familiar with the rule language, but would there be a way to change the chain rule to something like this:

SecRule MATCHED_VARS "@pm %{tx.933120_tx_0}=" \
        "capture,\
        t:removeWhitespace,\
        setvar:'tx.php_injection_score=+%{tx.critical_anomaly_score}',\
        setvar:'tx.inbound_anomaly_score_pl1=+%{tx.critical_anomaly_score}'"

I haven't gotten it to actually work, but if you could remove whitespace and then check for the matched word (e.g. "engine"), followed but an equal sign, that might solve this.

Nice try, but this is not going to work. For one, I think @pmf does not do macros and I do not think you can put the match in the tx variable like to you tried. Well maybe, but it would be a lot of playing around with this and I lack the time right now.

Yeah, I just got to the point where I noticed that macro expansion isn't supported for @pm. Thanks for your help with this. I'll see what other tricks I might try.

Maybe this, but I'm not sure if there's a way to escape the macro replacement for tx.933120_tx_0.

SecRule MATCHED_VARS "@rx \b%{tx.933120_tx_0}\s*=" \
    "capture,\
    setvar:'tx.php_injection_score=+%{tx.critical_anomaly_score}',\
    setvar:'tx.inbound_anomaly_score_pl1=+%{tx.critical_anomaly_score}'"

This seems pretty good if it's okay to require only whitespace before the =. The only other question I have is on the macro expansion.

SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* "@pmFromFile php-config-directives.data" \
    "id:933120,\
    phase:2,\
    block,\
    capture,\
    t:none,t:normalisePath,\
    msg:'PHP Injection Attack: Configuration Directive Found',\
    logdata:'Matched Data: %{TX.933120_TX_0} found within %{MATCHED_VAR_NAME}',\
    tag:'application-multi',\
    tag:'language-php',\
    tag:'platform-multi',\
    tag:'attack-injection-php',\
    tag:'paranoia-level/1',\
    tag:'OWASP_CRS',\
    tag:'capec/1000/152/242',\
    ver:'OWASP_CRS/4.1.0',\
    severity:'CRITICAL',\
    setvar:'tx.933120_tx_0=%{tx.0}',\
    chain"
    SecRule MATCHED_VARS "@pm =" \
        "chain"
        SecRule MATCHED_VARS "@rx \b%{tx.933120_tx_0}\s*=" \
            "capture,\
            setvar:'tx.php_injection_score=+%{tx.critical_anomaly_score}',\
            setvar:'tx.inbound_anomaly_score_pl1=+%{tx.critical_anomaly_score}'"

From some testing, macro extension for @rx doesn't seem to escape it. Here's an alternative. What do you think? I can add it to a PR if you want.

SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* "@pmFromFile php-config-directives.data" \
    "id:933120,\
    phase:2,\
    block,\
    capture,\
    t:none,t:normalisePath,\
    msg:'PHP Injection Attack: Configuration Directive Found',\
    logdata:'Matched Data: %{TX.933120_TX_0} found within %{MATCHED_VAR_NAME}: %{MATCHED_VAR}',\
    tag:'application-multi',\
    tag:'language-php',\
    tag:'platform-multi',\
    tag:'attack-injection-php',\
    tag:'paranoia-level/1',\
    tag:'OWASP_CRS',\
    tag:'capec/1000/152/242',\
    ver:'OWASP_CRS/4.1.0',\
    severity:'CRITICAL',\
    setvar:'tx.933120_tx_0=%{tx.0}',\
    chain"
    SecRule MATCHED_VARS "@rx \b([^\s]+)\s*=" \
        "capture,\
        setvar:'tx.933120_possible_directive=%{TX.1}',\
        chain"
        SecRule TX:933120_possible_directive "@pmFromFile php-config-directives.data" \
            "capture,\
            setvar:'tx.php_injection_score=+%{tx.critical_anomaly_score}',\
            setvar:'tx.inbound_anomaly_score_pl1=+%{tx.critical_anomaly_score}'"

@ssigwart that looks to work but it's overengineered imo, each chained rule added makes a rule harder to read and negatively effects performance. I'd remove the false positive words from 933120 and create another rule 933121 that uses the aforementioned regex to avoid the false positive.

I do not think the performance is an issue since the chained rules and the creation of TX variables etc. only ever happens after a hit in the first rule. So if this behaves correctly, I'm inclined to accept it.

I mean the introduction of rule 933121 would mean an additional rule for every request at PL2 plus the FP now at PL2, while the new recipe above works around the FPs, allows us to maintain the keyword list without a separate manual exclusion list and does not add another rule at PL2.

So I think this is actually better even if it's harder to read.

And @ssigwart is showing impressive rule writing talent. I like this.

Thanks, @dune73. Yes, I intentionally did the @rx in the chain thinking that it would only execute if it already thought it was a potential issue. I'll update the PR.

Hey 👋
I have stumbled over this issue because I had a lot of FPs in the logs. php-config-directives.datacontains smtp as a word. This character sequence is somewhat common to appear in Bearertokens/Cookies, which resulted in Users getting blocked. Could you change rules using this file to not trigger when the keyword is within 5 letters before the string and after the string?

So a;sljfgqegSMTPaksjföalj would not trigger the rule, but asljfgqe;gSMTPaksjföalj would trigger the rule? This way, there would not be as many FPs and you could still detect attempts to send SMTP as a string.

I think all problematic worlds in the list should be this ones (found using: ^[a-zA-Z]*$):

browscap
engine
extension
precision
smtp
xbithack

I have added the following rule as a work around:

SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* "@within browscap,engine,extension,precision,smtp,xbithack"
    "id:933119,\
    phase:2,\
    t:none,\
    t:normalizePath,\
    nolog,\
    ctl:ruleRemoveById=933120"

@niklasweimann There's already a PR open here that should resolve this issue, but I'm not sure of the status of it. #3638

I've added your proposal to the agenda for the 2nd May meeting of the team, @ssigwart. Thank you very much. Feel free to join on May 20.

#3694

Thanks, @dune73. The May 20th meeting is in the middle of my workday, but thanks for the invite.

I'm not sure if this is a case of 'works as designed', but I work a lot with SAML-related software and I see two clear cases for rule 933120 that to me are FPs:

1. URLs containing SAMLRequest=hVLLTiNBDPyVUd8n88**sMtP**...vBoUORLU%2BS%2F%2F%2Fd8h8%3 where the value is a base64 encoded string.

2. SAML also defines a HTTP param called RelayState, which contains an encoded url. One common SAML-product standardizes their hostname on engine, leading to a HTTP param like RelayState=https%3A%2F%2Fengine.example.org%2F

Is this expected to hit this rule, or have I just hit an edge case that is fixable?
One major drawback is that I now have to disable the rule completely for the specific HTTP params, or I have to edit the php-config-directives.data file on every update of this package.

This is potentially something we would want to cover, namely if it's a standard product. Please open a separate bug report and add as much details as possible, namely the name of said product.