Working Dokuwiki and Nextcloud rulesets. #899
Conversation
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
|
Hey @bagley. This is very cool. Thank you very much for this initial submission. AFAICT, we do not have any DocuWiki and Nextcloud users in the project. So we would need to rely on you for the finalization, testing and possibly maintenance of this rule set. We love contributions like this, but it also means that there is a bit more work for you. Are you willing to to that? Right now, Travis (our continuous integration checker) complains a lot about trailing spaces in your code. We are currently working very hard on strict coding guidelines. That's a pain in the ass, but it also guarantees better readability and easier maintenance in the future. |
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
|
Well, I got those trailing spaces. It took a few tries (I tried to squash the commits but I didn't know the right way to do it). I'm good with testing and finalizing it. And maintenance as far as I can. I've been using it on my own sites, and I tried to make them work on all cases I use and could think of, excluding all of the plugins that can be used on them - I can't test every plugin, but I did test many of the most popular ones. |
|
That sounds very good @bagley. Thank you very much. Covering all the plugins is certainly beyond our standard coverage. It's the same with WP and Drupal. Probably even less than what you did. At what Paranoia Level did you test this? Our general plan is to have exclusion rule packages cover up to PL2. The rules are written in a clean style. That is already very nice. But given we are now super-strict, it would be nice if you could edit your file to follow these soon to be merged guidelines. https://github.com/SpiderLabs/owasp-modsecurity-crs/pull/879/files I think, it is best we merge your code afterwards and then I will edit it a bit, namely comments, grouping of rules and other things that are hard and cumbersome to explain. You would then try this edited rule package again and when it still performs as expected we are done. So these would be the steps: Are you OK with this plan? |
|
I actually just did Paranoia Level 1. However, I will increase it to level 2 and test that out, and modify it accordingly (I've seen other rulesets group PL2 rules by themselves, so I will do that if needed. There's already a section in both rulesets for that, as I copied one of the other rulesets for my template). I'm good with that plan. I will take a look at those guidelines this next week, and format the code as needed. And I'll test it with v3.1/dev. And then I'll push the results here for step 2 of the plan. |
|
This sounds very good. Thanks a lot. Looking forward to this. |
|
Any progress here @bagley? |
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
|
Well, I got it tested against v3.1dev, on level 2. Sorry, it took longer than expected. But I finished the testing and it looks good. I just added some things for the builtin/common apps, so they will work. Go ahead and feel free to merge it and proceed with the plan. One note: increasing it to Paranoia Level 2 made it start throwing messages about "PCRE limits exceeded" when viewing picture thumbnails in the file manager. Increasing the following settings fixed that. It just took somewhat longer for the page to come up. SecPcreMatchLimit 150000 (was 1000 in PL1) The errors referred to the rules 932100, 932105, 932110, 932115, and 932150 (which only run in PL2+). I didn't add exceptions for those rules as increasing the above settings took care of the messages. Just letting you know in case that may be an issue or raise a concern. |
|
Thank you. The PCRE problems are frequent with CRS / ModSecurity. Raising the limits to 150K is within reason. Surprised you actually did notice some lag, but 150K is about where the lag becomes noticeable. I'm off for two days and will take a close look after my return. |
|
Hello all, As already discussed with @dune73 on the mailing list, I am trying to make ModSecurity v3/Nginx 1.12.1 working together with NextCloud 12.0.3 on a Debian Jessie. I am using the latest available code from the git repository for ModSecurity v3 and its nginx connector and the owasp 3.0.2 rules. I tried the rulesets from @bagley but still have exceptions from ModSecurity. I joined my config file and detailed log. In crs-setup-cloud2.conf I allowed all the HTTP methods and content-types. Here a short resume of the ModSecurity exceptions log : Compared to before using @bagley rulesets, it seems I don't have anymore 953120 PHP source code leakage or 941100 XSS Attack Detected via libinjection. Thanks for your help, and let me know if you need to do some testing. crs-setup-cloud2.conf.txt |
|
I am also joining a second log from exceptions due to file not uploaded from the nextcloud desktop client to the server. |
|
Hey @Aurelien-, Feedback on the remaining alerts:
I think this should be tuned away too. Or am I missing something? |
|
Thanks @dune73 for your feedback. Unfortunately I think I made some mistake on my configuration and the Nextcloud rules was probably not working. So I took a serious look at what @bagley did this weekend and edited the REQUEST-903.9012-NEXTCLOUD-EXCLUSION-RULES.conf file to cover the alerts I got by exploring the nextcloud website and doing some basic sync file stuff with the desktop client. I do not know much about writing correct rules and I mostly used the ctl:ruleRemoveByID and tx.allowed_method for specific urls. I put all what I done at the bottom of the file starting with rule id 9012225 (just after 9012220 dealing with password). Here the rules : I will provide the log files for each case so maybe someone can refine the rules and use something else than ruleRemoveByID when possible. One question, should we not use the more strict @beginswith instead of @contains for url prefix ? |
|
Here the log about the text/vcard and forbidden file extension. Forbidden file extension happened when I tried to synchronize file with .log extension with the Nextcloud desktop client. So I disabled the rule checking extensions for /remote.php/dav/files/. The rules from REQUEST-903.9012-NEXTCLOUD-EXCLUSION-RULES.conf : modsec-extension-log1.txt |
|
Then when trying to insert some comments on file/folder/pictures from the Nextcloud web interface I got some JSON parsing error with char \x0a. So I disabled request body check for comments but also other url from admin/user storage pages. I think it can be refine for taking care only about the char \x0a. This error happen when accessing url from : /remote.php/dav/comments/files/ The rules from REQUEST-903.9012-NEXTCLOUD-EXCLUSION-RULES.conf : modsec-comments.txt |
|
There are some PHP code leakage warnings so I disabled the rule for the concerned urls. But this should be investigated, maybe some wrong way of doing some stuff from the Nextcloud php code ? modsec-calendar.txt |
|
There is also a XML parsing error on body request check during access to the address book page from web interface. |
|
Finally I also modified rule 9012010 about the File Manager at the beginning of the conf file with ruleRemoveByID for 941100 and 941130 because of the false positive you already noticed Here the rule : And addition of PUT DELETE for the video chat app and PROPFIND for /public.php/webdav/ |
|
It should also be better to combine the urls with same rules to process all together like for PHP code leakage or Json parsing error. Best. Aurélien |
|
Thanks for including those. I'm out away from a system where I can test those. But I'll be back where I can test those out and help out more next week. |
|
Whenever you find the time. |
|
I wonder if it wouldn't be smarter to leave out the index.php in the @contains sections (or make it an optional group); own/nextcloud do offer to use mod_rewrite to hide the index.php from the url. |
|
@macgeneral : Would that mean to do |
|
@emphazer: is your only concern that another CMS running on the same host (with the same config) could „bypass“ mod_security through similar named files/urls? And how would the index.php prevent that? |
|
@dune73 ok, thats right. I think it should be turned off by default then. @macgeneral yes, there are not that many cms with this url path. But a lot with word like thumbnail and core etc... |
|
We do something similar to #899 (comment), but we use 'SecWebAppId' instead. and in our local ruleset We use Drupal and Owncloud, so this had help us to set the correct context for our rules. In fact, this enables migrating your testing ruleset to production without changing names. We used to do a lot of SERVERNAME dependant before, but from staging/testing to production it was a pain to rename rules. I wish |
|
@bagley Haven't you got any problems with SecRequestBodyLimit (https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual#secrequestbodylimit)? I had to go to max size in my owncloud install. Maybe add something like this would help in this case: To use the web uploader without size glitches. |
|
Well...I didn't have to wait until christmas to see my wish coming true! owasp-modsecurity/ModSecurity@23cf656 Thanks @zimmerle for adding 'SecWebAppId' support! |
|
That directive / variable is greatly underappreciated. Useful for all sorts of funny things. |
|
Any news here @bagley? |
| #SecRule REQUEST_FILENAME "@endsWith /lib/exe/ajax.php" \ | ||
| # "id:9050004,phase:2,t:none,nolog,pass,\ | ||
| # chain" | ||
| # SecRule ARGS:do "streq edit" \ |
There was a problem hiding this comment.
I think this should probably be @streq
|
|
||
|
|
||
| # Reset password | ||
| SecRule REQUEST_FILENAME "@contains /doku.php" \ |
There was a problem hiding this comment.
would @endsWith be more appropriate?
| #SecRule REQUEST_FILENAME "@beginsWith /doku.php" \ | ||
| # "id:9050004,phase:2,t:none,nolog,pass,\ | ||
| # chain" | ||
| # SecRule ARGS:do "streq edit" \ |
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
…orking. Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
|
@lifeforms Thanks for telling us about those. I have made the changes and pushed them. @aurelien Sorry about the delay. I added the rules about the file manager. I'm not sure why you were getting a "PHP code leakage check". That sounds like there's some other issue. I wasn't able to reproduce it. Are you still getting it if you remove those rules? Have you checked that the webpages aren't leaking code out, such as a php error? I'm not able to reproduce the issues with the rules referencing the storage pages on the the web interface. I can take a deeper look with those, but is it when you are submitting the forms, or just loading the page? As for using "beginsWith", the issue with using that is those installs that are using a subdirectory won't be able to use the rules. Using "endsWith" will be better, or "contains" when that doesn't work. @fzipi With the uploading issue, I wasn't uploading anything too large, so I didn't see that issue. I tried to implement your suggestion, but I couldn't get it to work. It appears that requestBodyLimit doesn't like being told to use a variable. It's commented out at the bottom of the file if anyone wants to take a look at it. With the index.php question, my thoughts are to leave it in. It's a simple search and replace in the rules to remove it, if anyone has the mod_rewrite setup. But if anyone sends me donuts, I may reconsider :) |
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
|
I tried to rebase it to 3.1/dev, but it listed lots of conflicts and issues. So I put it back to 3.0/dev. (I'm still testing with 3.1/dev). |
|
I see. You should rebase to v3.1/dev tough, I can work with you on that. You'll have problems with many things also:
I always hesitate to do it, because when you need it you will not have it, but it is possible to add
For the bodylimit size, you could use something like this (I'm using this one in my internal owncloud): |
|
That's good to know about the 507 error. Good ol log fill ups. I don't think I'll include it as people probably need to see those errors, but we can definitely add it to the documentation. DVDs, lol. For that upload limit, I'm just wondering if we really need this. Seems like it would go under something more "custom" like file extensions, than something global. And as AFAIK the limit can be set higher inside of just a single app/VirtualHost container using SecRequestBodyLimit. We can include an example that can be added their app's config (VirtualHost): Another option is including a variable to enable the higher limit, such as "setvar:tx.crs_nextcloud_max_upload=1073741824" or "setvar:tx.crs_nextcloud_allow_max_upload=1" It just has to be something that they enable, and that was where I was having issues with the variables. Because otherwise people could be DOSed on upload.php, and not know why/how their SecRequestBodyLimit=1000 is still accepting large uploads. |
|
@bagley Well, let's do this: as there are no additions/changes to the base rules, just the addition of the Then create a PR against v3.1/dev, and add as reference this closed PR. Similar for dokuwiki (so there will be two independet PRs). This will simplify a lot the process for us. Are you ok with that? |
|
This is very good. Thank you. |
I made these two rulesets for Dokuwiki and Nextcloud for my sites a while ago, and they have been working great. Feel free to modify if you want/need to, but just wanted to share so others can use them.