Closed nick-ge closed 1 year ago
Hey ! First of all, thank you for the detailed issue. It's way easier to debug this way.
I will validate but I think chunk transfers are removed from the http request objects in golang, which means coraza will never have access to it in this connector. Probably coraza receives the full buffered request.
I think @M4tteoP has information on this
Hi, many thanks for taking the time on providing lots of details!
I will validate but I think chunk transfers are removed from the http request objects in golang, which means Coraza will never have access to it in this connector.
I really think this is the issue. 920171-2
and 920171-3
are two of the few CRS tests that are still failing against the go/http middleware (that is the same used by the latest commit in the caddy connector). The Go library removes the Transfer-Encoding
header (see here), and this is exactly what the 920171
rule is looking for (SecRule &REQUEST_HEADERS:Transfer-Encoding "!@eq 0"
).
What we should maybe do is understand if it is possible from the http.Request
to read the Chunked boolean. If so we can populate again the header. But I don't know if it is feasible or if the HTTP library makes the Chunked logic completely transparent.
Update: https://github.com/golang/go/blob/master/src/net/http/transfer.go#L586 this line is definitely interesting. Maybe something like the following code can fix it.
if r.TransferEncoding != nil && r.TransferEncoding[0] == "chunked" {
r.Header.Set("Transfer-Encoding", "chunked")
}
I will give it a go next week :)
Thank you for your fast response!
@M4tteoP This could do the trick and you should definitively give it a shot!
In my opinion however, it is not a problem of this connector but of the go/http library which gives you a HTTP request without a header that indicates a body is available. As the RFC says, Content-Length
and Transfer-Encoding
are responsible for that. So a more cleaner solution would be, if the go/http library replaces the Transfer-Encoding
with a appropriate Content-Length
header instead of removing it completely. This should trigger CRS rule 920170 then. So maybe we should populate this problem to the devs of go/http?
I believe the behavior Go is going for is that, because the body that is provided doesn't have chunked headers, the header is removed - I guess the idea is to present to business logic in the same way regardless of encoding.
But even so the body is still streamed so it can't compute a content length.
There doesn't seem to be a great reason to remove the encoding header anyways but I guess they'll keep the behavior for backwards compatibility.
Probably synthesizing the header for the coraza transaction is going to be the way to go if it's possible. Otherwise maybe a phase 2 rule could be added that checks &REQUEST_BODY
.
Dear Coraza-Caddy maintainer,
it seems if CRS rule 920171 is not applied on chunked-encoded requests (i.e., requests using
Transfer-Encoding: chunked
). I used the following request to identify this behavior:Issue
This request gets accepted by Coraza+Caddy as the screenshots below demostrate:
However, it is expected that a
GET
request with a message body is blocked by rule 920171.Side Note
Sending this request to another CRS engine (e.g., ModSecurity) triggers the expected rejection:
Here ModSecurity's logs indicating the resposible rule:
Impact
This has no severe impact since it seems all other rules are still applied on the body as expected. Here a further request line was embedded into the body which triggers the respective rule:
Setup
Summarized I've used:
Here is my Dockerfile I've used to build Caddy + Coraza:
nick/caddy:2.6.2-no-expose
is a customized Dockerfile which removed Docker'sEXPOSE
statements and an error (see this issue for more information). Here is its source code:Caddy configuration file:
Coraza configuration:
If I'm missing something please let me know. Otherwise I'm glad if I can contribute to your project!
King regards Nick