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

Jump to bottom

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
Closed

net/http: potential DOS: request context not cancelled if a client sends one byte after ServeHTTP starts #37145

cyolosec opened this issue Feb 9, 2020 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@cyolosec
Copy link

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 dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 14, 2020
@dmitshur
Copy link
Contributor

/cc @bradfitz @katiehockman @FiloSottile

@AlexanderYastrebov
Copy link
Contributor

AlexanderYastrebov commented Oct 19, 2021 •
edited

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

Duplicate of #23262

@AlexanderYastrebov AlexanderYastrebov mentioned this issue Oct 21, 2021
Open
@neild neild closed this as completed Oct 21, 2021
@golang golang locked and limited conversation to collaborators Oct 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@neild @AlexanderYastrebov @dmitshur @gopherbot @cyolosec