caddyserver / caddy

Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS
https://caddyserver.com
Apache License 2.0
57.83k stars 4.02k forks source link

[bug] cannot deny transfer-encoding requests #6628

Open shyim opened 4 days ago

shyim commented 4 days ago

Hey,

I am trying to block Transfer-Encoding: chunked requests. But this seems not possible right now in Caddy:

I tried following:

@length {
   header Transfer-Encoding chunked
}

respond @length 411

I think the header Transfer-Encoding is missing in the available headers: as the logs does not mention it too:

{"level":"info","ts":1729001178.8945065,"logger":"http.log.access","msg":"NOP","request":{"remote_ip":"192.168.117.1","remote_port":"49353","client_ip":"192.168.117.1","proto":"HTTP/1.1","method":"POST","host":"127.0.0.1:8000","uri":"/","headers":{"Content-Type":["application/json"],"User-Agent":["curl/8.7.1"],"Accept":["*/*"]}},"bytes_read":0,"user_id":"","duration":0.000008292,"size":0,"status":0,"resp_headers":{"Server":["Caddy"]}}

When i change the header to X-Foo everything works

francislavoie commented 4 days ago

Interestingly, Go doesn't keep Transfer-Encoding in the headers map, it moves it to a separate field on the request: https://pkg.go.dev/net/http#Request.TransferEncoding

We have special handling for the header matcher for Host which is also moved separately, we could probably do the same for TransferEncoding. Would make sense to allow matching it I think.

steffenbusch commented 4 days ago

@francislavoie Wouldn't it make sense to add the special handling for the Transfer-Encoding header also to the json logging area to have Transfer-Encoding be logged in request>headers>Transfer-Encoding[]?

francislavoie commented 4 days ago

Hm, maybe yeah. Lemme take a look.

shyim commented 4 days ago

It should be here right? https://github.com/caddyserver/caddy/blob/a211c656f12bcab73df0de114f2b6100ee5a0fe4/modules/caddyhttp/matchers.go#L972

francislavoie commented 4 days ago

@shyim did you see https://github.com/caddyserver/caddy/pull/6629?

@steffenbusch It's tricky actually. I think right now we're logging with no copying of headers, but if we try to manipulate the request header list to log it then we do need to copy it to avoid the inserted header field from affecting anything else outside of logging. I think I can add it as a new field beside headers though.

WeidiDeng commented 3 days ago

@shyim Simply matching by transfer-encoding isn't enough, http2/3 doesn't have this header. The problem here is that the content-length is unknown in advance, the php-fpm needs this info to proceed.

I'll retry to figure out how to buffer requests in this case. Might take a few day.

dallyger commented 3 days ago

@WeidiDeng If I understand correctly, php-fpm will read the content-length header. But if Transfer-Encoding: chunked is used, that header is not given by the client. Instead each chunk will be prefixed with the content length.

php-fpm hangs if there is no content-length header. This header is only missing if transfer chunked is used. Therefore, it should be enough to match transfer-encoding. Anything else should work as expected. Am I missing something?

Or would it be a better idea to match against a missing content-length header?

WeidiDeng commented 3 days ago

@dallyger Yes, it's better to match against the missing content-length header.