Added V3.1/dev contributing

[fzipi]

@dune73 commented:

This is a good start, but we'll need to talk about this some time as per your suggestion.

I'd like to merge this to v3.1/dev first. Then we can talk about backporting, but personally I do not see much sense in this.

Adding a remark with regards to doing PRs against the v3.1/dev file would also be worthwhile.

Your indentation example does not really adhere to the proposed style. At least not on my screen, please check.

We need a description of the tagging system we use. Given we think about overhauling this, a FIXME might do for the time being.

This is what springs to me right now. Not necessarily exhaustive list, though. Thank you for your initiative; really makes sense.

and @Spartani:

a sample rule template ready to copy paste would be nice :)
there are no naming conventions for vars and markers and rule id numbering

First of all, I'm doing this PR against v3.1/dev. I tried to add all your comments, and proposed some standards for vars and markes (which I'll be adding to the rule cleanup issue later).

wrt vars and markers, below are all the definitions summarized:

SecMarker BEGIN_REQUEST_BLOCKING_EVAL
SecMarker END_CORRELATION
SecMarker END_DOS_PROTECTION_CHECKS
SecMarker END-DRUPAL-RULE-EXCLUSIONS
SecMarker END_HOST_CHECK
SecMarker END_RBL_CHECK
SecMarker END_RBL_LOOKUP
SecMarker "END-REQUEST-910-IP-REPUTATION"
SecMarker "END-REQUEST-911-METHOD-ENFORCEMENT"
SecMarker "END-REQUEST-912-DOS-PROTECTION"
SecMarker "END-REQUEST-913-SCANNER-DETECTION"
SecMarker "END-REQUEST-920-PROTOCOL-ENFORCEMENT"
SecMarker "END-REQUEST-921-PROTOCOL-ATTACK"
SecMarker "END-REQUEST-930-APPLICATION-ATTACK-LFI"
SecMarker "END-REQUEST-931-APPLICATION-ATTACK-RFI"
SecMarker "END-REQUEST-932-APPLICATION-ATTACK-RCE"
SecMarker "END-REQUEST-933-APPLICATION-ATTACK-PHP"
SecMarker "END-REQUEST-941-APPLICATION-ATTACK-XSS"
SecMarker "END-REQUEST-942-APPLICATION-ATTACK-SQLI"
SecMarker "END-REQUEST-943-APPLICATION-ATTACK-SESSION-FIXATION"
SecMarker "END-REQUEST-949-BLOCKING-EVALUATION"
SecMarker "END-RESPONSE-950-DATA-LEAKAGES"
SecMarker "END-RESPONSE-951-DATA-LEAKAGES-SQL"
SecMarker "END-RESPONSE-952-DATA-LEAKAGES-JAVA"
SecMarker "END-RESPONSE-953-DATA-LEAKAGES-PHP"
SecMarker "END-RESPONSE-954-DATA-LEAKAGES-IIS"
SecMarker "END-RESPONSE-959-BLOCKING-EVALUATION"
SecMarker "END-RESPONSE-980-CORRELATION"
SecMarker "END-SAMPLING"
SecMarker END-WORDPRESS
SecMarker END-WORDPRESS-ADMIN

So the text for naming markers is somewhat similar to what we have now. I'm assuming there is no impact on renaming markers, also.

Below are the vars and its usage:

1 global.alerted_970018_iisDefLoc
2 ip.dos_block
3 ip.dos_block_counter
1 ip.dos_block_flag
2 ip.dos_burst_counter
3 ip.dos_counter
5 ip.previous_rbl_check
11 ip.reput_block_flag
11 ip.reput_block_reason
1 tx.allowed_http_versions
1 tx.allowed_methods
1 tx.allowed_request_content_type
196 tx.anomaly_score
1 tx.critical_anomaly_score
1 tx.do_reput_block
1 tx.dos_block_counter
1 tx.error_anomaly_score
2 tx.extension
1 tx.header_name_%{tx.0}
2 tx.httpbl_msg
11 tx.http_violation_score
3 tx.inbound_anomaly_score
1 tx.inbound_anomaly_score_threshold
2 tx.inbound_tx_msg
5 tx.lfi_score
195 tx.msg
1 tx.notice_anomaly_score
28 tx.outbound_anomaly_score
1 tx.outbound_anomaly_score_threshold
1 TX.paramcounter_%{MATCHED_VAR_NAME}
1 tx.paranoia_level
14 tx.php_injection_score
13 tx.rce_score
1 tx.real_ip
1 tx.reput_block_duration
1 tx.restricted_extensions
1 tx.restricted_headers
5 tx.rfi_score
7 tx.%{rule.id}-AUTOMATION/MALICIOUS-%{matched_var_name}
2 tx.%{rule.id}-AVAILABILITY/APP_NOT_AVAIL-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/AUTOMATION/CRAWLER-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/AUTOMATION/SCRIPTING-%{matched_var_name}
3 tx.%{rule.id}-OWASP_CRS/AUTOMATION/SECURITY_SCANNER-%{matched_var_name}
19 tx.%{rule.id}-OWASP_CRS/LEAKAGE/ERRORS-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/LEAKAGE/INFO-%{matched_var_name}
3 tx.%{rule.id}-OWASP_CRS/LEAKAGE/SOURCE_CODE-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/POLICY/CONTENT_TYPE_NOT_ALLOWED-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/POLICY/EXT_RESTRICTED-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/POLICY/HEADERS_RESTRICTED-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/POLICY/IP_HOST-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/POLICY/METHOD_NOT_ALLOWED-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/POLICY/PROTOCOL_NOT_ALLOWED-%{matched_var_name}
6 tx.%{rule.id}-OWASP_CRS/POLICY/SIZE_LIMIT-%{matched_var_name}
10 tx.%{rule.id}-OWASP_CRS/PROTOCOL_VIOLATION/EVASION-%{matched_var_name}
8 tx.%{rule.id}-OWASP_CRS/PROTOCOL_VIOLATION/INVALID_HREQ-%{matched_var_name}
5 tx.%{rule.id}-OWASP_CRS/PROTOCOL_VIOLATION/INVALID_REQ-%{matched_var_name}
8 tx.%{rule.id}-OWASP_CRS/PROTOCOL_VIOLATION/MISSING_HEADER-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/ABNORMAL-ESCAPE-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/COMMAND_INJECTION-%{matched_var_name}
2 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/DIR_TRAVERSAL-%{matched_var_name}
2 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/FILE_INJECTION-%{matched_var_name}
4 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/HEADER_INJECTION-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/HTTP_PARAMETER_POLLUTION-%{matched_var_name}
13 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/PHP_INJECTION-%{matched_var_name}
12 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/RCE-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/REQUEST_SMUGGLING-%{matched_var_name}
1 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/REQUEST-SMUGGLING-%{matched_var_name}
2 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/RESPONSE_SPLITTING-%{matched_var_name}
5 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/RESTRICTED_SQLI_CHARS-%{matched_var_name}
4 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/RFI-%{matched_var_name}
3 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/SESSION_FIXATION-%{matched_var_name}
22 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/SQLI-%{matched_var_name}
11 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/SQL_INJECTION-%{matched_var_name}
27 tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/XSS-%{matched_var_name}
1 tx.sampling_percentage
4 TX.sampling_rnd100
4 tx.session_fixation_score
2 tx.sql_error_match
57 tx.sql_injection_score
1 tx.static_extensions
1 tx.ua_hash
1 tx.warning_anomaly_score
28 tx.xss_score

I don't know if there is a clear naming there, maybe we can say:

• K&R (lower_case_naming)
• when there is string interpolation using rule id, use %{rule.id}-TAGNAME-%{matched_var_name}

[csanders-git]

So @jnazarioFastly brought it up, but i think common variable naming is as follows. When used as part of a collection in the variable section it is capitalized (i.e SecRule TX:hello). However when used as an action it is lowercased. setvar:tx.hello=test. Thoughts?

[dune73]

Dev Branch

Please replace

* Please base your changes on branch ```v3.1/dev```

with

* Please base your changes on the current development branch.
  As of this writing, this is ```v3.1/dev```.

It is very likely we overlook this when we switch the dev branch and the additional clarification will avoid mistakes.

[dune73]

Collections

Unfortunately, Apache and ModSecurity leave the choice in our hands, so there is some ambiguity. Also, I do not know if libModSecurity 3.0 will change anything in this regard. Maybe @zimmerle / @victorhora want to chime in.

In the ModSecurity Handbook, we propose

SecRule Variable: TX:xxx   -> Capital letters for collection, colon as separator, lowercase variable name using [a-z0-9_]
Action / setvar:  TX.xxx   -> Capital letters for collection, dot as separator, lowercase variable name using [a-z0-9_]

The current usage in CRS and @jnazarioFastly / @csanders are close to this proposal, but the action / setvar syntax writes the collection in lowercase. This is perfectly sound. And it has the advantage of making more of the action text lowercase. However, TX in capital has the advantage of serving as an anchor for the eyes guiding them to the place where the interesting content happens. Given our variable names are really long, this could be an advantage.

Personally, I have no strong feelings either way, but please consider the anchor idea. It is something people overlook very often.

[dune73]

ID Numbering Scheme

I am proposing the following text for the numbering scheme. It is very extensive, but we better get this right and describe it in detail or we'll have to do a lot of cleanup.

The CRS project used the numerical id rule namespace from 900,000 to 999,999 for the CRS rules as well as 9,000,000 to 9,999,999 for default CRS rule exclusion packages.

Rules applying to the incoming request use the id range 900,000 to 949,999.
Rules applying to the outgoing response use the id range 950,000 to 999,999.

The rules are grouped by vulnerability class they address (SQLi, RCE, etc.) or functionality (initialization). These groups occupy blocks of thousands (e.g. SQLi: 942,000 - 942,999).
The grouped rules are defined in files dedicated to a single group or functionality. The filename takes up the first three digits of the rule ids defined within the file (e.g. SQLi: REQUEST-942-APPLICATION-ATTACK-SQLI.conf).

The individual rule files for the vulnerability classes are organized by the paranoia level of the rules. PL 1 is first, then PL 2 etc.

The block from 9XX000 - 9XX099 is reserved for use by CRS helper functionality. There are no blocking or filtering rules in this block.

Among the rules serving a CRS helper functionality are rules that skip rules depending on the paranoia level. These rules always use the following reserved rule ids: 9XX011-9XX018 with very few exceptions.

The blocking or filter rules start with 9XX100 with a step width of 10. E.g. 9XX100, 9XX110, 9XX120 etc. The rule id does not correspond directly with the paranoia level of a rule. Given the size of a rule group and the organization by lower PL rules first, PL2 and above tend to have rule IDs with higher numbers.

Within a rule file / block, there are sometimes smaller groups of rules that belong to together. They are closely linked and very often represent copies of the original rules with a stricter limit (alternatively, they can represent the same rule addressing a different target in a second rule where this was necessary). These are stricter siblings of the base rule. Stricter siblings usually share the first five digits of the rule ID and raise the rule ID by one. E.g. Base rule at 9XX160, stricter sibling at 9XX161.

Stricter siblings often have a different paranoia level. This means that the base rule and the stricter sibling do not reside next to one another in the rule file. Instead they are ordered in their appropriate paranoia level and can be linked via the first digits of the rule id. It is a good practice to introduce stricter siblings together with the base rule in the comments of the base rule and to reference the base rule with the keyword stricter sibling in the comments of the stricter sibling. E.g. "... This is
performed in two separate stricter siblings of this rule: 9XXXX1 and 9XXXX2", "This is a stricter sibling of rule 9XXXX0."

[csanders-git]

Good write up on the rules, looks right to me. I didn't realize we stuck so closely to the 9XX011-9XX018 rule. There is probably something to add about in the event we run out of rule ID's in the step width of 10. Interesting.feedback on the variables. i'm don't feel strongly one way or the other, my previous post only relates to the fact that almost always the entire action is lowercase. It also gives you a reminder to use the dot syntax instead of the colon syntax.

[dune73]

We tried to stick very closely, with only four exceptions (-> 912019, 950020, 950021, 950022) we accepted in order to avoid rule collisions with CRS2. One of the goals of the renumbering in CRS3 was to allow parallel installations with CRS2. With the PL helper rules I made sure we did not spoil that.

You do have a point with the reminder: TX: vs tx.. Only realizing this after your comment.

[fzipi]

I've updated the text with the paranoia levels and rule id numbering. The last thing mentioned which is in the FIXME (and discussed here) is the variable naming conventions.

[dune73]

I think we settled on tx. for the variable names in the actions. I'm fine with that. If you think you can deduce a real variable naming scheme, then please go ahead and write it down. Personally, I get the impression we do not have a stringent convention and personally, I think we can postpone that cleanup as the number of vars is really limited and we use most of them only in 1-2 places.

@fzipi: could you replace the dev branch description with my proposal above, please. I think this got overlooked.

Your PL description had me chuckle. It's a funny text. Maybe we should tune it down a bit though and I also think a discussion is due with regards to your characterisation of the various PLs.

@csanders-git / @lifeforms : Could you please review that part of the PR as well?

[fzipi]

@dune73 The PL description was the one that @spartantri made on PR #887, it aren't my words. It was funny indeed. We should change it though, I agree.

I will replace the tx description while we give another look at the PL description.

[dune73]

I see. :)

However, #887 covers something completely different, I think.

[fzipi]

@dune73 I'm sorry, the funny comment was from issue #882, not PR 887.

[dune73]

Oh, It's in one of the issues. That's why I did not find it myself. Thanks for helping me out.

Yes, It's a funny text, but let's tune it down a bit. Nothing wrong with a joke or too, but this is overdoing it. Besides, we only have PL1-PL4 as of this writing.

[dune73]

In your description, setvar is first, then in the example, SecRule is first. Please align it.

[fzipi]

Done.

[dune73]

Please add a reference to the crs-setup.conf where PLs are introduced and explained. Here we only give some rules of thumb.

[fzipi]

Done.

[fzipi]

Travis is failing from white spacing in comment lines. I don't know if this is the expected behavior.

1. I can correct this spacing, and/or
2. Fix python test to not include comments in whitespace testing.

What do you think?

[dune73]

I think we concluded that we do not want trailing whitespaces no matter where.

An exception would be unit tests that need trailing whitespace as part of their payload, but everywhere else, the whitespace should go.

[fzipi]

Good. Yes, indeed. But I was somewhat reluctant to add "rules" changes in this PR, to keep clearly separated. Now I corrected this to pass travis, but I think maybe this could be handled in a new PR until this one gets merged.

[spartantri]

Nice progress here :)

[dune73]

@csanders-git wrote in the summary that @lifeforms would be assigned, but then it's actually me according to this PR. I think it is time to merge and then update as we see fit.

Thank you for the PR @fzipi, it's a most welcome contribution (file).