Rule Cleanup

[dune73]

There are now several issues and even PRs that deal with a cleanup of rules. This issue is meant to keep all these tasks together by discussing them centrally and by governing the label "Rule Cleanup Project".

In focus are:

• Ordering of actions
• Indentation
• Clean whitespace
• Remove maturity action
• Remove accuracy action

@fgsch has volunteered to carry this project.

[csanders-git]

So what are we saying are the best practices? should we start a wiki page for that?

[dune73]

Creating a wiki page would probably make sense. Here on the fly the order that I used in the book:

id
phase
disruptive-action
status
capture
t:xxx
log
nolog
auditlog
noauditlog
msg
logdata
tag
sanitiseArg
sanitiseRequestHeader
sanitiseMatched
sanitiseMatchedBytes
ctl
setenv
setvar
chain
skip
skipAfter

What I think is essential is starting with id and phase and disruptive action. Other items are mostly optional and do not matter much for CRS. I have logfile definitions early as are the transformations. Msg and logdata are coming only afterwards. Chain is listed near the end to make a visible link to the chained rule that has indentation.

Of course the list above is only proposal. I think it has some merits, but whatever the reasonable order is that we want to apply, we need to make sure it is applied consistenly and that we check it automatically. @fgsch also speaks of lint.

[dune73]

I have not heard any feedback on the proposal above. I have transferred it into a wiki page: https://github.com/SpiderLabs/owasp-modsecurity-crs/wiki/Order-of-ModSecurity-Actions-in-CRS-rules

If there is no comment in the next few days, I assume the proposal has support and @fgsch is good to go.

[fgsch]

It's been 20 days since @dune73 comment.
I assume it's OK and I will proceed as per comment above.

[fzipi]

In addition to your proposal, I would like to propose a standard for indenting rules.

I noticed these inconsistencies trying to commit the ruleset to my own git. It began complaining in lots of places about tabs and spaces mix, and others.

Examples of actual inconsistency:
REQUEST-913-SCANNER-DETECTION.conf (33-34):

SecRule REQUEST_HEADERS:User-Agent "@pmFromFile scanners-user-agents.data" \
 "msg:'Found User-Agent associated with security scanner',\

In this case there are two spaces at the beggining of the second line. But in ./REQUEST-921-PROTOCOL-ATTACK.conf (line 36):

SecRule REQUEST_HEADERS:'/(Content-Length|Transfer-Encoding)/' "," \
        "msg:'HTTP Request Smuggling Attack.',\
        phase:request,\
        id:921100,\
        rev:'2',\

It is indented using tabs.

Anothe in REQUEST-920-PROTOCOL-ENFORCEMENT.conf (337-359):

SecRule REQUEST_HEADERS:Range|REQUEST_HEADERS:Request-Range "(\d+)\-(\d+)\," \
  "capture,\
   phase:request,\
   rev:'2',\
   ver:'OWASP_CRS/3.0.0',\
   maturity:'6',\
   accuracy:'8',\
   t:none,\
   block,\
   msg:'Range: Invalid Last Byte Value.',\
   logdata:'%{matched_var}',\
   severity:'WARNING',\
   id:920190,\
   tag:'application-multi',\
   tag:'language-multi',\
   tag:'platform-multi',\
   tag:'attack-protocol',\
   tag:'OWASP_CRS/PROTOCOL_VIOLATION/INVALID_HREQ',\
   chain"
        SecRule TX:2 "!@ge %{tx.1}" \
                "setvar:'tx.msg=%{rule.msg}',\
                setvar:tx.anomaly_score=+%{tx.warning_anomaly_score},\
                setvar:tx.%{rule.id}-OWASP_CRS/PROTOCOL_VIOLATION/INVALID_HREQ-%{matched_var_name}=%{matched_var}"

This case has a chained SecRule, and indentation begins at a different level, and below setvars are indented using tabs.

This are only a small part of the inconsistencies.

I don't wan't to start a 'tabs vs. spaces', I'm only concerned about consistency. I prefer tabs for this case, but ymmv.

So, in summary:

1. Use tabs for indentation. This could be aided by uploading an '.editorconfig' with simple rules.
2. Whenever there is a continuation at the end (''), the next line will be indented using tab.
3. If there is a rule with a chain, the next chained rule begins on the next line with the same indenting, and if there is another continuation, indent again using tab.
4. Align with commas at the begin of the rule, don't add a space. Just tabs.

Example:

SecRule &REQUEST_HEADERS:Host "@eq 0" \
        "msg:'Request Missing a Host Header',\
        severity:'WARNING',\
        ...\
        chain"
        SecRule REQUEST_METHOD "!^OPTIONS$" \
                "chain"
                SecRule REQUEST_HEADERS:User-Agent "SomeStuff" \
                        "t:none,\
                        setvar:'tx.msg=%{rule.msg}',\
                        setvar:tx.anomaly_score=+%{tx.notice_anomaly_score},\
                        setvar:tx.%{rule.id}-OWASP_CRS/PROTOCOL_VIOLATION/MISSING_HEADER-%{matched_var_name}=%{matched_var}"

Let me know what you think. It isn't written in stone :simple_smile:

[fgsch]

This was already mentioned in the first comment.
Personally I'd prefer spaces to keep the width limited to (or close to) 80 cols but I don't mind either way. Changing this one way or another should be rather trivial.

[fzipi]

I was trying just to put it down some criteria which could be used afterwards on a CONTRIBUTING file, or something like that.

Spaces is fine for me also, btw! 4?

[fzipi]

So, I created #859 (dev branch), let me know what you think or if there is a need to change something.

[csanders-git]

we can add tests to the travis-ci tests to enforce these standards as well once we solidify. I think this feedback is great. I like spaces personally but don't feel strongly one way or the other.

[fgsch]

I like 4 spaces but I'd like to see a 4 space indentation for each SecRule in the chain. i.e.

SecRule .. ..
   "...."
   SecRule .. ..
       "..."
       SecRule .. ..
       ..

Am I the only one?

[fzipi]

I concur. I updated my patch.

Another thing I've noticed is that when there is a only one action there is no "" surrounding it. For example whenever there is a chain alone. e.g:

SecRule .... \
    chain
    SecRule
        ...

It is better visually for me if this is used as "chain" (coloring helps) also, e.g:

SecRule ..... \
    "chain"

So I fixed that also.