Fix 933160-21 and 942500-1 tests broken due to invalid URIs

[nullpo-head]

Hi CRS team. Thanks for the great product!
Here is a pull request to fix broken tests on Nginx. Let me know if it's better to open an issue first.

Issue

The regression test cases, 933160-21 and 942500-1, are failing on Nginx.

Reason

The uri parameters in tests cases 933160-21 and 942500-1 start with ?param= without the actual path such as /. This is an ill-formed path in terms of the HTTP protocol spec, because it lacks the absoluteURI, abs_path nor authority. (Link to the spec fyi.)
Thus, the generated requests are rejected by Nginx before WAF sees them, which is not an expected behavior.
JFYI, this happens on Envoy server as well.

Logs

Before my fix, Nginx rejects the requests by the test cases with the following log.

2021/07/27 11:47:43 [info] 23#23: *4 client sent invalid request while reading client request line, client: 192.168.208.1, server: localhost, request: "POST ?x=eval%28chr%28112%29.chr%28104%29.chr%28112%29 HTTP/1.1"

With my fix, the test cases pass.

[fzipi]

Hi @nullpo-head !

Thanks for your PR, will check it soon. Just remember sometimes we are precisely trying to break specs/rfcs :)

[fzipi]

Hi @nullpo-head !

Thanks for your PR, will check it soon. Just remember sometimes we are precisely trying to break specs/rfcs :)

[nullpo-head]

Thanks for your so quick response, fzipi!

Just remember sometimes we are precisely trying to break specs/rfcs :)

As for these tests, they are not intentionally violating RFCs. They are a test for php function and SQLi respectively, and what they want to test are the values of their parameters, not an invalid HTTP path request. Thanks for the note!

[dune73]

@airween : You are usually keeping an eye on broken NGINX tests. This PR seems to fix something that is quite obviously broken. Could you take a look, please?

[airween]

Sure, I'll check these soon.

[franbuehler]

Meeting decision August 2021: @airween will review this PR.

[nullpo-head]

This PR seems to fix something that is quite obviously broken

Yes, it does, but I'm curious what your expectations are for Nginx's tests: it sounds like you're expecting all tests to pass on Nginx. However, in fact, 126 tests fail on Nginx (modec3-nginx), and this PR only fixes 2 of them. Do you normally expect all tests to pass on Nginx?

IIUC, to fix the all failing tests,

1. The CRS regression test has to introduce test skipping mechanism in order to skip Apache-specific tests
2. Rewrite CRS rules that use regular expressions that match Apache-specific UTF-8 character escapes.
3. Rewrite CRS rules that use REQUEST_BODY, which don't work well with XML/JSON on ModSec3

[fzipi]

Thanks for you comments. We know that some tests fail in nginx because of problems in the connector, and disimilar behavior patterns.

My idea was to use the feature available in go-ftw to skip well known failing tests, once we are happy with the actual testing pipeline and we declare it stable.

[dune73]

The idea is clearly for ModSec3 to pass all the tests. We know that the engine / NGINX platform has shortcomings and we keep a close eye on those. Your PR fixes things where our close eye did not see sharp enough. That's why I called @airween to join us here.

It's unfortunate that ModSec3 is lacking several years after they declared it production ready, but there is relatively little we can do outside of providing patches (what we've done in the past). The very thing we won't be doing is making CRS weaker in order to make ModSec3 pass 100% of the tests. CRS is meant to be the full functionality.

[airween]

Hi @nullpo-head,

thanks again for your pull request. I did some research, now please let me summarize my ideas.

I've reviewed the RFC, and as I interpret, it allows the empty URI - but this is just my opinion, please check it someone else as well.

Even so I think in these cases, when the request can be adjusted for more valid test results, we can change it. There is only one small note from me: the first test's rule checks the REQUEST_FILENAME|ARGS_NAMES|ARGS, and the test sends the attack in ARGS_NAMES, not the REQUEST_FILENAME (in the current state it's an empty string, with your patch is a /). That's why I can say we should accept this PR.

My opinions for your other observations:

However, in fact, 126 tests fail on Nginx (modec3-nginx), and this PR only fixes 2 of them. Do you normally expect all tests to pass on Nginx?

you are right, but I'm not sure anybody will be more happier if there will be two less faulty test from the 126 :).

The CRS regression test has to introduce test skipping mechanism in order to skip Apache-specific tests

it's a good idea, and as @fzipi wrote above, our new regression test framework will know this feature

Rewrite CRS rules that use regular expressions that match Apache-specific UTF-8 character escapes.

sorry, could you help me to clarify which "Apache-specific UTF-8 character escapes" are you thinking about?

Rewrite CRS rules that use REQUEST_BODY, which don't work well with XML/JSON on ModSec3

Unfortunately I know what are you feeling - but sorry to say, this is a known libmodsecurity3 bug. There are two separated issues, we really hope that the developers will fix them soon.

Please note, that the reference shows: This variable is available only if the URLENCODED request body processor was used, which will occur by default when the application/x-www-form-urlencoded content type is detected, or if the use of the URLENCODED request body parser was forced.

Please accept our viewpoint, that we need a reference system - not only for the mod_security2 (and Apache) users, but all of our customers.

Summed up so far: we can merge this PR without any risk.

[nullpo-head]

@fzipi

My idea was to use the feature available in go-ftw to skip well known failing tests, once we are happy with the actual testing pipeline and we declare it stable.

I didn't realize that there's go version. Faster and supporting more features. It sounds awesome! I feel it may be even nicer if we could maintain testoverride per test file basis. My PR, which dune already mentioned you, might help, though this would be out of the scope of this PR of fixing failing tests.

[nullpo-head]

The idea is clearly for ModSec3 to pass all the tests

Thanks for clarification, @dune73! It sounds awesome.

The very thing we won't be doing is making CRS weaker in order to make ModSec3 pass 100% of the tests. CRS is meant to be the full functionality.

Yes, I understand it and I didn't mean it, either. I just wondered if the status of Nginx tests is tracked, but it seems that was a completely unnecessary concern. Thanks for your great work!

[nullpo-head]

I really appreciate the detailed discussion, @airween.

I've reviewed the RFC, and as I interpret, it allows the empty URI

we should accept this PR.

Thanks! It's of course OK to merge this PR anytime. As for the RFC, thanks for taking your time to check it and I'm sorry if I was wrong. It seems that another section says abs_path in a request cannot be empty, but I'm not a HTTP lawyer and that would be a bit off-topic, so no problem.

sorry, could you help me to clarify which "Apache-specific UTF-8 character escapes" are you thinking about?

I'm talking about the regular expression of the rule 920460 and 941330, which seems to have too-many backslashes. It seems to depend on the fact that Apaches do escaping like \\xffff or \\uffff, instead of \xffff or \uffff I'll open another issue on that if I confirm that.

this is a known libmodsecurity3 bug

Oops, I didn't notice that the guy in the discussion was you. Thanks so much for your effort on that.

[airween]

hi @nullpo-head,

I really appreciate the detailed discussion, @airween.

thanks :)

Thanks! It's of course OK to merge this PR anytime. As for the RFC, thanks for taking your time to check it and I'm sorry if I was wrong. It seems that another section says abs_path in a request cannot be empty, but I'm not a HTTP lawyer and that would be a bit off-topic, so no problem.

sure, that's why I asked someone to review that part. No problem.

I'm talking about the regular expression of the rule 920460 and 941330, which seems to have too-many backslashes. It seems to depend on the fact that Apaches do escaping like \\xffff or \\uffff, instead of \xffff or \uffff I'll open another issue on that if I confirm that.

oh, I see - sorry to say, but this is also a know libmodsecurity3 bug (if I understand you correctly). Here is a small summary, how the escape char handling works.

Oops, I didn't notice that the guy in the discussion was you. Thanks so much for your effort on that.

no problem :). I've sent a very ugly PR to fix that, but I was just curious - that's not a real fix. Rather I devoted it to start a discussion.

[dune73]

Hey guys, I only skimmed through your conversation above. Can you give me a status report and a confirmation this PR is ready to be merged?

[nullpo-head]

@dune73 Hi, we were discussing another topic, but I think the conclusion of this PR itself is this PR can be merged.

[fzipi]

Agreed, merging 😄