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

Improve request Content-Type checks #1103

Merged
merged 8 commits into from Jul 31, 2018

Conversation

Projects
None yet
3 participants
@lifeforms
Copy link
Collaborator

lifeforms commented May 27, 2018

This PR contains two changes to request Content-Type header checking:

Changed: 920420 improve policy checks

Rule 920420 omitted to check the mime-type in the Content-Type request header in case of GET, HEAD, PROPFIND or OPTIONS requests.

These types of requests should not have a Content-Type header, since they have no body, but that is no reason to not check the mime-type, if one should be submitted.

Note that this rule only works on properly formatted Content-Type headers, from which a mime-type can be parsed.

Added: 920470 whitelist header format

Since rule 920420 only works on well-behaved Content-Type headers, I've inserted a new rule above it, which first validates Content-Type against a regular expression.

This provides generic whitelist protection against vulnerabilities like Apache Struts Content-Type arbitrary command execution (CVE-2017-5638), which uses a Content-Type header like:

Content-Type: %{(#nike='multipart/form-data').(#dm=@ognl ...

For the Struts vuln, we have blacklisting in the 944xxx rules, but I feel the Content-Type header is a good candidate for whitelisting in addition to this.

The rule might be a little pedantic, but I've tried to be flexible towards possible practical uses. I've tested it out on some traffic. But it's possible that we'll discover from testing that we need to be a little more lax.

@lifeforms lifeforms added this to the CRS v3.1.0 milestone May 27, 2018

@lifeforms lifeforms changed the title Check Content-Type header on all requests Improve Content-Type header checks May 27, 2018

@lifeforms lifeforms changed the title Improve Content-Type header checks Improve request Content-Type checks May 27, 2018

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jun 2, 2018

This is a very useful addition. We ought to test this some more (no time this morning here, though).

Being in a pedantic mood, I do not like the introduction of in-between id numbers like xxxx25. The rules are related, but we do not mirror this unless they are strict siblings which is not the case here.
So I think you should bring this new rule with the new id 920470.

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jun 2, 2018

Good suggestion, I'll take care of it.

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jun 4, 2018

Thanks.

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 7, 2018

Did you run the additional tests @dune73 ?

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jul 9, 2018

Some more, but probably not enough.

Given we are currently working on this, maybe we want to cover issue #1137 with this PR too.

@lifeforms: I think you have been in the said talk at AppSecEU too.

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jul 9, 2018

Encoding check would be a separate rule, so in the interest of keeping PRs tractable, I'd rather have this accepted and then start work on that.

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jul 9, 2018

ACK

Question: Am I supposed to test this extensively, or is somebody else doing that? Or do we call it a day and merge?

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 10, 2018

I don't know but if we're still working on it it should be pushing to v3.2 - perhaps it'd be best to test what we have now and save the rest for v3.2

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jul 10, 2018

We're not really working on this. @lifeforms stated he would rather merge this and do additional changes separately. So this is only waiting for some more testing.

I have ran a few additional tests triggering 920470, 920420 and activating 900220. This looks good. Ready to be merged (and already assigned to you @csanders-git; so I let you do the merge).

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 12, 2018

@lifeforms @dune73 I think there is a conflict from the hybrid mode, can we resolve these to ensure we don't do anything bad :-)

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jul 12, 2018

Conflict resolved. And tested for functionality again. @lifeforms: Could you check it too before merge, please?

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jul 12, 2018

I will check and add tests.

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 15, 2018

any update @lifeforms ? I'd like to get this finished so we can merge the other elements.

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jul 15, 2018

@csanders-git It's my top priority!

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 31, 2018

@lifeforms any update here? we may be forced to merge without tests if we can't get this by the weekend.

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jul 31, 2018

@csanders-git Will update the PR by the end of the afternoon.

lifeforms added some commits Jul 31, 2018

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jul 31, 2018

Tests added.

Note that 920* tests are not run yet by Travis, however the tests run well here:

# crs test 920420
.
1 passed in 0.28 seconds
Running tests for rule 920420...
==================================== 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 9 items 

util/regression-tests/CRS_Tests.py::test_crs[ruleset0-920420.yaml -- 920420-1] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset1-920420.yaml -- 920420-2] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2-920420.yaml -- 920420-3] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset3-920420.yaml -- 920420-4] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset4-920420.yaml -- 920420-5] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset5-920420.yaml -- 920420-6] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset6-920420.yaml -- 920420-7] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset7-920420.yaml -- 920420-8] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset8-920420.yaml -- 920420-9] PASSED

================================= 9 passed in 2.82 seconds ==================================

# crs test 920470
.
1 passed in 0.36 seconds
Running tests for rule 920470...
==================================== 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 8 items 

util/regression-tests/CRS_Tests.py::test_crs[ruleset0-920470.yaml -- 920470-1] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset1-920470.yaml -- 920470-2] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2-920470.yaml -- 920470-3] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset3-920470.yaml -- 920470-4] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset4-920470.yaml -- 920470-5] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset5-920470.yaml -- 920470-6] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset6-920470.yaml -- 920470-7] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset7-920470.yaml -- 920470-8] PASSED

================================= 8 passed in 2.50 seconds ==================================
@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 31, 2018

Perfect, great job -- just confirmed these.

@csanders-git csanders-git merged commit cd407cb into SpiderLabs:v3.1/dev Jul 31, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment