fix(941120): new regex is eligible for Paranoia Level 1#4291
Conversation
|
📊 Quantitative test results for language: |
|
This needs to be discussed - adding it to the agenda of next monthly chat. |
|
@RedXanadu The PR has been updated with both the old and new regex. I don’t have the energy to find the commit - it’s somewhere between branches 3 and 4. : v3.3.7...v4.0.0 |
fe95dfc to
4e96d22
Compare
Removed XSS Filter - Category 2 rules and comments.
fzipi
left a comment
There was a problem hiding this comment.
I agree that this was mostly because of base64 encoded requests. No this should be less problematic and it is probably reasonable to move to PL1.
I would say let's merge and see what 4.23.0 gives us.
|
@fzipi My anecdotal experience with this rule is that the false positives are quite rare with this rule.
If your concerned about false positives then delay then we should delay this change until after the LTS release. |
|
@EsadCetiner the old CRS 3 regex was a false-positive nightmare in e-commerce contexts, where hashes, tokens, and RSA signatures are particularly common. I think this understandably traumatized quite a few integrators - rightly so. However, the regex has evolved and is now significantly simpler; we just need to be pedagogical with integrators who were impacted by that past experience. |
|
I think more or less the same as @touchweb-vincent, but just to be sure, can you add some more tests involving exactly what you mentioned? E.g. "hashes, tokens, and RSA signatures..". |
|
I haven’t had this kind of dataset for years anymore, but I added a Base64 payload that used to trigger the old regex - representative of what we had to deal with in an e-commerce context, for example with payment gateway callbacks. |
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
|
Well, ended up adding some more tests. Even without artificial XSS patterns, these tests verify that:
The tests somehow represent realistic payment gateway callbacks that could legitimately appear in production e-commerce systems. |
Hello,
The new regex for rule 941120 has resolved most of the issues previously associated with it - including the erratic Base64 triggers.
Since 941100 remains quite tricky in terms of false positives, I don’t really understand why a rule that now produces fewer false positives than a PL1 rule is still classified under PL2.
Therefore, I’d like to suggest moving it down to Paranoia Level 1 - it’s a sensitive rule that should ideally be active even at the lowest paranoia level.
This regex has been updated on 4.0.0. The previous regex on https://github.com/coreruleset/coreruleset/releases/tag/v3.3.7 was :
This old regex was wrongly tagged as PL1 IMO - should have been in PL2 because of its false positives. Then you decided to move it to PL2.
The new one since 4.0.0 is : https://github.com/coreruleset/coreruleset/blob/main/rules/REQUEST-941-APPLICATION-ATTACK-XSS.conf#L837
In my opinion, there’s no reason for this new regex to be in PL2 - it only inherits the false-positive history of the previous one, which is no longer relevant.
What do you think?
Vincent