Skip to content
This repository has been archived by the owner on May 14, 2020. It is now read-only.

Rules for Dokuwiki. #983

Merged
merged 16 commits into from
Feb 5, 2018
Merged

Rules for Dokuwiki. #983

merged 16 commits into from
Feb 5, 2018

Conversation

bagley
Copy link
Contributor

@bagley bagley commented Dec 8, 2017

Here are rules to allow Dokuwiki to work with modsecurity. They were in #899 but it was decided to make them their own PR.

Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
@dune73
Copy link
Contributor

dune73 commented Jan 24, 2018

Again, a very decent PR.

The same general remarks I made about #982 apply here as well.

An additional question:

  • 9011200: disabling CRS completely for the username parameter on the login screen looks a bit extreme. Is that really necessary?

And then addresses to everybody: There are deeply chained rules in this PR. Do we want to additional indentation for every level, or stick to a 2nd level in that case?

If you do not have the time, to look into this again @bagley, then I will follow the procedure outlined in #982.

Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
Signed-off-by: Matt Bagley <firstlife22@gmail.com>
…BODY.

- Also left note about 'summary' arg, but decided not to include it.

Signed-off-by: Matt Bagley <firstlife22@gmail.com>
@bagley
Copy link
Contributor Author

bagley commented Jan 24, 2018

I put a check for dokuwiki's cookies in a lot of the rules. That's probably not needed, and may be slowing it down. Taking those out will shorten the rules/chains. Let me know if those should be taken out.

@bagley
Copy link
Contributor Author

bagley commented Jan 24, 2018

On one rule, I turn off requestBodyAccess if it's a large upload (only on the php file that does the uploading). Is that a good thing to do? Or should that be left to the user's decision?

SecRule REQUEST_FILENAME "@endsWith /lib/exe/ajax.php"\
    "id:9011120,\
...
    SecRule REQUEST_HEADERS:Content-Length "@gt 31486341" \
        "t:none,\
        ctl:requestBodyAccess=Off"

nolog,\
noauditlog,\
chain"
SecRule REQUEST_HEADERS:Content-Length "@gt 31486341" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you choosing this @gt number for any particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Not anything definite beyond just seeming like a good balance for performance. But starting to think I should just comment it out at the top, for the config, if at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually not referring to removing that rule, but from my experience even running CRS on anything below 32MB seems rather problematic and can cause FPs and long latency/high CPU. But having the default which the user should set anyway should work fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's what I tried before, setting the 'crs_exclusions_dokuwiki_scan_upload=31486341' variable that would respond to a rule setting the upload scan limit, but I couldn't get it to work. It was as if it was being ignored. Is that what you are referring to? I don't think it would be good to hard code it in, as if someone needed to modify it, they'd have to do so each time it was updated.

Or just set crs_exclusions_dokuwiki_scan_upload=0|1 for if uploaded files are scanned or not. And then a simple rule to turn off scanning for uploads.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option C: Put a note at the top that the user can add to crs-setup.conf or website config.

# If you want to relax the upload restrictions of file types,
# see rule 900240. For Dokuwiki you can limit the exception
# to the ajax.php file:
#
# SecRule REQUEST_FILENAME "@endsWith /lib/exe/ajax.php"\
#    "id:9011120,\
#    phase:1,\
#    t:none,\
#    nolog,\
#    pass,\
#    chain"
#    SecRule REQUEST_COOKIES:/S?DW[a-f0-9]+/ '@rx ^[%a-zA-Z0-9_-]+' \
#      "t:none,\
#      tx.restricted_extensions='.bak/ .config/ .conf/ .db/ .ini/ .log/ .old/ .pass/ .pdb/ .sql/'"
#
# If you have have performance/latency issues on large uploads, 
# you can try disabling scanning of uploaded files (may open security
# holes). Here's an example for crs-setup.conf or your website's config 
# that will only make the exception for uploads (ajax.php)
#
# SecRule REQUEST_FILENAME "@endsWith /lib/exe/ajax.php"\
#    "id:9011121,\
#    phase:1,\
#    t:none,\
#    pass,\
#    nolog,\
#    noauditlog,\
#    chain"
#    SecRule REQUEST_HEADERS:Content-Length "@gt 31486341" \
#        "t:none,\
#        ctl:requestBodyAccess=Off"
#
# And to increase the upload size for only uploaded files (512MB):
#
# SecRule REQUEST_FILENAME "@endsWith /lib/exe/ajax.php"\
#    "id:9011122,\
#    phase:1,\
#    t:none,\
#    pass,\
#    nolog,\
#    noauditlog,\
#    ctl:requestBodyLimit=536870912"

Signed-off-by: Matt Bagley <firstlife22@gmail.com>
@dune73
Copy link
Contributor

dune73 commented Feb 5, 2018

Merging this and moving to fixup afterwards. Will also settle open questions from above.

@dune73 dune73 merged commit b0feead into SpiderLabs:v3.1/dev Feb 5, 2018
@bagley
Copy link
Contributor Author

bagley commented Feb 5, 2018

Thanks for merging this. I also took a look at #1011, tested it, and it worked great.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants