Conflict between missing/empty host CRS tests and setting "Host" header from dest_addr override

[M4tteoP]

Hi!
I'm debugging CRS tests 920280-1 and 920290-1 that are failing against Coraza. It seems that there is a conflict between these specific tests (They are trying to send a request without a Host header or with an empty one) and the feature added in feat: set "Host" header from dest_addr override.
When a destination address override is provided, the feature replaces the empty host field with the destination address making the request a valid one.

It is not happening against Apache-Modsec (and CRS CI) because I think that destination overriding is not used there. A small ftw config like the following reproduces the issue:

logfile: '/Users/Repo/coreruleset/tests/logs/modsec2-apache/error.log'
testoverride:
  input:
    dest_addr: localhost

I may be missing something about #63, but I'm wondering why the host header is replaced only when == ""? I was expecting that this feature was mainly intended for replacing all the Host: "localhost" spread across the rules

I gave it a little go (just replacing == with !=, see this branch): 920280-1 and 920290-1 would be fixed, but ["920270-4" "920350-1" "920350-3" "920350-4" "920350-5" "920350-6" "931130-9" "931130-11" "931130-15" "980170-2"] start to fail.
It happens because, for example in test 920270-4, a manipulated host is replaced by a legit one (localhost%00 -> localhost).

Thanks for any input about it!

[jcchavezs]

Ping @theseion

[theseion]

You're right, running those tests with a dest_addr override will make them fail, since those rules check for missing and empty Host header. It is also true that we never run the test suite with any overrides.

I think what's missing is that the headers should also be applied from the overrides. But that's probably a different issue.

The reason we only set the Host header if it's missing is that under normal circumstances, the dest_addr override should not change the Host header. In the related bug report, however, the reporter said that applying the dest_addr didn't work for them in the case where the Host header was missing, because they were testing against cloud providers and their ingress inspects the Host header (#60). If the Host header has been set in a test, it shouldn't be touched by dest_addr.

I suspect that they are using go-ftw with their own test suite, not with CRS, which is why this issue never came up before.

[theseion]

One way to fix this issue would be to add a flag for toggling this behaviour, e.g.,

---
testoverride:
  input:
    dest_addr: "10.10.1.2"
    override_empty_host_header: false

[M4tteoP]

Hi @theseion, thanks! I was misinterpreting a bit the feature. I thought that what would have been useful was an option to replace the dummy dest_addr (like 127.0.0.1 or localhost) with a real one, accepted by a cloud provider.

That being said, I followed your idea and created a PR that implements override_empty_host_header: #129. Looking for feedback :D