8000 Rule 933150 Has False Positive for URLs · Issue #3641 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Rule 933150 Has False Positive for URLs #3641

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
Open
1 task done
ssigwart opened this issue Apr 2, 2024 · 10 comments · May be fixed by #3840
Open
1 task done

Rule 933150 Has False Positive for URLs #3641

ssigwart opened this issue Apr 2, 2024 · 10 comments · May be fixed by #3840
Labels
➕ False Positive

Comments

@ssigwart
Copy link
Contributor
ssigwart commented Apr 2, 2024

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
Copy link
Contributor Author
ssigwart commented Apr 2, 2024 •
edited
Loading

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
Copy link
Member
azurit commented Apr 3, 2024

@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
Copy link
Member
dune73 commented Apr 3, 2024

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
Copy link
Contributor Author
ssigwart commented Apr 3, 2024

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:

@RedXanadu
Copy link
Member

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
Copy link
Contributor Author

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?

@EsadCetiner
Copy link
Member

Couldn't printf just be moved to 933160 and this false positive would be easily resolved? I don't know much about PHP code, but it doesn't look like it should open up any possible bypasses. Although false positives on this rule is fairly rare.

@RedXanadu
Copy link
Member

This one is getting a bit old, now…

Re-tested and confirmed that this false positive still exists.

I would support what you propose, @EsadCetiner. I think, if we had a negative test for "sprintforwards" and moved printf (I think that's the only offending pattern, here?) to the rule that looks also for \b then we could merge that.

@EsadCetiner EsadCetiner linked a pull request Oct 2, 2024 that will close this issue
Open
@EsadCetiner
Copy link
Member

@RedXanadu @ssigwart I've opened a PR to resolve this issue #3840

@ssigwart
Copy link
Contributor Author
ssigwart commented Oct 2, 2024

That's great. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
➕ False Positive
Projects
None yet
Milestone
No milestone
5 participants
@dune73 @azurit @ssigwart @RedXanadu @EsadCetiner
2AB6
0