Unsupported "accuracy" Action in SecRule Configuration

tigerwill90 commented 4 months ago

Description

Hi, I'm new to ModSecurity and Coraza, so please excuse me if this report is not entirely accurate.

I encountered an issue while using Coraza and testing some plugins. It appears that the accuracy action, despite being documented here, is not recognized as valid.

Steps to reproduce

Configure the following rule:

SecRule REQUEST_FILENAME "@rx \.(conf|htaccess|htpass|sql|orig|bak|db|ini|md|log|git|github|swp|DS_STORE)($|/)?" \
        "id:108,\
        phase:1,\
        t:lowercase,t:normalizePath,t:trim,\
        severity:'NOTICE',\
        accuracy:'9',\
        deny,\
        capture,\
        logdata:'Request Filename %{REQUEST_FILENAME}',\
        msg:'Wordpress hardening: denied access to sensitive files'"

The following error is returned:

invalid WAF config from file: failed to parse string: failed to compile the directive \"secrule\": invalid action \"accuracy\

When the accuracy action is removed, the rule compiles successfully.

Interestingly, when trying with the example rule below (from the documentation), it does not return the error:

SecRule REQUEST_FILENAME|ARGS_NAMES|ARGS|XML:/* "\bgetparentfolder\b" \
    "id:'958016',phase:2,ver:'CRS/2.2.4,accuracy:'9',maturity:'9',capture,\
    t:none,t:htmlEntityDecode,t:compressWhiteSpace,t:lowercase,\
    ctl:auditLogParts=+E,block,msg:'Cross-site Scripting (XSS) Attack',\
    tag:'WEB_ATTACK/XSS',tag:'WASCTC/WASC-8',tag:'WASCTC/WASC-22',tag:'OWASP_TOP_10/A2',\
    tag:'OWASP_AppSensor/IE1',tag:'PCI/6.5.1',logdata:'%{TX.0}',\
    severity:'2',setvar:'tx.msg=%{rule.msg}',\
    setvar:tx.xss_score=+%{tx.critical_anomaly_score},\
    setvar:tx.anomaly_score=+%{tx.critical_anomaly_score},\
    setvar:tx.%{rule.id}-WEB_ATTACK/XSS-%{matched_var_name}=%{tx.0}"

However, it looks like the ver:'CRS/2.2.4 is missing a ' at the end, so my guess is that the action is not intepreted.

Additionally, I noticed that in the ModSecurity Reference Manual , the ver also end without the ', so I'm not sure if it's something expected.

Expected result

The accuracy action should be supported as documented.

Actual result

The following error is encountered:

invalid WAF config from file: failed to parse string: failed to compile the directive \"secrule\": invalid action \"accuracy\

jptosso commented 4 months ago

We'll take a look, but I'm mostly impressed you are still running crs 2 👀

tigerwill90 commented 4 months ago

This is a copy/paste of the rule from Coraza Documentation, not my CRS setup.

By the way, looking at this section of the Coraza source code, it seems some actions are not implemented, so this may be intended. Feel free to close the issue if that's the case.

M4tteoP commented 4 months ago

Hey @tigerwill90, thanks for raising this issue and for the detailed description, it is definitely on point!

It appears that the accuracy action, despite being documented here, is not recognized as valid.

I confirm that. We have the notion of the accuracy of a rule in the codebase (There is an Accuracy_ field in the RuleMetadata, we are printing it in the logs, etc.) but we do not actually have the action registered, therefore it can not be parsed and that field in the rules will always be zero.

Accuracy and maturity as far as I know, are fields with not much value (They look like quite arbitrary numbers), and they were removed from the CRS rules quite a while ago. This is likely the reason why it has never been spotted.

That being said, we have to take some action about it. We can entirely wipe off the notion of accuracy of a rule from both the codebase and documentation, or implement it. I would go for the latter, it brings compatibility with Seclang and rules that might still have it, and, users might independently find a meaningful usage of that field for their own use-case.

However, it looks like the ver:'CRS/2.2.4 is missing a ' at the end, so my guess is that the action is not intepreted.

Additionally, I noticed that in the ModSecurity Reference Manual , the ver also end without the ', so I'm not sure if it's something expected.

I really think that the typo of missing ' has just been propagated across different engines' documentation 😅.

it seems some actions are not implemented

Did you spot any other action you were expecting to find that seems not implemented? Related to it, we have https://github.com/corazawaf/coraza.io/issues/228 that would be great to be addressed, syncing also actions documentation directly from the code. We would have spotted that

tigerwill90 commented 4 months ago

Hey @M4tteoP, thank you for your response. I'm glad that the issue is on point:

Did you spot any other action you were expecting to find that seems not implemented? Related to it, we have https://github.com/corazawaf/coraza.io/issues/228 that would be great to be addressed, syncing also actions documentation directly from the code. We would have spotted that

I double-checked all actions, and here are a few potential inconsistencies in the documentation that I can report:

Not Implemented and Not Supported in ModSecurity v3.x

append Documented (not implemented): https://coraza.io/docs/seclang/actions/#append Not supported in v3.x: https://github.com/owasp-modsecurity/ModSecurity/wiki/Reference-Manual-%28v3.x%29#user-content-append
pause Documented (not implemented): https://coraza.io/docs/seclang/actions/#pause Not supported in v3.x: https://github.com/owasp-modsecurity/ModSecurity/wiki/Reference-Manual-%28v3.x%29#pause
prepend Documented (not implemented): https://coraza.io/docs/seclang/actions/#prepend Not supported in v3.x: https://github.com/owasp-modsecurity/ModSecurity/wiki/Reference-Manual-%28v3.x%29#prepend
proxy Documented (not implemented): https://coraza.io/docs/seclang/actions/#proxy Not supported in v3.x: https://github.com/owasp-modsecurity/ModSecurity/wiki/Reference-Manual-%28v3.x%29#proxy

Other actions like sanitiseArg, sanitiseMatched, etc., are not supported in ModSecurity v3.x and are documented as "Supported on Coraza: TBI" (but not implemented). However, I'm not sure what "TBI" stands for 😅.

Besides these few points, the only action in the ModSecurity v3.x Reference Manual that is not currently implemented seems to be xmlns (here), but this action is also not present in the Coraza documentation.

jptosso commented 4 months ago

TBI is To Be Implemented

Sanitize sets should become a priority, but regarding prepend and append are a bit more complicated. Although we have full control of the request and response body, there are many implications that affects the integrations that could lead to breaking changes. For example, how do we update the content length? Also we decided at some point that we will only read the data and not change it. This is something that can be re-evaluated though

corazawaf / coraza