Allow regular expressions in ctl:ruleRemoveTargetByX variable names #911

[vvidic]

SecRule REQUEST_URI "@beginswith /index.php"
"id:1001,phase:1,pass,nolog,
ctl:ruleRemoveTargetById=942100;ARGS:/^password[\d+]$/"

[zimmerle]

Hi @vvidic,

Thank you for the patch. As of the release of version 3 we are only merging new features if they are also available for v3.

[vvidic]

So you mean I should send a patch for v3 branch than?

[zimmerle]

Hi @vvidic,

It means that it won't be released until we have the same functionality in v3.

[emphazer]

@vvidic thx a lot for this wonderful patch.
this makes modsec v2 much better and easier to handle on multi home servers

[fzipi]

@vvidic Do you have the functionality available for v3?

[vvidic]

On Wed, Jul 17, 2019 at 10:38:29AM -0700, Felipe Zipitría wrote: @vvidic Do you have the functionality available for v3?

Nope, don't think I found the place where it should go. Let me know if you have any suggestions.

…

-- Valentin

[fzipi]

Looking for it....

[azurit]

Should be merged even if there's no v3 version, some rules cannot be written without this feature.

[vvidic]

On Tue, Aug 27, 2019 at 07:40:03AM -0700, azurit wrote: Should be merged even if there's no v3 version, some rules cannot be written without this feature.

Right, but I think this was a request from the upstream authors.

…

-- Valentin

[azurit]

I was talking to them off source :) btw, thnx for the patch.

[vvidic]

On Tue, Aug 27, 2019 at 10:44:47AM -0700, azurit wrote: I was talking to them off source :) btw, thnx for the patch.

Great :) In the meanwhile I checked the v3 code and found 2 places in src/rule.cc where RemoveTargetById/Tag is being used so this is probably where the changes should go...

…

-- Valentin

[azurit]

Example of exclusive rule which cannot be written without this feature (Typo3, probably a CSRF security token which is part of input name):

"Warning. Pattern match \"(?:;|\\\\{|\\\\||\\\\|\\\\||&|&&|\\\\n|\\\\r|\\\\$\\\\(|\\\\$\\\\(\\\\(|`|\\\\${|<\\\\(|>\\\\(|\\\\(\\\\s*\\\\))\\\\s*(?:{|\\\\s*\\\\(\\\\s*|\\\\w+=(?:[^\\\\s]*|\\\\$.*|\\\\$.*|<.*|>.*|\\\\'.*\\\\'|\\\".*\\\")\\\\s+|!\\\\s*|\\\\$)*\\\\s*(?:'|\\\")*(?:[\\\\?\\\\*\\\\[\\\\]\\\\(\\\\)\\\\-\\\\|+\\\\w'\\\"\\\\./\\\\\\\\]+/)?[\\\\\\\\'\\\"]*(?:l[\\\\\\\\'\\\"]* ...\" at ARGS:data[tt_content][NEW5d67a8343a775100352544][bodytext]. [file \"/usr/share/modsecurity-crs/rules/REQUEST-932-APPLICATION-ATTACK-RCE.conf\"] [line \"81\"] [id \"932100\"] [rev \"4\"] [msg \"Remote Command Execution: Unix Command Injection\"] [data \"Matched Data: ;\\x0d\\x0a(function found within ARGS:data[tt_content][NEW5d67a8343a775100352544][bodytext]: <style>\\x0d\\x0a #_form_52_ { font-size:14px; line-height:1.6; font-family:arial, helvetica, sans-serif; margin:0; }\\x0d\\x0a #_form_52_ * { outline:0; }\\x0d\\x0a ._form_hide { display:none; visibility:hidden; }\\x0d\\x0a ._form_show { display:block; visibility:visible; }\\x0d\\x0a #_form_52_._form-top { top:0; }\\x0d\\x0a #_form_52_._form-bottom { bottom:0; }\\x0d\\x0a #_form_52_._form-left { left"

[vvidic]

Another obstacle in the v3 implementation seems to be the parser, as it does not except the regex in the variable name: SecRule REQUEST_FILENAME "@ENDSWITH /wp-login.php" "id:9002100,phase:2,t:none,nolog,pass,ctl:ruleRemoveTargetById=123;ARGS:/^pwd$/" Rules error. File: action-ctl_rule_remove_target_by_id.json. Line: 1. Column: 132. Expecting an action, got: ^pwd$/" Not sure how to work around this?

…

-- Valentin

[azurit]

@vvidic Today i came accross a bug in this feature: It's not possible to match a pipe symbol ( | ), even when escaped. Apache will print this error:
Error parsing actions: Unknown action: _]+\\]$/

Example rule (Prestashop web translation feature):

SecRule REQUEST_FILENAME "@rx /admin[0-9a-zA-Z]+/index\.php$" \
    "id:9990317,\
    phase:2,\
    pass,\
    t:none,\
    nolog,\
    chain"
    SecRule ARGS_GET:controller "@streq AdminTranslations" \
        "t:none,\
        chain"
        SecRule &ARGS_GET:controller "@eq 1" \
            "t:none,\
            ctl:ruleRemoveTargetById=932100;ARGS:/^core_mail\[txt\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=932105;ARGS:/^core_mail\[txt\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=932105;ARGS:/^core_mail\[html\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=932100;ARGS:/^core_mail\[html\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=921130;ARGS:/^core_mail\[html\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=941180;ARGS:/^core_mail\[html\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=932110;ARGS:/^core_mail\[html\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=941100;ARGS:/^core_mail\[html\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=941140;ARGS:/^core_mail\[html\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=941260;ARGS:/^core_mail\[html\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=941250;ARGS:/^core_mail\[html\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=941190;ARGS:/^core_mail\[html\]\[[a-z_]+\]$/,\
            ctl:ruleRemoveTargetById=932100;ARGS:/^module_mail\[html\]\[[a-z0-9\-\|_]+\]$/,\
            ctl:ruleRemoveTargetById=921130;ARGS:/^module_mail\[html\]\[[a-z0-9\-\|_]+\]$/,\
            ctl:ruleRemoveTargetById=941180;ARGS:/^module_mail\[html\]\[[a-z0-9\-\|_]+\]$/,\
            ctl:ruleRemoveTargetById=932110;ARGS:/^module_mail\[html\]\[[a-z0-9\-\|_]+\]$/,\
            ctl:ruleRemoveTargetById=941100;ARGS:/^module_mail\[html\]\[[a-z0-9\-\|_]+\]$/,\
            ctl:ruleRemoveTargetById=941140;ARGS:/^module_mail\[html\]\[[a-z0-9\-\|_]+\]$/,\
            ctl:ruleRemoveTargetById=941190;ARGS:/^module_mail\[html\]\[[a-z0-9\-\|_]+\]$/,\
            ctl:ruleRemoveTargetById=941250;ARGS:/^module_mail\[html\]\[[a-z0-9\-\|_]+\]$/,\
            ctl:ruleRemoveTargetById=941260;ARGS:/^module_mail\[html\]\[[a-z0-9\-\|_]+\]$/,\
            ctl:ruleRemoveTargetById=930120;ARGS:/^[0-9a-z]+$/,\
            ctl:ruleRemoveTargetById=933180;ARGS:/^[0-9a-z]+$/"

The interesting is that escaped pipe symbol can be used in parts where regex is officially supported (e.g. in REQUEST_FILENAME matching in the beginning of the rule).

[azurit]

Won't compile with --with-pcre2, modsec 2.9.7:

re.c:124:41: error: 'PCRE_DOTALL' undeclared (first use in this function); did you mean 'PCRE2_DOTALL'?
                                         PCRE_DOTALL | PCRE_CASELESS | PCRE_DOLLAR_ENDONLY, (const char **)&errptr, &erroffset);
                                         ^~~~~~~~~~~
                                         PCRE2_DOTALL
re.c:124:41: note: each undeclared identifier is reported only once for each function it appears in
re.c:124:55: error: 'PCRE_CASELESS' undeclared (first use in this function); did you mean 'PCRE2_CASELESS'?
                                         PCRE_DOTALL | PCRE_CASELESS | PCRE_DOLLAR_ENDONLY, (const char **)&errptr, &erroffset);
                                                       ^~~~~~~~~~~~~
                                                       PCRE2_CASELESS
re.c:124:71: error: 'PCRE_DOLLAR_ENDONLY' undeclared (first use in this function); did you mean 'PCRE2_DOLLAR_ENDONLY'?
                                         PCRE_DOTALL | PCRE_CASELESS | PCRE_DOLLAR_ENDONLY, (const char **)&errptr, &erroffset);
                                                                       ^~~~~~~~~~~~~~~~~~~
                                                                       PCRE2_DOLLAR_ENDONLY
re.c:131:96: error: 'PCRE_ERROR_NOMATCH' undeclared (first use in this function); did you mean 'PCRE2_ERROR_NOMATCH'?
                                 if (!(msc_regexec(regex, myvalue, strlen(myvalue), &errptr) == PCRE_ERROR_NOMATCH)) {
                                                                                                ^~~~~~~~~~~~~~~~~~
                                                                                                PCRE2_ERROR_NOMATCH

[airween]

Won't compile with --with-pcre2, modsec 2.9.7:

Thanks, I'll take a look at it soon.

[airween]

I'm closing this PR, I've already added it with PCRE2 macros in #3121. Thanks @vvidic.

[marcstern]

if((strlen(myvalue) == strlen(value)) && strncasecmp(myvalue,value,strlen(myvalue)) == 0)
Why not
if(strcasecmp(myvalue,value,strlen(myvalue)) == 0)

[marcstern]

else if (value == NULL && myvalue == NULL)    {
   if (msr->txcfg->debuglog_level >= 9) {
      msr_log(msr, 9, "fetch_target_exception: Target %s will not be processed.", target);
   }
   match = 1;
} else if (value == NULL && myvalue != NULL)   {
   if (msr->txcfg->debuglog_level >= 9) {
      msr_log(msr, 9, "fetch_target_exception: Target %s will not be processed.", target);
   }
   match = 1;
}

Simplify:

else {
   if (msr->txcfg->debuglog_level >= 9) {
      msr_log(msr, 9, "fetch_target_exception: Target %s will not be processed.", target);
   }
   match = 1;
}