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

New rule 920480 to check content type charset via tx.allowed_..._charset #1143

Merged
merged 7 commits into from Aug 7, 2018

Conversation

Projects
None yet
5 participants
@dune73
Copy link
Collaborator

dune73 commented Jul 12, 2018

See #1137 for complete reasoning.

Also: I took to liberty to erase trailing spaces in crs-setup.conf.example.

chain"
SecRule TX:1 "!@rx ^%{tx.allowed_request_content_type_charset}$" \
"t:none,\
ctl:forceRequestBodyVariable=On,\

This comment has been minimized.

@csanders-git

csanders-git Jul 12, 2018

Collaborator

@dune73 why do we have this ctl:forceRequestBodyVariable here?

This comment has been minimized.

@dune73

dune73 Jul 12, 2018

Collaborator

I've taken this from 920420. I assumed it was mean to log the request body, so it could later be inspected.

This comment has been minimized.

@mirkodziadzka-avi

mirkodziadzka-avi Jul 26, 2018

Without having checked the code completely, but if I remember correctly this is a none-op in v3. Together with ctl:auditEngine, drop and expirevar

This comment has been minimized.

@dune73

dune73 Jul 26, 2018

Collaborator

Thank you for the hint. Do you mean the ctl:forceRequestBodyVariable=On is a none-op?

This comment has been minimized.

@mirkodziadzka-avi

mirkodziadzka-avi Jul 27, 2018

@dune73 As far as I understand the v3 code, yes.

The parser maps this action in line https://github.com/SpiderLabs/ModSecurity/blob/eed6b5f86d00ce7d6bf5e14d6f03647bafd6251d/src/parser/seclang-parser.yy#L2560 to a generic actions::Action object instead of a specific action subclass. This generic actions::Action evaluates to true in line https://github.com/SpiderLabs/ModSecurity/blob/eed6b5f86d00ce7d6bf5e14d6f03647bafd6251d/src/actions/action.cc#L55 without doing something. If I don't miss anything here, this action does nothing.

It's the same for the actions drop, expirevar an ctl:auditEngine.

Maybe it's worth to open a ticket on ModSecurity v3 and ask for clarification about the indented semantic of ctl:forceRequestBodyVariable action. Because I also do not find a description in the reference manual either.

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jul 21, 2018

Added iso-8859-1 to the default charsets on request of @emphazer.

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 31, 2018

@dune73 this is read for merge now I think yes?

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jul 31, 2018

I will review on Thursday. We might need iso-8859-15 (with EURO!) ;) And I'll add tests.

@lifeforms lifeforms self-assigned this Jul 31, 2018

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Aug 1, 2018

Thanks @lifeforms. Adding iso-8859-15 now.

Open question: Do we drop ctl:forceRequestBodyVariable=On from the rule, because it's not really needed and because v3 does not implement it anyways?

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Aug 1, 2018

@dune73 About ctl:forceRequestBodyVariable=On, it depends on the plans of ModSec v3. It could be a bug or omission, or it could be a deliberate deprecation. I don't know. Let's keep this action for now. We use it also in another place. And it has merit in ModSec v2. Should v3 totally phase out ctl:forceRequestBodyVariable, and v3 will overtake v2 in usage, then we might consider cleaning all instances up from our codebase in a future PR.

@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Aug 1, 2018

Do you know how v3 handles this? in v2 this could lead to a big time WAF bypass

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Aug 1, 2018

I agree with you @lifeforms.

@spartantri: What are you referring to? ctl:forceRequestBodyVariable=On?

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Aug 4, 2018

I've added windows-1252 to the default allowed charsets. It's equivalent to iso-8859-1. I found in my logs that many Windows clients send this when posting XML using XHR. Could be application dependent.

I've also added tests for the rule. Some are failing:

# crs test 920480
.
1 passed in 0.47 seconds
Running tests for rule 920480...
================================== test session starts ==================================
platform linux2 -- Python 2.7.15rc1, pytest-2.9.1, py-1.5.4, pluggy-0.3.1 -- /usr/bin/python
cachedir: util/regression-tests/.cache
rootdir: /crs/util/regression-tests, inifile: 
plugins: ftw-1.1.0
collected 13 items 

util/regression-tests/CRS_Tests.py::test_crs[ruleset0-920480.yaml -- 920480-1] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset1-920480.yaml -- 920480-2] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2-920480.yaml -- 920480-3] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset3-920480.yaml -- 920480-4] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset4-920480.yaml -- 920480-5] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset5-920480.yaml -- 920480-6] FAILED
util/regression-tests/CRS_Tests.py::test_crs[ruleset6-920480.yaml -- 920480-7] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset7-920480.yaml -- 920480-8] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset8-920480.yaml -- 920480-9] FAILED
util/regression-tests/CRS_Tests.py::test_crs[ruleset9-920480.yaml -- 920480-10] FAILED
util/regression-tests/CRS_Tests.py::test_crs[ruleset10-920480.yaml -- 920480-11] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset11-920480.yaml -- 920480-12] FAILED
util/regression-tests/CRS_Tests.py::test_crs[ruleset12-920480.yaml -- 920480-13] FAILED

========================== 5 failed, 8 passed in 3.22 seconds ===========================

About the failing tests:

  • 920480-6 sends application/x-www-form-urlencoded; charset=UTF-80. This charset has garbage after the allowed UTF-8. This should definitely be rejected. Hopefully the rule can be improved to reject this.
  • 920480-9 sends application/x-www-form-urlencoded; charset=ibm037. This charset is not on the default whitelist. This should be rejected. To be honest, I have no idea why this test is failing. It's almost the same as 920480-11 except that one has charset=ibm038. And if I do curl -H 'Content-Type: application/x-www-form-urlencoded; charset=ibm037' http://localhost/ the request IS blocked. Something strange seems to be happening on FTW or webserver level. Maybe @csanders-git could take a look at this failing test?
  • 920480-10 is the same as 920480-9, just without space in front of charset.
  • 920480-12 sends two charsets, the second is forbidden. Sending multiple charsets could trick a request parser further downstream into using a forbidden one, and should be rejected IMHO.
  • 920480-13 sends two charsets, the first is forbidden. Sending multiple charsets could trick a request parser further downstream into using a forbidden one, and should be rejected IMHO.
@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Aug 4, 2018

Thanks for all these tests. The advanced ones that fail are known failures. We discussed this in #1137.
I can not see to avoid these bypasses for semantic reasons and I think we need to look into whitelisting for 3.2.

If you see a solution for a rule that passes xxx-6, xxx-12 and xxx-13, then I'd be happy to update.

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Aug 6, 2018

Given this information, I think we're ready to merge .. Confirmation will be tmrw.

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Aug 6, 2018

Commented out the non passing tests: either out of scope for 3.1, or due to FTW weirdness that we will look at later.

@csanders-git csanders-git merged commit a6a87e6 into SpiderLabs:v3.1/dev Aug 7, 2018

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

dune73 commented Aug 7, 2018

Thank you for merging.

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Aug 7, 2018

And thanks for adding the tests @lifeforms. Most welcome.

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