Adds common and uniform http headers to tests #2362

fzipi · 2022-01-24T23:19:04Z

This is a heavy to review patch. Fixes #2344

I've tried to split commits one per directory, so they can be reviewed independently.
There are lots of formatting changes, but I think they will be good afterwards.

airween · 2022-01-25T17:03:06Z

There were some failed test, so I have collected the reasons.

The list of the errors:
https://github.com/coreruleset/coreruleset/runs/4929701146?check_suite_focus=true#step:4:2837

The cases:

913100

I'm not sure we can change the User-Agent header, because that's the meaning of the rule, eg:

https://github.com/coreruleset/coreruleset/pull/2362/files#diff-70624f5dbe4122947942480691f9b72512dd30dabd9166dc05e0db5ec5e72800R22

but the original was:
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-70624f5dbe4122947942480691f9b72512dd30dabd9166dc05e0db5ec5e72800L24-L25

List of patterns of User-Agent here:
https://github.com/coreruleset/coreruleset/blob/v3.4/dev/rules/scanners-user-agents.data#L86

920490

rule checks x-up-devcap-post-charset; if it exists, then checks the User-Agent, which has to contains string 'up'. The new one doesn't contains.
the old one:
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-e46cb93e7c78522861930602c6c6e6f5b67c7fc835bd805847e667479081cd93L46

the new:
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-e46cb93e7c78522861930602c6c6e6f5b67c7fc835bd805847e667479081cd93R16

931130

this rules checks the arguments with an URI-like schema; if it matches, checks compares the Host header with a transaction variable, so I think we can't change it

the old tests:
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-baf121d9cfd54321b35a246c96295de823021919a0892fa144e01ea95026a657L154
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-baf121d9cfd54321b35a246c96295de823021919a0892fa144e01ea95026a657L188

the new ones:
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-baf121d9cfd54321b35a246c96295de823021919a0892fa144e01ea95026a657R144
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-baf121d9cfd54321b35a246c96295de823021919a0892fa144e01ea95026a657R176

941100

this is an XSS rule, some part of the request must have contains the attack
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-e27ff48fe39e5d0486a366f191daac5352729810270ee84ef44e6024df474e11L52
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-e27ff48fe39e5d0486a366f191daac5352729810270ee84ef44e6024df474e11R48

941110

same as 941100
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-481b31a2307b10c6a65c37181b76163eab6cc559b6e1195d13129611dfa48a31L53
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-481b31a2307b10c6a65c37181b76163eab6cc559b6e1195d13129611dfa48a31R49

941160-8

this is a "NoScript InjectionChecker" rule, the affected header is the User-Agent - we can't change the old one:
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-fdbcc26676eab1de35a89bc6a3e0e930bdbf6c976396570d61c883543b095f48L132
https://github.com/coreruleset/coreruleset/pull/2362/files#diff-fdbcc26676eab1de35a89bc6a3e0e930bdbf6c976396570d61c883543b095f48R123

If you need any other help, just let me know.

Btw: why do we want to use ModSecurity CRS 3 Tests as User-Agent? Would be enough that just CRS 3 Tests?

fzipi · 2022-01-25T17:24:03Z

Hey @airween , thanks for the review. Was doing it now, it was taking a bit because I'm rebasing on old commits.

fzipi · 2022-01-25T17:37:01Z

I second the CRS tests user agent change, but also, will it depend on major version? E.g. when CRS 4.x is out, are we changing that?

airween · 2022-01-25T17:41:51Z

I second the CRS tests user agent change, but also, will it depend on major version? E.g. when CRS 4.x is out, are we changing that?

Hmmm... good question.

A crazy idea, but can we detach the test set from the rule set? I mean now you've started to add/modify the headers for the better consistency, so we can say this is a new version of our test set.

May be we can use some new format, like:

CRS Test v1.1, for version 3.4.0-dev

And if we release the 4.0, the we replace the version at the end.

fzipi · 2022-01-25T18:28:28Z

All tests passing now.

airween · 2022-01-25T19:27:45Z

All tests passing now.

Let me check this PR in next few days.

10000

dune73 · 2022-01-26T07:33:55Z

I'd hate to do separate versioning for the tests. This is making things so complicated. In fact I do not think we need any version in the test UA. Tests are always run in a controlled environment so when something needs debugging you can easily find out the test that triggered. Having the test-ID submitted as part of the test would be more interested (unless we have that already :)

What I would suggest, though, would be to add the CRS test suite user agent string to any existing attacking test string we have already. Thus libwww-perl; OWASP ModSecurity Core Rule Set Test.

airween · 2022-01-31T20:37:56Z

I've reviewed the modified tests, and most of them are done.

There you have all the converted tests and no new tests.

These issues are what I've found:

920171-1

this test contains an encoded_request in base64 format, which includes the necessary headers. I think we should left the headers from here and change those values in the base64 encoded string.

920171-2

this test contains an encoded_request in base64 format, which includes the necessary headers. I think we should left the headers from here and change those values in the base64 encoded string.

920171-3

this test contains an encoded_request in base64 format, which includes the necessary headers. I think we should left the headers from here and change those values in the base64 encoded string.

920300-1

wrong comment, the correct should be "test needs a missing 'Accept' header"

920300-2

wrong comment, the correct should be "test needs a missing 'Accept' header"

920300-3

wrong comment, the correct should be "test needs a missing 'Accept' header"

920300-4

'User-Agent' not uniform, should be "ModSecurity CRS 3 Tests AppleWebKit/537.36"
wrong comment, the correct should be "test needs a missing 'Accept' header"

920310-5

'User-Agent' not uniform, should be "ModSecurity CRS 3 Tests Business/6.6.1.2"

920310-6

'User-Agent' not uniform, should be "ModSecurity CRS 3 Tests Entreprise/6.5.0.177"

944200-1

this test contains an encoded_request in base64 format, which includes the necessary headers in right form. I think we can remove the necessary headers from here.

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fzipi · 2022-02-03T12:33:58Z

Thanks @airween for the thorough review!

Included all your comments. This should be ready now.... unless you want to change all user agents to OWASP ModSecurity Core Rule Set Test and that is a PITN.

airween · 2022-02-03T12:44:54Z

Included all your comments.

great, thanks!

This should be ready now.... unless you want to change all user agents to OWASP ModSecurity Core Rule Set Test and that is a PITN.

I don't know, what would be the best solution - I wrote my suggestions above based on @dune73 's comment:

What I would suggest, though, would be to add the CRS test suite user agent string to any existing attacking test string we have already. Thus libwww-perl; OWASP ModSecurity Core Rule Set Test.

More thoughts about tests: I think it would be good to extend the list of skeleton tests, eg. add skeleton for encoding_request. Also we should fix these uniform headers in those skeletons. A small documentation also is missing now.

(I can add these after the merge, if you want to apply this patch.)

Ideas?

fzipi · 2022-02-03T13:38:24Z

To be honest, I think this is massive and don't want to hold it too much. Drift is happening on every merge, and I was careful to create commits separated, so it is a pain to update.

@dune73 @airween I would do this:

merge this one.
if we want to change the user agent (which I think makes sense), let's go for a new issue and PR
new issue and PR for the skeleton tests

airween · 2022-02-03T14:09:14Z

LGTM!

dune73 · 2022-02-07T14:43:03Z

Agreed on the plan.

And thank you for this huge PR @fzipi and the timely review @airween.

fzipi force-pushed the add-common-http-headers-2344 branch 2 times, most recently from d9e86dc to 7b385bc Compare January 25, 2022 18:11

airween assigned airween and fzipi Jan 25, 2022

fzipi added 13 commits February 3, 2022 12:24

fix(test): add common headers

5fac474

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 913

b34652a

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 920

7d04a43

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 921

6b15a53

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 930

0915550

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 931

57be277

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 932

a9917fd

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 933

19017c7

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 934

b70446d

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 941

72143c7

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 942

13bed77

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 943

6e8a13d

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fix(test): add common headers - 944

78fdff8

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

fzipi force-pushed the add-common-http-headers-2344 branch from 7b385bc to 78fdff8 Compare February 3, 2022 12:31

fzipi merged commit 5c4684d into coreruleset:v3.4/dev Feb 3, 2022

fzipi deleted the add-common-http-headers-2344 branch February 3, 2022 14:13

This was referenced Feb 3, 2022

Update test User-Agent to "OWASP ModSecurity Core Rule Set Test" #2368

Closed

Monthly Chat Agenda February 2022 (2022-02-07 and 2022-02-21) Meeting Agenda #2350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Adds common and uniform http headers to tests #2362

Adds common and uniform http headers to tests #2362

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Adds common and uniform http headers to tests #2362

Adds common and uniform http headers to tests #2362

Uh oh!

Conversation

Uh oh!

Uh oh!

The cases:

913100

913101

920280

920290

920300

920310

920311

920320

920330

920350

920490

931130

941100

941110

941160-8

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!