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

Rules for NextCloud installs (and possibly OwnCloud). #982

Merged
merged 5 commits into from Feb 5, 2018

Conversation

Projects
None yet
4 participants
@bagley
Contributor

bagley commented Dec 8, 2017

Here are the rules to allow NextCloud (and likely OwnCloud) to be able to work. They are from #899.

As I was making this PR, I noticed that many of the rules are just turning off other rules. I would like for them to specifically target the variables being used, and try to make them more exact. But they are still fairly decent. (I'm just putting this here to remind myself of this).

I added some notes to each rule, to show more of what it is doing. And fixed the formatting. They should be up to par with the Contributing.md.

Rules for NextCloud installs (and possibly OwnCloud).
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
@fzipi

This comment has been minimized.

Collaborator

fzipi commented Dec 8, 2017

@bagley I think this is a very good first approach.

Anyone care to comment?

@dune73

This comment has been minimized.

Collaborator

dune73 commented Dec 8, 2017

I will. But I am a bit short on time these days.

Xeboc added a commit to Xeboc/hegemon that referenced this pull request Dec 29, 2017

@dune73

This comment has been minimized.

Collaborator

dune73 commented Jan 20, 2018

Hi,

I finally took the time to take a closer look at this.
Sorry about the long delay.

This is very good. Clean and systematic.

There are a few minor questions and requests, you might be
able to respond - if you have not given up in the meantime.

  • Why did you pick the rule block 9012 and not 9003, which
    would be the next in the line?
  • I think tx.crs_exclusions_nextcloud=1 ought to be added
    to crs-setup.conf next the wordpress and drupal - and
    consequently removed from the rule exclusion file.
    A link / hint in said file is enough.
  • I really like the way you write your comments / explanations.
    I will reword a few of them after the merge, but that is
    more a project thing. Nothing wrong with your wording.
  • You start your rules 90xx010. Please start with 90xx100.
    That's a general policy in rule files. 0-99 is reserved
    for meta stuff. Then come the real rules, like yours.
  • What I do not like is the way you remove rule by IDs and
    then use an arbitrary range within the rule files to disable
    the whole set of rules in said file.
    E.g. 933100-933200
    I think that's asking for trouble when we we grow over 933200
    in the future and do not think about nextcloud rule exclusions.
  • Can't you remove by tag? If not, then please do 100-999.
  • Can you explain the commented out passage "Testing for
    upload fix". Do we need this? Does it have to be near the
    end of the file?

Thank's once again for this PR. If you have a response to this,
then fine. If not, I'll merge in a week or two and start to
move things around myself afterwards.

@dune73 dune73 referenced this pull request Jan 24, 2018

Merged

Rules for Dokuwiki. #983

@bagley

This comment has been minimized.

Contributor

bagley commented Jan 24, 2018

Yes, I can go ahead and fix these. That "Testing for upload fix" was just there to see if anyone could get that variable based fix to work. But I'm going to put it up to the top with just the 4 lines of code that are actually needed. Give me a few and I'll upload the fixes.

bagley added some commits Jan 24, 2018

Add tx.crs_exclusions_nextcloud=1 to crs-setup.conf.example
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Use a tag rather than a range of rules.
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Condense the explanation at the top of the file.
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
@dune73

This comment has been minimized.

Collaborator

dune73 commented Jan 24, 2018

Wonderful. Thank you very much.

Renumbered rules.
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
@bagley

This comment has been minimized.

Contributor

bagley commented Jan 24, 2018

Ya, no problem. As for the block choosing 9012 rather than 9003, at first I wanted to jump a few blocks in case someone else was using those. And I picked 9012 because 12 devs made nextcloud. Kind of random, but that's why. If you'd like it moved it to 9003, I can do that. I can do the same with dokuwiki, and make it 9004.

Oh, and feel free to reword the comments as needed.

@fzipi

We are nearly there. The ordering of actions in the rules does not adhere to the contributing standard. Most of the rules need to be modified, mainly the disruptive action needs to be moved upwards, after the phase declaration. Also, SecMarker tags must be quoted using double quotes.

I think it can be merged after that. Very good job!

phase:2,\
t:none,\
nolog,\
pass,\
ctl:ruleRemoveByID=933100-933200,\
ctl:ruleRemoveByTAG=attack-injection-php,\

This comment has been minimized.

@fzipi

fzipi Feb 1, 2018

Collaborator

The ctl action is ruleRemoveByTag, not by TAG.

This comment has been minimized.

@dune73

dune73 Feb 5, 2018

Collaborator

Fixing this as of this writing. Found another two small bugs.

This comment has been minimized.

@dune73

dune73 Feb 5, 2018

Collaborator

Fixed bugs in 9012310

@dune73

This comment has been minimized.

Collaborator

dune73 commented Feb 1, 2018

I plan to merge this before the monthly meeting next Monday and fix this small formatting problems immediately afterwards. @bagley has done so much work on this and #983, it's only fair.

@spartantri

This comment has been minimized.

Collaborator

spartantri commented Feb 1, 2018

We really need to write a rule checker script to put the actions in order and use our time wisely :)

@dune73 dune73 merged commit d2a5ac9 into SpiderLabs:v3.1/dev Feb 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dune73

This comment has been minimized.

Collaborator

dune73 commented Feb 5, 2018

Thank you for a great contribution @bagley. Merged this.

Small and final updates after this merge now ready in #1010.

@bagley

This comment has been minimized.

Contributor

bagley commented Feb 5, 2018

Thanks for merging this.

@fzipi, after seeing @dune73 's edits, I'm getting a much better idea of what you meant on the rule reordering. I agree with you about a tool that could be used for checking (and possibly fixing) rules. It wouldn't be very hard to do in perl or something like that. I may even write up something, unless you have already started writing such a tool. (I wrote a quick tool to check for trailing white spaces before git commits, as the failed builds were annoying me).

@bagley

This comment has been minimized.

Contributor

bagley commented Feb 5, 2018

Also, @dune73, I'm not sure if Nextcloud changed the password changing setup, or maybe I missed something. But rule 9003500 needs a change so it won't block passwords. And another rule was needed for user password changes. Here's the changes on line 266 and on:

@@ -264,8 +264,11 @@
             ctl:ruleRemoveTargetByTag=CRS;ARGS:pass2"
 
 # Change Password and Setting up a new user/password
+# /index.php/settings/users
+# /index.php/settings/users/users
+# /index.php/settings/users/changepassword
 
-SecRule REQUEST_FILENAME "@endsWith /index.php/settings/users" \
+SecRule REQUEST_FILENAME "@rx /index.php/settings/users(|/users|/changepassword)$" \
     "id:9003500,\
     phase:2,\
     pass,\
@@ -274,6 +277,16 @@
     ctl:ruleRemoveTargetByTag=CRS;ARGS:newuserpassword,\
     ctl:ruleRemoveTargetByTag=CRS;ARGS:password"
 
+SecRule REQUEST_FILENAME "@endsWith /index.php/settings/personal/changepassword" \
+    "id:9003510,\
+    phase:2,\
+    pass,\
+    t:none,\
+    nolog,\
+    ctl:ruleRemoveTargetByTag=CRS;ARGS:password,\
+    ctl:ruleRemoveTargetByTag=CRS;ARGS:oldpassword,\
+    ctl:ruleRemoveTargetByTag=CRS;ARGS:newpassword,\
+    ctl:ruleRemoveTargetByTag=CRS;ARGS:newpassword-clone"
 
 SecMarker END-NEXTCLOUD-ADMIN
@spartantri

This comment has been minimized.

Collaborator

spartantri commented Feb 6, 2018

I will love to have a python checking script that implements all the things from CONTRIBUTING.md and fix everything automatically, no only blank tailing spaces (I use sed for that 's,\s+$,,').

@fzipi

This comment has been minimized.

Collaborator

fzipi commented Feb 6, 2018

I made a parser for the CRS that has now full syntax sanity checks (thanks to @fgsch).

I was hoping to get full python objects from that to rewrite as needed but haven't had time for keeping the development.

@dune73

This comment has been minimized.

Collaborator

dune73 commented Feb 8, 2018

Thank you for the update @bagley, I'll incorporate that in the other PR.

As for the script: @fzipi's work together with @fgsch's contribution is what you should base yourself on. It would help the project and other contributors a big deal. Please don't revert to perl, if you can do it in python. And yes, contributing this would be awesome (on top of two very nice rule exclusion packages you contributed).

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