Chore: updates Middleware adding synthesis of Transfer-Encoding header

[M4tteoP]

Aligns the coraza-caddy middleware with the Coraza one porting corazawaf/coraza#768.
Closes #53.

[jcchavezs]

Does this remain true for caddy aswell? Shall we provide some refs to this assertion?

[M4tteoP]

I can't find the precise call from the caddy codebase, but before ServeHTTP which is the caddy entry point for all HTTP requests, the go/http transfer encoding handling is already called. Dumping the headers from the Coraza filter we experience the same behavior of our Coraza middleware: with the Transfer-Encoding header stripped off.

I added a reference to the Transfer-Encoding handling go/http side.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[M4tteoP]

I did some more manual tests with the help of the example. With this fix caddy manages to detect the Transfer-Encoding header and triggers the interruption (if it is the case).

One corner case I noticed with a different behavior compared to the plain Go HTTP server: Just sending the chunked header without a body, the WAF detects it and triggers the interruption, but then the request remains hanging. 🤔
Request: curl -H "Transfer-Encoding:chunked" 'localhost:8080' -v
Logs: {"level":"debug","ts":1682501883.4048684,"logger":"http.handlers.waf","msg":"Transaction finished","tx_id":"DDBoQbOLaWYqpUZR","is_interrupted":true,"status":403,"rule_id":111}
I can reproduce this behavior also before this PR, I think it is rather a Caddy behavior, of which I am not an expert

[jcchavezs]

Maybe @mholt can help us to understand why the requests remains hanging.

…

On Wed, 26 Apr 2023, 13:47 Matteo Pace, ***@***.***> wrote: I did some more manual tests with the help of the example <#56>. With this fix caddy manages to detect the Transfer-Encoding header and triggers the interruption (if it is the case). One corner case I noticed with a different behavior compared to the plain Go HTTP server: Just sending the chunked header without a body, the WAF detects it and triggers the interruption, but then the request remains hanging. 🤔 Request: curl -H "Transfer-Encoding:chunked" 'localhost:8080' -v Logs: {"level":"debug","ts":1682501883.4048684,"logger":"http.handlers.waf","msg":"Transaction finished","tx_id":"DDBoQbOLaWYqpUZR","is_interrupted":true,"status":403,"rule_id":111} I can reproduce this behavior also before this PR, I think it is rather a Caddy behavior, which I'm not an expert :( — Reply to this email directly, view it on GitHub <#55 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXOYAU7G2FVOJHLKGGMD73XDEDLLANCNFSM6AAAAAAXAMBWI4> . You are receiving this because you commented.Message ID: ***@***.***>

[mholt]

I'm not sure, but possibly caddyserver/caddy#5420 ? (Recently fixed, will be tagged in 2.7 beta 1 soon-ish) -- would be curious if it still hangs if you try the latest caddy on master.

[M4tteoP]

Sorry @mholt, I missed the edit. I gave a go with caddy 1af419e7eca0c7b8559ec2f3b397f948f1ef13c2 (currently the latest commit on master), and I'm experiencing the same behavior. Basically, even if the module performs a WriteHeader and returns from the function that implements caddyhttp.MiddlewareHandler, that response code is not returned to the client. Caddy keeps hanging (presumably waiting for the chunked part of the request (that will never come)?)

[mholt]

Thanks for trying it out. I guess my next question is why say the transfer-encoding is chunked when there's actually no body?

[M4tteoP]

Good question 😅, it definitely is a corner case, made of a malformed request. Being the module a WAF integration, I considered the possibility that the server could enforce the WAF action and interrupt the request as soon as it was detected as malicious. During this "hanging status" we already know that it is a bad request, but we can't act accordingly.

One significant note: I have been wrong during the first message, the caddy behavior seems consistent with the plain Go HTTP server (They are sharing the same Middleware); also the latter triggers the interruption, but remains hanging.

[jcchavezs]

@mholt so this PR aimed to test a rule that mitigated an attack. The transfer encoding header gets removed as a header because if becomes a first class field but coraza needs to be aware of it so we needed to somehow propagate it to coraza, that is fine.

On the other hand, sending a transfer encoding without a body is exactly a way of attacking the middlewares/proxies because some of them will send the headers on beforehand before receiving the body (triggering a header based attack).

@M4tteoP the behaviour of hanging response waiting for body seems fine as of course the server ia waiting for a body to be sent. If it is consistent with Go we should just accept it.

[mholt]

Gotcha. Sounds like you've decided on a good solution then.

I also generally recommend setting I/O timeouts for stuff like that.