New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net/http: Chunked request body incorrectly terminated on \r\n\r\n
instead of 0\r\n\r\n
#64517
Comments
(attn @neild) |
Turns out that some servers ignore extra CRLFs between chunks. Here's a payload that demonstrates that discrepancy:
If there were a way to get Go to see a request line where Jetty and Waitress see a chunk size (and extension), then there might be a way to make a request smuggling attack out of this? Not sure if that's possible. |
The simplest patch is probably this: diff --git a/src/net/http/internal/chunked.go b/src/net/http/internal/chunked.go
index aad8e5aa09..044a79d053 100644
--- a/src/net/http/internal/chunked.go
+++ b/src/net/http/internal/chunked.go
@@ -263,6 +263,9 @@ type FlushAfterChunkWriter struct {
}
func parseHexUint(v []byte) (n uint64, err error) {
+ if len(v) == 0 {
+ return 0, errors.New("invalid empty chunk length")
+ }
for i, b := range v {
switch {
case '0' <= b && b <= '9': This would just make the connection close after the invalid message, but it wouldn't make the response 400, so it's not an ideal fix on its own. |
Change https://go.dev/cl/553835 mentions this issue: |
Actually, it would respond with |
(I'm assuming you meant "error" instead of "non-error" here.) This isn't true. It only closes the connection; it does not respond 400. To see for yourself, build master (current commit is 8db1310) and run the following simple program: package main
import (
"fmt"
"net/http"
)
func handle_request(w http.ResponseWriter, req *http.Request) {
fmt.Fprintf(w, "Hello world\n")
}
func main() {
s := &http.Server{
Addr: "127.0.0.1:8080",
Handler: http.HandlerFunc(handle_request),
MaxHeaderBytes: 1 << 20,
}
s.ListenAndServe()
} Then send it a request with an empty chunk size:
You should see a valid response:
The only difference between the current behavior and the previous behavior is that now the connection is closed after the invalid message body is processed. This should really be a 400; not just a closed connection. I would appreciate it if someone would reopen this issue. |
To clarify, you need to do things explicitly with the current fix, something like this: func echoHandler(w http.ResponseWriter, r *http.Request) {
body, err := io.ReadAll(r.Body)
if err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
return
}
fmt.Fprintf(w, "Received: %s", body)
} because it's lazy read for On the code level, there is no doubt that we can change the current pattern of |
This is unorthodox, but okay. Thanks for letting me know.
I think it's damned if you do, damned if you don't. While changing this behavior would probably break someone's work somewhere, not changing it is probably silently breaking people's expectations now.
Agreed. |
…length Fixes golang#64517 Change-Id: I78b8a6a83301deee05c3ff052a6adcd1f965aef2 Reviewed-on: https://go-review.googlesource.com/c/go/+/553835 Auto-Submit: Damien Neil <dneil@google.com> Commit-Queue: Damien Neil <dneil@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: Bryan Mills <bcmills@google.com>
Go version
go version devel go1.22-2e6387cbec Fri Dec 1 18:47:51 2023 +0000 linux/amd64
What operating system and processor architecture are you using (
go env
)?What did you do?
nc
):What did you expect to see?
The server should either respond 400 or time out, because the chunked message body is invalid. A chunked message body must be terminated with
0\r\n\r\n
. Terminating chunked message bodies on\r\n\r\n
alone introduces risk from any gateway that may have interpreted the request framing differently.What did you see instead?
The server responds 200.
The text was updated successfully, but these errors were encountered: