Rule 933150 Has False Positive for URLs

[ssigwart]

Description

The PHP printf rule is triggering issues on URLs like "SprintForTheCause".

How to reproduce the misbehavior (-> curl call)

curl -H "x-format-output: txt-matched-rules" https://sandbox.coreruleset.org/SprintForTheCause

933150 PL1 PHP Injection Attack: High-Risk PHP Function Name Found
949110 PL1 Inbound Anomaly Score Exceeded (Total Score: 5)
980170 PL1 Anomaly Scores: (Inbound Scores: blocking=5, detection=5, per_pl=5-0-0-0, threshold=5) - (Outbound Scores: blocking=0, detection=0, per_pl=0-0-0-0, threshold=4) - (SQLI=0, XSS=0, RFI=0, LFI=0, RCE=0, PHPI=5, HTTP=0, SESS=0, COMBINED_SCORE=5)

Confirmation

• I have removed any personal data (email addresses, IP addresses,
  passwords, domain names) from any logs posted.

[ssigwart]

I have a possible fix of adding a chain. Please let me know if you want me to open a PR for this.

chain"
    SecRule MATCHED_VARS "@rx \b([^\s]+)\s*\(" \
        "capture,\
        setvar:'tx.933150_possible_func=%{TX.1}',\
        chain"
        SecRule TX:933150_possible_func "@pmFromFile php-function-names-933150.data" \
            setvar:'tx.php_injection_score=+%{tx.critical_anomaly_score}',\
            setvar:'tx.inbound_anomaly_score_pl1=+%{tx.critical_anomaly_score}'"

[azurit]

@ssigwart Thanks for reporting this. Your solution looks too complex and prone to errors. For example, you are not catching $ddd=exec( because you are searching for keyword ddd=exec.

[dune73]

Yes, it's probably too specific. But you got the hang of this really quickly @ssigwart. Congratulations on your progress with the rule language.

[ssigwart]

Your solution looks too complex and prone to errors. For example, you are not catching $ddd=exec( because you are searching for keyword ddd=exec.

I'm a bit confused by this comment. php-function-names-933150.data doesn't include exec, so it doesn't trigger with the current rule either.

However, I can test $ddd=bzopen( and the my rule suggestion seems to catch that.

I tested locally with these:

• http://localhost:8888/?test=bzopen(
• http://localhost:8888/?test=%24ddd%3Dbzopen(

[RedXanadu]

I think what's needed here is a rule exclusion to tune away that false positive. We have some great documentation on how to do that. Please shout if we can help with that and we will.

We've done a lot of work recently on resolving false positives with natural language, but sprintf / printf doesn't fall in that category, so I'm not sure this is something we would address in the rule set itself.

E.g. "Print forty pages" would be a big problem if that caused false positives, but something like "sprintforwards" is not natural language and should probably be addressed with a rule exclusion if it's causing a problem in a specific deployment.

[ssigwart]

Thanks, @RedXanadu. I have already tweaked rules to prevent the FP in our code, so I'm fine with you closing this if you want. I do find it odd that it's picking up substrings though. I can definitely see how it would be really hard to prevent FPs on some things and I'm not an expert on the types of attacks these try to prevent, but wouldn't matching with \b around the function names be less prone to FPs and still be safe? So if you had "sprintforwards", it would not detect it, but if you had "sprintf orwards", it would?

Ha. I fall under the https://coreruleset.org/docs/concepts/false_positives_tuning/#directly-modifying-crs-rules section with the big red warning that it's a bad idea. In our case, I think it's actually the best way for us to handle it. For rules we want to exclude, we don't want to totally ignore it, so I typically add a chain rule to ignore certain patterns that seem like likely FPs. I know why it's documentation as a bad practice though. It's a pain doing the diff of the rule set each time there's an update to see which customizations I need to apply. There's not a great alternative for what I'm going though, right?