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: GET request hangs if "Content-Length" reqest-header field ist set (possible DoS vulnerability) #33269

Open
je4 opened this issue Jul 24, 2019 · 9 comments
Milestone

Comments

@je4
Copy link

@je4 je4 commented Jul 24, 2019

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

PS C:\WINDOWS\System32> go version
go version go1.12.7 windows/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output
PS C:\WINDOWS\System32> go env
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\juergen.enge\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\daten\go
set GOPROXY=
set GORACE=
set GOROOT=c:\go
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\JUERGE~1.ENG\AppData\Local\Temp\go-build111174446=/tmp/go-build -gno-record-gcc-switches

What did you do?

sending a GET request to the native go http server with "Content-length" request parameter

package main

import (
	"log"
	"net/http"
)

func main() {
	server := &http.Server{Addr: "localhost:80"}

	log.Fatal(server.ListenAndServe())

}

What did you expect to see?

immediate response with error code 404 page not found (<2ms)

What did you see instead?

request hangs for very long time (>1min, client kills connection).

Initial debugging

it seems, that the function call w.finishRequest() in func (c *conn) serve(ctx context.Context) from server.go does not return. There is a problem in flushing the write buffer...

@je4 je4 changed the title net/http: GET request hangs if "Content-Length" header field ist set (possible DoS vulnerability) net/http: GET request hangs if "Content-Length" reqest-header field ist set (possible DoS vulnerability) Jul 24, 2019
@av86743

This comment has been minimized.

Copy link

@av86743 av86743 commented Jul 24, 2019

If you are sending non-zero Content-Length and not providing data of that length, what else would you expect?

@je4

This comment has been minimized.

Copy link
Author

@je4 je4 commented Jul 24, 2019

since a request body does not make any sense in most cases for a GET request, the idle-timeout for waiting for the body data could be automatically decreased.
with the actual status, i feel, that most golang webservers could have a problem with long running requests if there are many of them...
if you are using a handler which does not read the request body, the handler is finished while the system is waiting for the request body...

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Jul 30, 2019

For this to be a security issue, it should bypass the server timeouts, as otherwise there are a number of ways to get the server to wait until the timeout.

Could you provide the code or command you are using to generate the request?

@FiloSottile FiloSottile added this to the Go1.14 milestone Jul 30, 2019
@je4

This comment has been minimized.

Copy link
Author

@je4 je4 commented Aug 1, 2019

i have used the "Advanced REST Client" (https://install.advancedrestclient.com/install)
the content-length variable remained while switching from POST to GET

2019-08-01 16_09_49-Advanced REST client

@je4

This comment has been minimized.

Copy link
Author

@je4 je4 commented Aug 1, 2019

Further investigation shows, that an nginx server which serves static files seems to ignore this field. nginx with php-fpm activated folder shows the same behaviour...

@lucasmoten

This comment has been minimized.

Copy link

@lucasmoten lucasmoten commented Aug 17, 2019

From this..

if you are using a handler which does not read the request body, the handler is finished while the system is waiting for the request body

I believe appropriate course of action is to have all handlers read the request body to the end, and make use of ReadTimeout and ReadHeaderTimeout on Server.

@je4

This comment has been minimized.

Copy link
Author

@je4 je4 commented Aug 17, 2019

I believe appropriate course of action is to have all handlers read the request body to the end, and make use of ReadTimeout and ReadHeaderTimeout on Server.

I am not sure about the definition of "end". If there's a content length given and the data is needed the end correlates with the data length.
Timeout is a very good thing. But for handlers which do not use any request body data I like the "Error 400 Bad Request" solution because no client has any reason to send this data.
This could be done for the handlers shipped with go.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Oct 17, 2019

@je4 thank you for this report! So we've been thinking about this for a while as per #13722 /cc @bradfitz and in 2016 @bradfitz added a great suggestion to perhaps make an opt-in knob #13722 (comment) to accept GET bodies.

My data point to accept bodies with GETs is that in the wild and even widely encouraged is Elastic search which permits GET with bodies e.g. https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-body.html.

Your reference in #33269 (comment) doesn't necessarily apply as apples-to-apples because most of those are consumer sites whereas Go creates web servers so the business logic is what you are comparing yet this is a general purpose web server.

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Oct 17, 2019

I'd say let's close this issue and move the discussion and advocacy to issue #13722, please let me know what you think @je4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.