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: document security considerations for serving internet traffic #22085

Open
mmcloughlin opened this Issue Sep 28, 2017 · 13 comments

Comments

Projects
None yet
@mmcloughlin
Contributor

mmcloughlin commented Sep 28, 2017

The net/http/pprof package implicitly registers HTTP handlers through its init() function. I argue this implicit behavior is too subtle and may contribute to people inadvertently leaving such endpoints open. Some IPv4 scans reveal a non-trivial number of pprof endpoints exposed (http://mmcloughlin.com/posts/your-pprof-is-showing). Since Go 1.9 an exposed pprof endpoint leaks source code.

I would like to spark a discussion on the possibility of moving to an explicit handler registration model. It is not clear to me that this would qualify for the Security exemption of the Go 1 compatibility guarantee, but I thought it was worthy of mention. Perhaps a warning in documentation would be enough.

@mvdan mvdan added the NeedsDecision label Sep 28, 2017

@mvdan mvdan added this to the Go1.10 milestone Sep 28, 2017

@dlsniper

This comment has been minimized.

Contributor

dlsniper commented Sep 28, 2017

It only registers those handlers implicitly if you use the default http server instance. If you use a custom one, that doesn't happen. The problem is that people are using http.ListenAndServe without understanding its implications, not net/http/pprof imho. Furthermore, the default server usually needs tweaking when exposed to the Internet.

@mmcloughlin

This comment has been minimized.

Contributor

mmcloughlin commented Sep 28, 2017

This is ultimately user error as you say. People are indeed using http.ListenAndServe and net/http/pprof without understanding its implications. I'm suggesting that this is an easy and understandable mistake, and we should strive to make the consequences as benign as possible.

If we were to address this by making backwards incompatible changes, I would suggest that replacing the init() function of net/http/pprof with a RegisterHandlers() (or similar) would be the most effective.

Arguably this is not serious enough to break compatibility. I think at least a warning in documentation is warranted.

@docmerlin

This comment has been minimized.

docmerlin commented Sep 29, 2017

I concur about a warning in documentation!

@mappu

This comment has been minimized.

mappu commented Sep 29, 2017

I understand the function names and file names are embedded into all binaries for panics and for reflection. I don't even particularly mind the pprof behaviour of attaching to the default mux.

But leaking source code is beyond the pale.

How does pprof get this information? Is it compiled into 1.9 binaries or loaded externally? If the former, is it only compiled into 1.9 binaries when pprof is used?

@dlsniper

This comment has been minimized.

Contributor

dlsniper commented Sep 29, 2017

@mmcloughlin shouldn't you update your blog post if you think that the better solution is to not use the default ListenAndServe / http server and instead initialize your own?

As for Go documentations, PRs are welcomed by the team, so improving the usage example of net/http/pprof would be nice as well.

@4ad

This comment has been minimized.

Member

4ad commented Sep 29, 2017

The source code is not embedded in the binary! go tool pprof has to have access to the source code in order to print the source code. The binaries only contain source code coordinates for symbols.

You ran all your tests on localhost, where pprof had access to the source of the server.

Of course, this does not mean the current functionality is not a footgun. I wish net/http/pprof would not work like this and I wish it didn't have a default mux, but it's too late to change any of that.

@mmcloughlin

This comment has been minimized.

Contributor

mmcloughlin commented Sep 29, 2017

@4ad You're completely right, apologies for my mistake. Classic case of getting excited at a result and not doing the due diligence. Thanks for pointing this out. I'll update the post accordingly.

As you say there are still risks here people should be aware of.

@dlsniper I believe the Prevention section of the post covers this.

@rsc

This comment has been minimized.

Contributor

rsc commented Oct 23, 2017

There are a number of packages that automatically register handlers under /debug/.... We chose that prefix specifically so that servers that serve direct internet traffic can put a handler in front of the http.DefaultServeMux to filter those out for untrusted clients.

@FiloSottile also has some tips about what to do as far as configuring other HTTP settings to avoid problems when serving direct internet traffic.

Probably a doc fix is correct, and probably http.ListenAndServe would be a good place, since (probably?) more people read the ListenAndServe docs than the pprof docs. Or maybe a link to a wiki page? Unclear.

@rsc rsc changed the title from net/http/pprof: handler registration in init as potential security risk to net/http: document security considerations for serving internet traffic Oct 23, 2017

@rsc

This comment has been minimized.

Contributor

rsc commented Oct 23, 2017

It seems like perhaps the right next step is for someone to write a wiki page, and then the ListenAndServe docs can have a short link to the page. Putting lots of detail into the doc comment doesn't make as much sense as a wiki page we can update out-of-band.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jul 11, 2018

Also to document somewhere: use of MaxBytesReader to limit the bytes read by Request.ParseForm, Request.FormValue, etc.

@tsuna

This comment has been minimized.

Contributor

tsuna commented Jul 13, 2018

Russ wrote:

@FiloSottile also has some tips about what to do as far as configuring other HTTP settings to avoid problems when serving direct internet traffic.

Just in case it's of any help to someone else wondering what those tips might be, a bunch of Googling revealed that Russ was most probably referring to this blog post: So you want to expose Go on the Internet, although that doesn't have anything to say about the things @bradfitz mentioned.

In any case, thanks @mmcloughlin for raising this issue, I was just about to push a change that would've exposed our debug endpoints to the Internets, the mistake is definitely very easy to make.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jul 25, 2018

@FiloSottile, are you going to do this for Go 1.11?

@rsc

This comment has been minimized.

Contributor

rsc commented Aug 17, 2018

Too late.

@rsc rsc modified the milestones: Go1.11, Go1.12 Aug 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment