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/pprof: disallow package to register to the default mux in Go 2 #42834

Open
rakyll opened this issue Nov 25, 2020 · 9 comments
Open

net/http/pprof: disallow package to register to the default mux in Go 2 #42834

rakyll opened this issue Nov 25, 2020 · 9 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@rakyll
Copy link
Contributor

rakyll commented Nov 25, 2020

net/http/pprof registers handlers to the default mux at init time. In order to register the handlers on a custom mux, you still have to import to package and have the debug handlers registered to the default mux. This creates the situation everyone who has a direct or transient dependency to the net/http/pprof package has the debug handles registered.

This creates security issues and long-term maintenance problems where you want to 100% avoid the use of the default mux to make sure debug endpoints are never exposed to the Internet accidentally. Instead of the current model, export a new API that registers these handlers to the default mux if users want to opt in.

(I remember seeing a similar issue but couldn't find it, filing another one but please close if it's a duplicate.)

@cagedmantis cagedmantis changed the title Disallow net/http/pprof to register to the default mux in Go 2 net/http/pprof: disallow package to register to the default mux in Go 2 Dec 2, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 2, 2020
@cagedmantis cagedmantis added this to the Go 2 milestone Dec 2, 2020
@cagedmantis
Copy link
Contributor

/cc @rsc

@kolyshkin
Copy link
Contributor

@cagedmantis cagedmantis modified the milestones: Go 2, Go2 Jan 26, 2021
@cagedmantis
Copy link
Contributor

It's been changed @kolyshkin. Thanks.

@leandrosansilva
Copy link

One intermediate step to achieve the aim of this request is providing some function to allow registering the debug handlers to any mux instead of asking the user to write them manually.

Here is a proposal in details:

net/http/pprof would continue registering them in the default mux as init(), but also offer the possibility of using a custom mux if the user prefers.

In any non trivial project I've worked so far I've never used the default mux (or globals in general), always being explicit in the mux I use.

If we assume that the most projects hardly (or never?) use the default mux in addition to one or more custom muxes, adding such handlers in the default mux is not a problem, as there's no ListenAndServe() using it so it's never exposed.

But I confess this is a strong assumption.

I imagine a call such as (on net/http/pprof), which basically is the current init(), but passing the mux as an argument:

// RegisterHandlers registers handlers with the mux
func RegisterHandlers(mux *http.ServeMux) {
       mux.HandleFunc("/debug/pprof/", Index)
       mux.HandleFunc("/debug/pprof/cmdline", Cmdline)
       mux.HandleFunc("/debug/pprof/profile", Profile)
       mux.HandleFunc("/debug/pprof/symbol", Symbol)
       mux.HandleFunc("/debug/pprof/trace", Trace)
}

Which could be called on init() with the http.DefaultServerMux, keeping the current expected behaviour.

A second step could be move such init() into a InitDefaultServerMux() and telling the users to explicit use it in case they are using the default mux.

Finally InitDefaultServerMux() would be deprecated and tell the user about the need to explicit register the handlers on whatever mux they are using.

Any thoughts? I would be glad on contributing withe adding RegisterHandlers() and use it on init(). It'd already remove some code duplication in some of my projects :-)

@kolyshkin
Copy link
Contributor

@cagedmantis perhaps it makes sense to also remove https://github.com/golang/go/milestone/175, otherwise someone else will make the same mistake.

@pedramteymoori
Copy link

I also wanted to +1 this change. We also had the exact security issue in our system, and pprof data was exposed on our main port, although we defined another ServerMux for it. Defining handlers in the init function can cause this severe security issue, as almost everyone uses DefaultServerMux to expose primary handlers.

@Deleplace
Copy link
Contributor

The preexisting "similar issue" might be this one (2017): #22085 net/http: document security considerations for serving internet traffic

@itmecho
Copy link

itmecho commented Aug 8, 2023

Would a backwards compatible change be possible where the handlers are moved to a subpackage like net/http/pprof/handlers along with the RegisterHandlers function from this comment? We could then re-export the handlers from the main net/http/pprof package and call the RegisterHandlers function in the init function to maintain the current behaviour.

This wouldn't eliviate the security issue but it gives people a way to use the handlers on a separate mux without being added to the default mux.

@itmecho
Copy link

itmecho commented Oct 9, 2023

Also, for what it's worth, a quick fix for anyone wanting to remove the handlers from the default mux is to add the following init function to your main package:

func init() {
    http.DefaultServerMux = http.NewServeMux()
}

This will replace the mux with the pprof handlers with a fresh one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Development

No branches or pull requests

9 participants