Skip to content
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: parse and ignore chunked encoding's chunk-extension #13135

Closed
uaalto opened this issue Nov 3, 2015 · 14 comments

Comments

Projects
None yet
5 participants
@uaalto
Copy link

commented Nov 3, 2015

The server:

package main

import (
  "fmt"
  "io"
  "net/http"
  "net/http/httputil"
)

func hello(w http.ResponseWriter, r *http.Request) {
  dump, _ := httputil.DumpRequest(r, true)
  fmt.Println(dump)
  io.WriteString(w, "Hello world!")
}

func main() {
  http.HandleFunc("/a", hello)
  http.ListenAndServe(":8000", nil)
}

HTTP request:

POST http://localhost:8080/a HTTP/1.1
Transfer-Encoding: chunked
Host: 209.169.10.134:8080
Connection: close


0; name=value

More information

  • The localhost host is irrelevant. Using the server address triggers it too.
  • It's triggered by DumpRequest, but that is just for the test case. Reading the request Body completely triggers it too.
  • It is probably due to the chunk extension. The Go HTTP server corrupts bodies that use it.

@bradfitz bradfitz changed the title HTTP Request triggers runtime error in HTTP Server net/http: parse and ignore chunked encoding's chunk-extension Nov 3, 2015

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 3, 2015

Yes, I guess we don't parse chunked-extension. We should, but I'm surprised anybody is using it.

Did this come up in real life, or is it contrived?

@uaalto

This comment has been minimized.

Copy link
Author

commented Nov 3, 2015

We are running our proxy through a bunch of HTTP conformity tests. I think is not very common, but it should be cleanly ignored. HTTP specification states that applications / proxies should just ignore them if they don't understand them. Ideally, we parse them, but otherwise I think just ignoring them is fine.

@myleshorton

This comment has been minimized.

Copy link

commented Nov 3, 2015

In particular this arose through tests for HTTP proxy spec conformity with co-advisor, which is a very handy tool. We agree it's not used in practice @bradfitz, but it would be good if it didn't crash as it creates an incredibly easy attack vector for anyone trying to attack the server.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 3, 2015

To be clear, it's not crashing, right? It's just truncating the request body?

This is an easy fix, though.

@uaalto

This comment has been minimized.

Copy link
Author

commented Nov 3, 2015

With Go 1.5.1 on Debian Jessie:

2015/11/03 19:48:11 http: panic serving [::1]:62757: runtime error: invalid memory address or nil pointer dereference
goroutine 19 [running]:
net/http.(*conn).serve.func1(0xc8200b4000, 0x5ac628, 0xc820072020)
    /usr/local/Cellar/go/1.5.1/libexec/src/net/http/server.go:1287 +0xb5
net/http.(*response).finishRequest(0xc8200b40b0)
    /usr/local/Cellar/go/1.5.1/libexec/src/net/http/server.go:1160 +0xc5
net/http.(*conn).serve(0xc8200b4000)
    /usr/local/Cellar/go/1.5.1/libexec/src/net/http/server.go:1365 +0xc28
created by net/http.(*Server).Serve
    /usr/local/Cellar/go/1.5.1/libexec/src/net/http/server.go:1910 +0x3f6
@uaalto

This comment has been minimized.

Copy link
Author

commented Nov 3, 2015

The other tests that trigger it.

    POST http://<edited-ip>:8080/ogdlqgxkbepd HTTP/1.1\r\n
    Transfer-Encoding: chunked\r\n
    Host: 209.169.10.134:8080\r\n
    Connection: close\r\n
    \r\n

    0; name=value\r\n
    \r\n
    POST http://<edited-ip>:8080/aedhcajqmrqy HTTP/1.1\r\n
    Transfer-Encoding: chunked\r\n
    Host: 209.169.10.134:8080\r\n
    Connection: close\r\n
    \r\n

    64; name="egtaww[ ... 16363 bytes, MD5: 1c8632befae6770ccd3fd2315df745c8 ]zkhnpunivhjsafwd"\r\n
    dojsfxsbxnctecvdoeseagowpyosdnvibeagbukbjmwnqtsgxlkatbwizkaexwmycohgithrhfhzzzfykqyftuoshqygokhryoxi\r\n
    0\r\n
    \r\n
    POST http://<edited-ip>:8080/zcdkvxbjdgwt HTTP/1.1\r\n
    Transfer-Encoding: chunked\r\n
    Host: 209.169.10.134:8080\r\n
    Connection: close\r\n
    \r\n

    64; name=hettnje[ ... 16362 bytes, MD5: c7fcaca7d0fe4e8fcc517bfdebad28f7 ]ibsoiezrjwttqkvo\r\n
    bqfimbbuacdeohawiumsynkjkfccpaqsqydfbezegciwmiuwegpdtzofgtjxtcqlavqbzpfhuqdgazefftkauyiarryktqyvlozk\r\n
    0\r\n
    \r\n
    POST http://<edited-ip>:8080/ouepavuazxic HTTP/1.1\r\n
    Transfer-Encoding: chunked\r\n
    Host: 209.169.10.134:8080\r\n
    Connection: close\r\n
    \r\n

    64; chunkextname\r\n
    pbcggproccuzedwmqgktnsboanhpqdqhgunojgdnixopambsvlmigpwiefzwkrermtfxaknkibzkqddnqrvyithpbgllyremlmln\r\n
    0\r\n
    \r\n
    POST http://<edited-ip>:8080/xtukkkklnpvi HTTP/1.1\r\n
    Transfer-Encoding: chunked\r\n
    Host: 209.169.10.134:8080\r\n
    Connection: close\r\n
    \r\n

    64; name="quoted string"\r\n
    qjqfopqotmpbqxzzxtuvmhelmcpcwvupekwszmjuaywqxxpurksdtyriagkzbeoiokcqwlmyliojiefbqzglxxtagdzjjnraxuqv\r\n
    0\r\n
    \r\n
    POST http://<edited-ip>:8080/mztsfqlumrlm HTTP/1.1\r\n
    Transfer-Encoding: chunked\r\n
    Host: 209.169.10.134:8080\r\n
    Connection: close\r\n
    \r\n

    64; name = value\r\n
    stmybbtrcepyectdqextknxfchibejjwevuhyqzawqaauudlycgipepulzxrigoodkvbawcyoeyjyewygciyizsvyrngzdwcpufq\r\n
    0\r\n
    \r\n
    POST http://<edited-ip>:8080/ngmmfwqdrlpe HTTP/1.1\r\n
    Transfer-Encoding: chunked\r\n
    Host: 209.169.10.134:8080\r\n
    Connection: close\r\n
    \r\n

    64; name=value\r\n
    wyxgczrucwhsncrxxamygamgrqseegrbgrhiqyeuwlojphjnhvnqvzwpsrvwxpzdghoyhsseggpvoykxwyntzmirdgqcvphbwxbg\r\n
    0\r\n
    \r\n
@myleshorton

This comment has been minimized.

Copy link

commented Nov 3, 2015

I believe all of those serve calls are wrapped in a recover() call though right @bradfitz? So the server will at least stay up in spite of the panic.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 3, 2015

The panic reported here is unrelated to chunked-extensions.

The panic comes from finishRequest trying to call Request.Body.Close(), but the Request.Body is set to nil by the handler (via httputil.DumpRequest, oddly). That panic can be reproduced just by writing an http.Handler which is only:

   req.Body = nil

But if you actually looked at the error value from httputil.DumpRequest, you can see that it's a valid error, not a panic:

http: suspiciously long trailer after chunked body

That's it not understanding chunk extensions.

The reason that DumpRequest sets the body to nil is because it can't synthesize an equivalent body after failing to read it in the first place. (see docs for DumpRequest on why it needs to do that... the body is just an io.Reader and can't be rewound, so it's recorded instead)

@uaalto

This comment has been minimized.

Copy link
Author

commented Nov 3, 2015

Thank you for checking it out!
I actually did this as a reduced case, but I guess the cause is the same. I'm logging all errors, and didn't see anything similar. The proxy code doesn't use DumpRequest (except with high log levels). DumpRequest knows this and can log it, but I was triggering the same underlying issue. If I find out, I'll bring it.
Since the documentation mentions "DumpRequest is semantically a no-op", it triggering the panic is also confusing.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 3, 2015

CL https://golang.org/cl/16572 mentions this issue.

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Nov 3, 2015

bradfitz added a commit that referenced this issue Nov 4, 2015

net/http: don't panic after request if Handler sets Request.Body to nil
The Server's server goroutine was panicing (but recovering) when
cleaning up after handling a request. It was pretty harmless (it just
closed that one connection and didn't kill the whole process) but it
was distracting.

Updates #13135

Change-Id: I2a0ce9e8b52c8d364e3f4ce245e05c6f8d62df14
Reviewed-on: https://go-review.googlesource.com/16572
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Nov 5, 2015

CL https://golang.org/cl/16680 mentions this issue.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 5, 2015

Note that https://golang.org/cl/16680 fixes it, but not if the line containing the chunk length is over 4KB. I'm not going out of my way to support that. Since nobody uses chunk extensions anyway, it follows that nobody uses chunk extensions over 4KB. If you do, you deserve to be truncated.

@uaalto

This comment has been minimized.

Copy link
Author

commented Nov 5, 2015

Thank you @bradfitz. That is great.
If someone uses chunk extensions over 4Kb, deserves something worse than just being truncated ;)

Do these patches get into the next minor version of Go?

@uaalto

This comment has been minimized.

Copy link
Author

commented Nov 5, 2015

Oh, I see the milestone is 1.6. Thanks!

@bradfitz bradfitz closed this in e36494e Nov 10, 2015

@golang golang locked and limited conversation to collaborators Nov 10, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.