Owasp-modsecurity-crs: Review severity levels of CRS to make sure all rules have severity levels

[csanders-git]

[lifeforms]

Hopefully we could lint the rules automatically.

I currently use the following primitive script to detect missing tags and severities:

# check missing attack tags
./cont.py rules/*.conf|grep ^SecRule|grep -v TX:PARANOIA_LEVEL|grep -v id:980|grep -v id:901|grep -v id:900|grep -v 959100|grep -v skipAfter|grep -v attack-|grep -o id:\\d\\+
# check missing severity tags
./cont.py rules/*.conf|grep ^SecRule|grep -v TX:PARANOIA_LEVEL|grep -v id:980|grep -v id:901|grep -v id:900|grep -v 959100|grep -v skipAfter|grep -v severity|grep anomaly|grep -o id:\\d\\+
# check wrong severities
./cont.py rules/*.conf|grep ^SecRule|grep -v TX:PARANOIA_LEVEL|grep -v id:980|grep -v id:901|grep -v id:900|grep -v 959100|grep -v skipAfter|grep critical_anomaly|grep -v CRITICAL|grep -o id:\\d\\+
./cont.py rules/*.conf|grep ^SecRule|grep -v TX:PARANOIA_LEVEL|grep -v id:980|grep -v id:901|grep -v id:900|grep -v 959100|grep -v skipAfter|grep warning_anomaly|grep -v WARNING|grep -o id:\\d\\+

cont.py just glues lines with \ together so any rule is one line:

#!/usr/bin/env python

import fileinput, sys

for line in fileinput.input():
    line = line.strip()
    if line == '':
        sys.stdout.write("\n")
        continue

    if line[-1] == '\\':
        sys.stdout.write(line[0:-1])
    else:
        sys.stdout.write(line)
        sys.stdout.write("\n")

[csanders-git]

Yeah i'm pretty much all but ready to have a serious linter as part of the travis CI integration that i plan to roll out soon :)

[emphazer]

@lifeforms that python script is a nice and dirty way to fix the newline problems with the apache < 2.4 bug https://bz.apache.org/bugzilla/show_bug.cgi?id=55910
for rhel7/centos7
:-)
thx

[dune73]

This is aimed for 3.1.0 and I - sorry for repeating myself - think it needs to be part of a bigger initiative doing the following:

• reformat all rules 100% consistently
• check severities
• re-order actions by a strict policy which we will have to discuss and define beforehand
• have a clear transformation concept and apply it consistently
• comb all rule messages and update as appropriate
• check all logdata actions and do housekeeping

Otherwise, I welcome this completely.

[csanders-git]

:-D i'm glad this ticket has lots of traction - I think it would behoove us to use a proper parser for this and not just make it dirty, that way we can say 100% this will work in ModSecurity (at least 3, i can't speak for apache :-P )

[lifeforms]

Yeah using the modsec parser would be very cool to do rule validation. At the other hand, the parser might not retain source information such as spacing and lines conventions, so we'll probably want both stupid and smart scripts :)

[csanders-git]

Absolutely @lifeforms -- I literally cannot wait to have TavisCI running on our code and making sure we don't have little issues, like missing regression tests, bad formatting, etc.... we're gonna #devops so hard with 3.1 :)

[dune73]

How about the python module to pull this off?

[csanders-git]

Thats what i was thinking @dune73 - it'd be great if it was in python since the rest of the project pretty much is. I really wanted to straight up convert the parser from c++ to python but the python regex engine is different :( so a little work there

[dune73]

Way to go then.

[csanders-git]

Haven't started yet but for CRS 3.1 this is a must in my opinion, i'm sure once we push 3.0.0 we'll have a nice long break and a conversation about what should make the cut :)

[dune73]

Yes, I suggest we rent a boat in the Carribean somewhere and talk it out - for a week or two. ;)

On a more serious note, I think we have to force ourselves to clean out the pile of old issues before we touch anything 3.1.

[csanders-git]

good idea about cleaning out the log .... if the flight wouldn't be so expensive for you two i'd do the boat trip, I could use one :-P Maybe i'll go to the next AppSec EU instead :)

[dune73]

I'm seeing myself at the AppSecEU as well next year. It's been a while.

[dune73]

I see it's Belfast. Hmm. Never been to Ireland. But it's like half way to the Caribbean. Makes me think...

[fzipi]

@dune73 What is remaining to do here? Maybe we can have a pass of airween 's magical tool here also, identifying what changes remain?

[airween]

What do you think about this? (List of severity levels at every rule.)

If you have any idea, just let me know.

[fzipi]

What I see is that there are lots of rules without severity :open_mouth:

[fgsch]

I think a lot of the rules missing severity are the paranoia level checks.
Could we exclude those from the list?

[dune73]

Yes, the rule IDs below xxx100 probably don't need the severity.

I made some comment above what else needs to be done. I stand by that comment, but I think it should not keep this issue back. Let's get this over and do the rest afterwards.

[airween]

Yes, the rule IDs below xxx100 probably don't need the severity.

Right, it's important info. Any other criteria? I mean, severity needs only if rule contains any disruptive action?

[dune73]

I'd skip the rule files below 910 and also 949, 959 and 980.

[fgsch]

Perhaps rather than skipping ranges we could exclude some of these files altogether?

[dune73]

That's what I tried to express, yes.

[fgsch]

Sorry, I can't read. Reading it again, your comment was clear.

[airween]

Table updated.

The used filter was:

    if ruleid%1000 >= 100 and
        round(current_ruleid/1000) not in [949, 959, 980] and
        round(current_ruleid/1000) > 910 and
        current_ruleid < 1000000

Tha last condition filters out the rules with id 900NNNN...

[annawinkler]

I would be happy to help out with this issue. I was talking with @airween about it and I can take an initial go at updating the rules in the range listed. Are there any other suggested heuristics for setting the severity?

[airween]

After dissussing with @fgsch I updated the table.

This table also filtered the rules with this condition:

if current_ruleid%1000 >= 100 and
        round(current_ruleid/1000) not in [949, 959, 980] and
        round(current_ruleid/1000) > 910 and
        current_ruleid < 1000000

The concept what I checked: the code follows the used PL (which controlled with a special rule what checks the TX:EXECUTING_PARANOIA_LEVEL variable) (this is the column F). It collects the used severity action's value (column G). Column E shows the disruptive actions (if exists) or pass, for help. Column H contains the used variable name at setvar action, eg tx.anomaly_score_pl1, and column I shows the given value (also in that setvar action), eg: +%{tx.notice_anomaly_score}'.

If the used PL not equals with variable name (column F and H) then it marked in column J.

If the given severity level not equals with variable value (column G and I) then it marked in column K.

I think I still can't decide when needs a rule any severity action, that's why I put this info into column E.

Hope it will helpfully :).

[fgsch]

Meeting outcome: @airween with make sure this issue is solved together with Anna.

[airween]

Updated the table, please review this issue the next monthly chat.

[dune73]

Decision during the CRS project chat on March 2, 2020: @lifeforms will sort this out.

https://github.com/SpiderLabs/owasp-modsecurity-crs/issues/1683#issuecomment-593584538

[lifeforms]

Addressed in #1732. Thanks for the help, everyone!