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: potential DOS: request context not cancelled if a client sends one byte after ServeHTTP starts #37145

Closed
cyolosec opened this issue Feb 9, 2020 · 3 comments

Comments

@cyolosec
Copy link

@cyolosec cyolosec commented Feb 9, 2020

What version of Go are you using (go version)?

$ go version
go version go1.13.7 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users//Library/Caches/go-build"
GOENV="/Users//Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/eranshmuely/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.7/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.7/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/1t/f2jm2c3x12n35wmyv6w4vmwc0000gn/T/go-build114416166=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

ran the following code:

package main

package main

import "net/http"

func main() {
	http.ListenAndServe(":9090", http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
		println(1)
		<-req.Context().Done()
		println(2)
	}))
}

and then ran the following command:

$ nc localhost 9090
GET / HTTP/1.1
host: test

t

<CTRL+C>

What did you expect to see?

The request context should have been cancelled as per documentation:

...
//
// For incoming server requests, the context is canceled when the
// client's connection closes, the request is canceled (with HTTP/2),
// or when the ServeHTTP method returns.
func (r *Request) Context() context.Context {
	if r.ctx != nil {
		return r.ctx
	}
	return context.Background()
}
...

and thus i expected to see the following output:

1
2

What did you see instead?

1

The context did not cancel and the goroutine that serves the http.Handler never returns.
http.Servers that select directly on

req.Context().Done()

and not

context.WithTimeout(req.Context())

(or something like that) could be vulnerable to denial of service attacks.

@cyolosec cyolosec changed the title net/http: potential DOS: request context not cancelled if a client sends one byte after net/http: potential DOS: request context not cancelled if a client sends one byte after ServeHTTP starts Feb 9, 2020
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Feb 14, 2020

@AlexanderYastrebov
Copy link
Contributor

@AlexanderYastrebov AlexanderYastrebov commented Oct 19, 2021

The context is cancelled when connection is closed which is detected by observing read errors:

go/src/net/http/server.go

Lines 729 to 742 in 1b24c9e

// handleReadError is called whenever a Read from the client returns a
// non-nil error.
//
// The provided non-nil err is almost always io.EOF or a "use of
// closed network connection". In any case, the error is not
// particularly interesting, except perhaps for debugging during
// development. Any error means the connection is dead and we should
// down its context.
//
// It may be called from multiple goroutines.
func (cr *connReader) handleReadError(_ error) {
cr.conn.cancelCtx()
cr.closeNotify()
}

The server expects remaining body and thus does not start (defers until EOF) background read to monitor connection close:

go/src/net/http/server.go

Lines 1951 to 1955 in 1b24c9e

if requestBodyRemains(req.Body) {
registerOnHitEOF(req.Body, w.conn.r.startBackgroundRead)
} else {
w.conn.r.startBackgroundRead()
}

Since handler never reads request body it gets stuck waiting forever.

See also #15224 and #15927 and #15735

@AlexanderYastrebov
Copy link
Contributor

@AlexanderYastrebov AlexanderYastrebov commented Oct 21, 2021

Duplicate of #23262

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants