Skip to content
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

feat: new rule 920620 #3237

Merged
merged 5 commits into from Jun 12, 2023
Merged

Conversation

theMiddleBlue
Copy link
Contributor

this PR adds a detection rule for private security issue C9K-230327

@theMiddleBlue theMiddleBlue marked this pull request as draft June 8, 2023 15:58
@dune73
Copy link
Member

dune73 commented Jun 8, 2023

Can you add a comment about the different behavior of various implementations / webservers in the light of multiple CT headers.

Why is this still a draft?

@theMiddleBlue
Copy link
Contributor Author

Can you add a comment about the different behavior of various implementations / webservers in the light of multiple CT headers.

sure!

Why is this still a draft?

I'm trying to understand how to test it (same key name in yaml for the header list seems to be a problem)

@dune73
Copy link
Member

dune73 commented Jun 12, 2023

Got you. Makes sense. It's tricky. Ultimately, we might have to go without a test for the time being. After all, the rule is very clear and easy to understand. It's more like most webserver do not even expose the weakness to ModSec.

@RedXanadu
Copy link
Member

RedXanadu commented Jun 12, 2023

@theMiddleBlue Until Go-FTW / the test parser supports multiple identical headers (whichever one is causing the issue - it looks like maybe Go-FTW just doesn't understand the YAML if you have the same header twice?), if you want to avoid doing Content-Type: "application/json\nContent-Type: application/foo", this test also works as an encoded request.

I got it working with this as a 'test' test:

  - test_title: 920620-2
    desc: Multiple Content-Type request headers
    stages:
      - stage:
          input:
            dest_addr: "127.0.0.1"
            port: 80
            encoded_request: "R0VUIC8gSFRUUC8xLjENCkhvc3Q6IGxvY2FsaG9zdA0KVXNlci1BZ2VudDogVEVTVCBDUlMNCkFjY2VwdDogKi8qDQpDb250ZW50LVR5cGU6IGFwcGxpY2F0aW9uL2pzb24NCkNvbnRlbnQtVHlwZTogYXBwbGljYXRpb24veG1sDQoNCg=="
          output:
            log_contains: "id \"920620\""

where R0VUIC8… is the Base64 encoded equivalent of:

GET / HTTP/1.1
Host: localhost
User-Agent: TEST CRS
Accept: */*
Content-Type: application/json
Content-Type: application/xml

(with 0D 0A line endings)

It doesn't work against Apache, though, as Apache seems to combine the headers into one, which triggers many of our other content type header rules anyway (I think this was already discussed a few weeks ago, anyway):

...[id "920470"] [msg "Illegal Content-Type header"] [data "application/json, application/xml"]...

But the test works against Nginx 🙂

@theMiddleBlue
Copy link
Contributor Author

thx @RedXanadu
idk if it worth to have a "if-not-webserver" directive to ignore the test on Apache
otherwise we can comment this test and wait for future solutions

@RedXanadu
Copy link
Member

RedXanadu commented Jun 12, 2023

Sorry @theMiddleBlue, I should have made that clearer: the example was just a very quick test 😅

Here's one which uses the correct CRS testing user agent, accept header, endpoint, etc., so should be ok to use for real:

R0VUIC9nZXQgSFRUUC8xLjENCkhvc3Q6IGxvY2FsaG9zdA0KVXNlci1BZ2VudDogT1dBU1AgQ1JTIHRlc3QgYWdlbnQNCkFjY2VwdDogdGV4dC94bWwsYXBwbGljYXRpb24veG1sLGFwcGxpY2F0aW9uL3hodG1sK3htbCx0ZXh0L2h0bWw7cT0wLjksdGV4dC9wbGFpbjtxPTAuOCxpbWFnZS9wbmcsKi8qO3E9MC41DQpDb250ZW50LVR5cGU6IGFwcGxpY2F0aW9uL2pzb24NCkNvbnRlbnQtVHlwZTogYXBwbGljYXRpb24veG1sDQoNCg==

<=>

GET /get HTTP/1.1
Host: localhost
User-Agent: OWASP CRS test agent
Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5
Content-Type: application/json
Content-Type: application/xml

You could set enabled: false at the top of the test file to skip the test. I agree it would be good to have conditional skipping per-webserver as a feature.

@theMiddleBlue
Copy link
Contributor Author

thanks @RedXanadu
done 👍🏻

@theMiddleBlue theMiddleBlue marked this pull request as ready for review June 12, 2023 15:26
Copy link
Member

@RedXanadu RedXanadu left a comment

Choose a reason for hiding this comment

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

Re-tested against the modsec3-nginx container. New rule works as expected 😄 curl request with multiple CT headers detected. Great job!

@dune73
Copy link
Member

dune73 commented Jun 12, 2023

Glad this is fixed. Thank you guys.

@dune73 dune73 merged commit b501f1a into coreruleset:v4.0/dev Jun 12, 2023
4 checks passed
@RedXanadu RedXanadu mentioned this pull request Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants