Add urlDecodeUni() operation to ARG/ARGS_NAMES

[csjperon]

• Add the urlDecodeUni transform operation to rules which inspect the
  ARGS/ARGS_NAMES collections, which do not currently call urlDecodeUni
• This operation should be here. Some rules were written under the assumption
  that Apache/Nginx will automatically decode these collections prior to
  entering Modsecurity.
• This change shouldn't impact the rule quality, but will introduce additional decoding overhead

I am creating this PR to start the discussion

Thoughts?

[csanders-git]

it isn't a NOP but is an extra transformation which is additional overhead - but saying as others use these rules it might be advisable.

[dune73]

This is a welcome fix.

I do not see it applied everywhere, though. How about the following rules?

• 913120
• 920230
• 921170
• 931100
• 931120
• 931130
• 932100
• 932110
• 932150
• 933140
• 933150
• 933160
• 933170
• 933180
• 933151
• 933161

Am I missing something?

[csjperon]

@dune73 No, I was missing something :) I will update the PR shortly

[csjperon]

So after running the tests, this change will create a regression in 920230:

SecRule ARGS "\%((?!$|\W)|[0-9a-fA-F]{2}|u[0-9a-fA-F]{4})" \
  "phase:request,\
   rev:'2',\
   ver:'OWASP_CRS/3.0.0',\
   maturity:'6',\
   accuracy:'8',\
   t:none,\
   block,\
   msg:'Multiple URL Encoding Detected',\
   id:920230,\
   tag:'application-multi',\
   tag:'language-multi',\
   tag:'platform-multi',\
   tag:'attack-protocol',\
   tag:'OWASP_CRS/PROTOCOL_VIOLATION/EVASION',\
   tag:'paranoia-level/2',\
   severity:'WARNING',\
   setvar:'tx.msg=%{rule.msg}',\
   setvar:tx.anomaly_score=+%{tx.warning_anomaly_score},\
   setvar:tx.%{rule.id}-OWASP_CRS/PROTOCOL_VIOLATION/EVASION-%{matched_var_name}=%{matched_var}"

So perhaps we shouldn't enable it here? The rest of the tests were not impacted.

Thoughts?

[dune73]

Thank you for the update. Are you positive, that all necessary rules have the transformation now? My comment was written after a quick glance over the ruleset.

I see why there would be a regression with this rule. It's an encoding check, after all. Maybe the regex needs to be adopted.

Are you understanding the regex and can you try and fix it?

If that is not possible, we need a comment next to the rule why the transformation is not being applied.

[csjperon]

Yes, I think this rule requires utftounicode() actually, since the rule is looking for the same pattern that would be returned as a result of that transform function [u[0-9a-fA-F]{4}] (basically implying that raw unicode characters were present in the ARGS strings. I will look into it in more detail though. There are a few issues reported with that function too:

https://github.com/SpiderLabs/ModSecurity/issues/created_by/katef