8000 phpBB exclusion rules file by azurit · Pull Request #1893 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

phpBB exclusion rules file #1893

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Nov 2, 2020
Merged

phpBB exclusion rules file #1893

merged 7 commits into from
Nov 2, 2020

Conversation

azurit
Copy link
Member
@azurit azurit commented Oct 3, 2020

No description provided.

@dune73 dune73 mentioned this pull request Oct 4, 2020
Closed
@lifeforms
Copy link
Member
lifeforms commented Oct 5, 2020 •
edited
Loading

TODOs from meeting:

  • replace @rx with @pm, pending some checks by airween about performance
  • outcomment the last rule as it may be useful while not being harmful
  • as for the first rule 9007000, we may have a misunderstanding so to clear up, I think it will be safer to uncomment it as a matter of style and to make sure another phpbb rule writer who uses phase 1, will also have the correct behavior (if we comment that rule out, then modsec won’t skip over this file if a user has crs_exclusions_phpbb=0)

@azurit
Copy link
Member Author
azurit commented Oct 6, 2020

@lifeforms Should i send a new PR or is it possible to update this one?

@azurit
Copy link
Member Author
azurit commented Oct 6, 2020 
* if we comment that rule out, then modsec won’t skip over this file if a user has crs_exclusions_phpbb=0

I understand that these rules will be activated only with crs_exclusions_phpbb=1 but, in that case, they will be (really?) executed twice: in phase 1 and also in phase 2. But, as you are more experienced in CRS and rules writing than me, i will follow your suggestion and uncomment it. Thank you!

@azurit
Copy link
Member Author
azurit commented Oct 7, 2020 
* if we comment that rule out, then modsec won’t skip over this file if a user has crs_exclusions_phpbb=0

I understand that these rules will be activated only with crs_exclusions_phpbb=1 but, in that case, they will be (really?) executed twice: in phase 1 and also in phase 2. But, as you are more experienced in CRS and rules writing than me, i will follow your suggestion and uncomment it. Thank you!

I realized you are completely right - every rule has 'phase' setting which defines in which phase it will be executed. Sorry for confusion!

So, should i create a new PR and close this one?

@dune73
Copy link
Member
dune73 commented Oct 7, 2020

From my perspective, you can fix this PR easily by adopting the changes @lifeforms proposed above.

However, it may be tricky this PR is not based on a branch, but your main repository. So it depends a bit on you and if you can make it work. If it's too complicated, starting from scratch is probably easier.

Either way, I am missing a change to the CONTRIBUTORS.md file. I think it's about time you place yourself among the developers. Ideally with your real name between Manuel and Felipe.

@azurit
Copy link
Member Author
azurit commented Oct 7, 2020

All done.

@dune73
Copy link
Member
dune73 commented Oct 7, 2020

@lifeforms : Ready to merge?

@lifeforms lifeforms self-assigned this Oct 9, 2020
@lifeforms
Copy link
Member

Thank you, it is looking great! I will review it, try it out on a phpBB install, and merge it this weekend!

@azurit azurit mentioned this pull request Oct 10, 2020
Closed
@lifeforms
Copy link
Member
lifeforms commented Oct 11, 2020 •
edited
Loading

I have tried out the PR, thank you!

I would recommend the following changes:

  • It should be possible to enable phpBB support easily in the crs-setup.conf.example template in rule 900130. Example:
SecAction \
 "id:900130,\
  phase:1,\
  nolog,\
  pass,\
  t:none,\
  setvar:tx.crs_exclusions_cpanel=0,\
  setvar:tx.crs_exclusions_dokuwiki=0,\
  setvar:tx.crs_exclusions_drupal=0,\
  setvar:tx.crs_exclusions_nextcloud=0,\
  setvar:tx.crs_exclusions_phpbb=0,\
  setvar:tx.crs_exclusions_wordpress=0,\
  setvar:tx.crs_exclusions_xenforo=0"
  • I ran into an error during phpBB install as it asked for a http:// or https:// URL. It can be remedied by adding a rule:
# Installation
SecRule REQUEST_FILENAME "@endsWith /install/app.php/install" \
    "id:9007190,\
    phase:2,\
    pass,\
    t:none,\
    nolog,\
    ver:'OWASP_CRS/3.4.0',\
    ctl:ruleRemoveTargetById=931130;ARGS:server_protocol"
  • I hit some false positives in a posted message. I would recommend to switch off all CRS rules on the message fields, instead of only partially excluding them from some attack rules. IMHO, we can assume that these fields are properly handled by phpBB.
            ctl:ruleRemoveTargetByTag=OWASP_CRS;ARGS:message,\
  • Finally, there are some commented-out lines in the file such as:
# Login, registration and password change
#SecRule REQUEST_FILENAME "@endsWith /ucp.php" \
#    "id:9007100,\

and

#            ctl:ruleRemoveTargetByTag=attack-injection-php;ARGS:subject_checked
#            ctl:ruleRemoveTargetByTag=attack-sqli;ARGS:subject_checked

In my opinion if a line is not active, it's better to remove it, to keep the file clean.

All in all, a very nice contribution - especially because forums are attacked often, so the CRS should be very valuable for this use case!

@azurit
Copy link
Member Author
azurit commented Oct 11, 2020 •
edited
Loading

Thanks for comments!

* I hit some false positives in a posted message. I would recommend to switch off all CRS rules on the `message` fields, instead of only partially excluding them from some attack rules. IMHO, we can assume that these fields are properly handled by phpBB.

Ok but i don't like to completely disable CRS like this. What about adding additional check using @validateUtf8Encoding like i did with password fields?

@azurit
Copy link
Member Author
azurit commented Oct 11, 2020

By the way, is replacing @rx wtih @pm really safe? See rules 9007140 and 9007150. The original version looked like this:

SecRule ARGS:mode "@rx ^(?:post|edit|quote|reply)$" \
SecRule ARGS:mode "@rx ^(?:compose|drafts)$" \

I mean, @pm is acting like @contains so it will match also other values for ARGS:mode.

@lifeforms
Copy link
Member
  • I hit some false positives in a posted message. I would recommend to switch off all CRS rules on the message fields, instead of only partially excluding them from some attack rules. IMHO, we can assume that these fields are properly handled by phpBB.

Ok but i don't like to completely disable CRS like this. What about adding additional check using @validateUtf8Encoding like i did with password fields?

In other exclusion sets, we disable the CRS fully on certain parameters too. In my opinion, the validateUtf8Encoding should be removed.

I think this does not add to security; attacks in general will be expressed in valid UTF-8.

We have the crs-setup.conf setting setvar:tx.crs_validate_utf8_encoding=1 for the users who want to do the UTF-8 validation check (this works globally on all parameters). The users who are sure that they handle only UTF-8 have that option. Doing the check in the exclusion file, even when they disable global validation, may even be harmful in some cases (when other encodings are used, admittedly this may be very rare).

@lifeforms
Copy link
Member

By the way, is replacing @rx wtih @pm really safe? See rules 9007140 and 9007150. The original version looked like this:

SecRule ARGS:mode "@rx ^(?:post|edit|quote|reply)$" \
SecRule ARGS:mode "@rx ^(?:compose|drafts)$" \

I mean, @pm is acting like @contains so it will match also other values for ARGS:mode.

Yes, you are totally correct. @pm will apply the exclusion also in case of a substring match (e.g. postage, apostle). I think that exactly due to this reason, your earlier version using @rx was better. It's safer to require that the string is an exact match.

@lifeforms
Copy link
Member

Finally, I have one small nitpick:

#  setvar:tx.crs_exclusions_xenforo=1,\
#  setvar:tx.crs_exclusions_phpbb=1"

The phpbb is now at the bottom of rule 900130, but it would be more clean to sort these variables alphabetically.

Now that I look at it, it appears that drupal and docuwiki are also in the wrong order 😬 Maybe you could fix that at the same time?

@azurit
Copy link
Member Author
azurit commented Nov 1, 2020

Everything makes perfect sense to me, done!

@lifeforms
Copy link
Member

Awesome, thank you, merging!

@lifeforms lifeforms merged commit 078b86e into coreruleset:v3.4/dev Nov 2, 2020
@dune73 dune73 mentioned this pull request Nov 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lifeforms lifeforms

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@azurit @lifeforms @dune73
0