net/http: parse and ignore chunked encoding's chunk-extension #13135

uaalto · 2015-11-03T17:39:28Z

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.

The text was updated successfully, but these errors were encountered:

bradfitz · 2015-11-03T18:16:25Z

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 · 2015-11-03T18:22:25Z

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 · 2015-11-03T18:43:55Z

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 · 2015-11-03T18:45:16Z

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

This is an easy fix, though.

uaalto · 2015-11-03T18:48:36Z

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 · 2015-11-03T18:53:42Z

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 · 2015-11-03T18:57:54Z

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 · 2015-11-03T19:44:09Z

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 · 2015-11-03T20:00:02Z

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 · 2015-11-03T21:01:46Z

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

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 · 2015-11-05T08:01:36Z

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

bradfitz · 2015-11-05T10:52:21Z

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 · 2015-11-05T12:56:36Z

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 · 2015-11-05T13:03:45Z

Oh, I see the milestone is 1.6. Thanks!

uaalto mentioned this issue Nov 3, 2015

Go server crashes when running all tests in Co Ad getlantern/http-proxy#44

Closed

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

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

bradfitz closed this as completed in e36494e Nov 10, 2015

golang locked and limited conversation to collaborators Nov 10, 2016

gopherbot added the FrozenDueToAge label Nov 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http: parse and ignore chunked encoding's chunk-extension #13135

net/http: parse and ignore chunked encoding's chunk-extension #13135

uaalto commented Nov 3, 2015

bradfitz commented Nov 3, 2015

uaalto commented Nov 3, 2015

myleshorton commented Nov 3, 2015

bradfitz commented Nov 3, 2015

uaalto commented Nov 3, 2015

uaalto commented Nov 3, 2015

myleshorton commented Nov 3, 2015

bradfitz commented Nov 3, 2015

uaalto commented Nov 3, 2015

gopherbot commented Nov 3, 2015

gopherbot commented Nov 5, 2015

bradfitz commented Nov 5, 2015

uaalto commented Nov 5, 2015

uaalto commented Nov 5, 2015

net/http: parse and ignore chunked encoding's chunk-extension #13135

net/http: parse and ignore chunked encoding's chunk-extension #13135

Comments

uaalto commented Nov 3, 2015

bradfitz commented Nov 3, 2015

uaalto commented Nov 3, 2015

myleshorton commented Nov 3, 2015

bradfitz commented Nov 3, 2015

uaalto commented Nov 3, 2015

uaalto commented Nov 3, 2015

myleshorton commented Nov 3, 2015

bradfitz commented Nov 3, 2015

uaalto commented Nov 3, 2015

gopherbot commented Nov 3, 2015

gopherbot commented Nov 5, 2015

bradfitz commented Nov 5, 2015

uaalto commented Nov 5, 2015

uaalto commented Nov 5, 2015