New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop unneeded capture groups #1124

Closed
wants to merge 1 commit into
base: v3.2/dev
from

Conversation

Projects
None yet
6 participants
@fgsch
Copy link
Collaborator

fgsch commented Jun 11, 2018

Follow up of #1111.

This covers all the remaining rules. Minor style changes while here.

For the curious, for this PR I wrote a small program that prints the number of capture groups using PCRE_INFO_CAPTURECOUNT and run it through all the patterns to have a list of rules that needed reviewing.

@fgsch fgsch force-pushed the fgsch:fgsch/capture-group-contd branch 4 times, most recently from 7b00c32 to 652d862 Jun 11, 2018

@emphazer

This comment has been minimized.

Copy link
Collaborator

emphazer commented Jun 12, 2018

@fgsch did you test the regexp assemble result with a diff?

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Jun 13, 2018

@emphazer What do you mean?

@emphazer

This comment has been minimized.

Copy link
Collaborator

emphazer commented Jun 13, 2018

i ment the ./regexp-assemble.pl result of regexp-942130.data and regexp-942180.data.

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Jun 13, 2018

I got that part :) What do you mean with test the result with a diff?

FWIW, the result of the regexp assemble is what's in the rules. I did not change the pattern manually for the generated ones but pasted the result.

@emphazer

This comment has been minimized.

Copy link
Collaborator

emphazer commented Jun 13, 2018

i wasn't sure if you have to backtick some parts.
your work looks good so far. :)

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 7, 2018

This looks good so far but there are a lot of changes.

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Jul 7, 2018

@csanders-git I agree. Maybe people can comment below each change?

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 7, 2018

Good idea!!

@@ -454,7 +454,7 @@ SecRule REQUEST_HEADERS:Content-Type "@rx ^(?:application\/x-www-form-urlencoded
ver:'OWASP_CRS/3.0.0',\
severity:'WARNING',\
chain"
SecRule REQUEST_BODY|XML:/* "@rx \%((?!$|\W)|[0-9a-fA-F]{2}|u[0-9a-fA-F]{4})" \
SecRule REQUEST_BODY|XML:/* "@rx \%(?:(?!$|\W)|[0-9a-fA-F]{2}|u[0-9a-fA-F]{4})" \

This comment has been minimized.

@csanders-git

csanders-git Jul 10, 2018

Collaborator

clearly equivalent... G2G

@@ -1194,7 +1194,7 @@ SecRule REQUEST_BASENAME "@endsWith .pdf" \
setvar:'tx.%{rule.id}-OWASP_CRS/PROTOCOL_VIOLATION/INVALID_HREQ-%{matched_var_name}=%{matched_var}'"


SecRule ARGS "@rx \%((?!$|\W)|[0-9a-fA-F]{2}|u[0-9a-fA-F]{4})" \
SecRule ARGS "@rx \%(?:(?!$|\W)|[0-9a-fA-F]{2}|u[0-9a-fA-F]{4})" \

This comment has been minimized.

@csanders-git

csanders-git Jul 10, 2018

Collaborator

clearly equivalent... G2G

@@ -305,7 +305,7 @@ SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|ARGS_NAME
setvar:'tx.anomaly_score=+%{tx.critical_anomaly_score}',\
setvar:'tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/SQLI-%{matched_var_name}=%{tx.0}'"

SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* "@rx (?i:(?:(union(.*?)select(.*?)from)))" \
SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* "@rx (?i:)union.*?select.*?from" \

This comment has been minimized.

@csanders-git

csanders-git Jul 10, 2018

Collaborator

Seems to be equivalent, good to go.

@@ -513,7 +513,7 @@ SecRule TX:PARANOIA_LEVEL "@lt 2" "id:942014,phase:2,pass,nolog,skipAfter:END-RE
# Identifies common initial SQLi probing requests where attackers insert/append
# quote characters to the existing normal payload to see how the app/db responds.
#
SecRule ARGS_NAMES|ARGS|XML:/* "@rx (^\s*[\"'`;]+|[\"'`]+\s*$)" \
SecRule ARGS_NAMES|ARGS|XML:/* "@rx (?:^\s*[\"'`;]+|[\"'`]+\s*$)" \

This comment has been minimized.

@csanders-git

csanders-git Jul 10, 2018

Collaborator

Seems to be equivalent, good to go.

@@ -587,9 +587,9 @@ SecRule ARGS_NAMES|ARGS|XML:/* "@rx (?i:(?:(?:^|\W)in[+\s]*\([\s\d\"]+[^()]*\)|\
# ./regexp-assemble.pl regexp-942130.data
# Note that after assemble an outer bracket with an ignore case flag is added
# to the Regexp::Assemble output:
# (?i:ASSEMBLE_OUTPUT)
# (?i)ASSEMBLE_OUTPUT

This comment has been minimized.

@csanders-git

csanders-git Jul 10, 2018

Collaborator

this is a comment...?

This comment has been minimized.

@fgsch

fgsch Jul 10, 2018

Collaborator

Yes, this is to keep the comment in sync with the change below.

This comment has been minimized.

@franbuehler

franbuehler Jul 31, 2018

Collaborator

Comment was in sync with the rule below. No change needed here.

This comment has been minimized.

@fgsch

fgsch Jul 31, 2018

Collaborator

@franbuehler No, I changed the regexp. Before:

(?i:([...

Now:

(?i)[...

So I updated the comment to be in sync with my change.

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Oct 2, 2018

This is a welcome PR. However, it has not been merged in months and the review is still not complete. We have therefore taken a closer look during the community chat on Slack last night.

The PR aims for 3.1, but given the change is not directly relevant to users and given it is a quite a big change coming in after the RC, we will not take it into the 3.1 release.

This means, the PR will need to have moved to 3.2/dev. Maybe @franbuehler wants to do the review.

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Dec 3, 2018

@lifeforms lifeforms added this to the CRS v3.2.0 milestone Dec 3, 2018

@fgsch fgsch force-pushed the fgsch:fgsch/capture-group-contd branch from 652d862 to be6543b Dec 3, 2018

@fgsch fgsch changed the base branch from v3.1/dev to v3.2/dev Dec 3, 2018

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Dec 3, 2018

Rebased against v3.2/dev.

Drop unneeded capture groups
This covers all but 3 of the remaining offenders.  Minor style
changes while here.

@fgsch fgsch force-pushed the fgsch:fgsch/capture-group-contd branch from be6543b to 5dfb9b5 Dec 3, 2018

fgsch added a commit to fgsch/owasp-modsecurity-crs that referenced this pull request Dec 4, 2018

Drop unneeded capture groups and tidy up regexp
Originally part of PR SpiderLabs#1124 but splitting it in multiple PRs to
make reviewing easier.

fgsch added a commit that referenced this pull request Dec 4, 2018

Drop unneeded capture groups and tidy up regexp
Originally part of PR #1124 but splitting it in multiple PRs to
make reviewing easier.
@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Dec 4, 2018

Superseded by #1252 and upcoming PRs.
Leaving it open until all the changes in this PR are covered.

@fgsch fgsch added On Hold and removed v3.2-dev labels Dec 4, 2018

fgsch added a commit to fgsch/owasp-modsecurity-crs that referenced this pull request Dec 4, 2018

Drop unneeded capture groups and tidy up regexp
Originally part of PR SpiderLabs#1124 but splitting it in multiple PRs to
make reviewing easier.

@fgsch fgsch removed this from the CRS v3.2.0 milestone Dec 4, 2018

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Dec 4, 2018

For the record here is the list of all the rules with capture groups:

 Rule 901420 has 1 groups
 Rule 901440 has 1 groups
 Rule 910140 has 1 groups
 Rule 912150 has 1 groups
 Rule 920230 has 1 groups
 Rule 920240 has 1 groups
 Rule 920440 has 1 groups
 Rule 920480 has 1 groups
 Rule 921180 has 1 groups
 Rule 931130 has 1 groups
 Rule 932140 has 1 groups
 Rule 942110 has 1 groups
 Rule 942180 has 1 groups
 Rule 942250 has 1 groups
 Rule 942420 has 1 groups
 Rule 942421 has 1 groups
 Rule 942430 has 1 groups
 Rule 942431 has 1 groups
 Rule 942432 has 1 groups
 Rule 943110 has 1 groups
 Rule 951240 has 1 groups
 Rule 901430 has 2 groups
 Rule 920190 has 2 groups
 Rule 942440 has 2 groups
 Rule 942270 has 3 groups
 Rule 942130 has 11 groups
 Rule 941190 has 17 groups
 Rule 941250 has 25 groups
 Rule 941330 has 60 groups
 Rule 941220 has 136 groups
 Rule 941210 has 168 groups

fgsch added a commit to fgsch/owasp-modsecurity-crs that referenced this pull request Dec 5, 2018

Drop unneeded capture groups and tidy up regexp
Originally part of PR SpiderLabs#1124 but splitting it in multiple PRs to
make reviewing easier.
@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Dec 12, 2018

The remaining rules except for the last 2 are covered in #1257.
Closing.

@fgsch fgsch closed this Dec 12, 2018

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Dec 12, 2018

How about the remaining two?

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Dec 12, 2018

Once the outstanding PR is merged I will open a new PR for those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment