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: Calling request.Body.Close() triggers DOS attack vector #9662

Closed
klauspost opened this issue Jan 22, 2015 · 10 comments

Comments

Projects
None yet
6 participants
@klauspost
Copy link
Contributor

commented Jan 22, 2015

This is related to Issue #2093 - however, I found that the issue can still be triggered, if the body of the request is closed.

Server: http://play.golang.org/p/bBhu-3VKAE

Client example: curl -X POST -T "/path/to/huge-file" http://localhost:8080/

Output from server:

2015/01/22 14:42:47 Closing Body
2015/01/22 14:42:53 Body Closed

The 8 seconds are spent transferring the entire POST body to the server, even though it is discarded. I would expect the Close() call to be a NO-OP at worst.

The fix for the server is simply to NOT close the request body. However there isn't any "calling Body.Close() is a bad idea", in fact people in the community have been encouraging it.

Github search

Go version: go version go1.4 windows/amd64

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 22, 2015

Hopefully everybody should switch to HTTP/2 soon. :-)

I'll look into this, but unfortunately HTTP/1 doesn't provide a signaling mechanism from the handler to the sender to stop sending its request body. We can stop reading, but then we can never get to the next request, so then we have to drop the TCP connection after our response, and the peer may not even get it, if they're blocked writing. But that's the best we can do I think. I thought that's roughly what we already did.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jan 23, 2015

I can see even larger pause when I send 1G file from linux to windows:

2015/01/23 10:55:57 Closing Body
2015/01/23 10:56:51 Body Closed

Alex

@mattn

This comment has been minimized.

Copy link
Member

commented Jan 23, 2015

I'm not sure. because the handler is called, content-length is set to something (maybe wrong). So it's possible to read for the end of request to skip for next request (without parsing)? if content-length is not set, just close connection, i think. Right?

@andybalholm

This comment has been minimized.

Copy link

commented Jun 6, 2015

I don't think this issue is much to worry about on plain HTTP. (On HTTPS, where you're spending CPU to decrypt all that useless data, maybe it could be a problem.)

I can think of three things that an attacker could potentially waste on your server by doing this: incoming bandwidth, CPU, and number of open connections (file descriptors and goroutines).

There are easier ways to DOS a server on bandwidth and number of open connections. So the only one left is CPU.

So I decided to see how much CPU is consumed:

$ time ./post
2015/06/06 10:21:45 Closing Body
2015/06/06 10:21:53 Body Closed
^C
real    0m18.970s
user    0m1.283s
sys 0m3.228s
$ time curl -X POST -T /Users/andy/Downloads/OS\ installs/Windows10_InsiderPreview_x64_EN-US_10074.iso  http://localhost:8080/

real    0m8.169s
user    0m0.540s
sys 0m5.491s

(This is on a MacBook Pro with a 2.2 GHz i7.)

So posting a 3.78 GB file takes 8 seconds. And the CPU is pretty busy during that time. But most of the time is kernel time. So processing the packets through the TCP/IP stack is taking more time than the Go code in Resquest.Body.Close.

But anyway, that's about 1 CPU-second per gigabyte, which implies that a bandwidth-to-CPU-core ratio of 8 Gb/sec per core on the server would be necessary to make this a useful DOS technique. Maybe some cloud servers have that. If I count all the computers in my office that are currently turned on, I'm working with more like 0.15 Mb/sec per core here…

@klauspost

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2015

@andybalholm - It will kill your inbound connection. CPU isn't the issue. Connection saturation is a very common DDOS technique.

There are easier ways to DOS a server on bandwidth [...]

There shouldn't be. UDP/ICMP/SYN flooding is much easier to filter.

HTTP flood do not use malformed packets, spoofing or reflection techniques, and require less bandwidth than other attacks to bring down the targeted site or server.

Your server accepting unlimited amounts of data is just leaving the door open, and if you have SSL you also use CPU for all data as you mention.

@andybalholm

This comment has been minimized.

Copy link

commented Jun 6, 2015

UDP/ICMP/SYN flooding is much easier to filter.

Only if your ISP does the filtering. Once the incoming packets hit your firewall, it's too late—they've already clogged your connection. (I have actually encountered a situation where miscellaneous IP packets from who knows where were consuming most of a site's incoming bandwidth. All I could do about it was tell them to contact their ISP.)

HTTP flood do not use malformed packets, spoofing or reflection techniques, and require less bandwidth than other attacks to bring down the targeted site or server.

The technique that article was talking about does not saturate the network connection. It consumes memory, CPU, etc. on the server. That's why it takes less bandwidth than other attacks.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 6, 2015

This will be fixed. Let's leave unrelated discussions off this bug.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 24, 2015

@gopherbot

This comment has been minimized.

Copy link

commented Jun 24, 2015

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

@bradfitz bradfitz closed this in 1045351 Jun 25, 2015

@gopherbot

This comment has been minimized.

Copy link

commented Jun 27, 2015

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

bradfitz added a commit that referenced this issue Jun 29, 2015

net/http: fix now-flaky TransportAndServerSharedBodyRace test
TestTransportAndServerSharedBodyRace got flaky after
issue #9662 was fixed by https://golang.org/cl/11412, which made
servers hang up on clients when a Handler stopped reading its body
early.

This test was affected by a race between the the two goroutines in the
test both only reading part of the request, which was an unnecessary
detail for what the test was trying to test (concurrent Read/Close
races on an *http.body)

Also remove an unused remnant from an old test from which this one was
derived. And make the test not deadlock when it fails. (which was why
the test was showing up as 2m timeouts on the dashboard)

Fixes #11418

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

@golang golang locked and limited conversation to collaborators Jun 27, 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.